add an option to run overlay operations in the background #4258

marta-lokhova · 2024-03-21T18:39:10Z

First stab at running some of overlay processing in the background. At a high level, the change splits processing of raw inbound/outbound data and higher level business logic of the application. The rationale for the split is that this way we actually get a more-or-less clear cut of responsibilities between overlay and the rest of the application (SCP, tx queue, etc).

In this change the following is moved to the background: socket reads/writes, XDR decoding, HMAC verification, signature verification of SCP messages, tx hashing, flow control outbound broadcasting. Now that we have BL snapshotting available, we can go further and do transaction validation in the background, and possibly exit early to avoid additional burden on the scheduler and the tx queue. For the sake of doing things iteratively, I'd rather do that in a subsequent PR as this change is already quite big.

There are a couple of design choices in this PR that attempt to de-risk it as much as possible. For simplicity, there's a single "overlay lock" and a single "overlay thread". Single lock simplifies synchronization and removes a whole class of deadlock bugs. A single thread seems good enough for now, as our traffic patterns are fairly heavily throttled, so most of the time we're actually idle, but periodically we receive bursts of data that main thread needs to process as fast as possible. So the value of many threads is not clear at the moment given that we're still bottle-necked by the main thread, which runs SCP that keeps the node in sync.

The code is structured such that everything before we get to a valid StellarMessage is managed by the background thread, and the business logic is done on the main thread. When main thread is done processing, some of the outbound processing, like flow control + asio queuing, is also done in the background.

I wanted to get the initial version out to get review going and kick-off discussions on de-risking these changes further, as well as minimizing the impact when the experimental flag is off.

Resolves #4320
Resolves #4309

src/simulation/LoadGenerator.cpp

bboston7

This is an exciting change! It was fun to think about possible interleavings with the backgrounded overlay thread.

src/main/ApplicationImpl.cpp

src/main/Config.cpp

src/overlay/OverlayManagerImpl.h

src/overlay/OverlayManagerImpl.cpp

src/overlay/FlowControl.cpp

src/overlay/FlowControlCapacity.h

src/overlay/Peer.cpp

src/overlay/TCPPeer.h

src/overlay/TCPPeer.cpp

SirTyson

Thanks for putting this together! Definitely a tricky change but I'm looking forward to parallel core! That being said, I don't think we should merge this as is. I haven't finished reviewing this quite yet, but I got particularly stuck on the Peer classes.

I think currently the Peer interface is too difficult to reason about and is error prone. For example, we have two overloads of recvMessage, one that asserts it's the main thread and the other which asserts it's the background thread. We also have multiple ways to get to the same object depending on if a given function is calling from a main thread or background thread context. For example, we have a const copy of Config that the background thread functions use, while main thread functions just call mApp.getConfig(). It's fairly implicit which functions should be able to access mApp and which functions should not. Finally, there's some functions used in both a main thread and background context, like drop, making the boundary even muddier.

Personally, I don't think we should merge this change without some stronger division between threadsafe and non-threadsafe functions in Peer. This is my first venture into the guts of overlay, so some of the confusion may be in part because I'm learning this part of the codebase. That being said, while unfortunately it's a fairly significant amount of work, I feel like a refactor of Peer may be necessary. If we decide not to refactor after all that's fine, but I will need to study a bit more and do a second pass to finish the review :).

SirTyson · 2024-04-11T17:58:28Z

src/main/ApplicationImpl.cpp

+    // start with 1 thread for simplicity, as there isn't enough overlay work to
+    // consume one whole CPU
+    , mHighPriorityIOContext(
+          !!mConfig.EXPERIMENTAL_BACKGROUND_OVERLAY_PROCESSING)


SirTyson · 2024-04-11T18:02:07Z

src/main/ApplicationImpl.cpp

@@ -143,6 +151,8 @@ ApplicationImpl::ApplicationImpl(VirtualClock& clock, Config const& cfg)
        }};
        mWorkerThreads.emplace_back(std::move(thread));
    }
+
+    mOverlayThread = std::thread{[this]() { mHighPriorityIOContext.run(); }};


I think this should be protected behind the mConfig.EXPERIMENTAL_BACKGROUND_OVERLAY_PROCESSING flag.

Good call, done.

SirTyson · 2024-04-11T18:02:41Z

src/main/ApplicationImpl.cpp

+    , mHighPriorityIOContext(
+          !!mConfig.EXPERIMENTAL_BACKGROUND_OVERLAY_PROCESSING)
+    , mHighPriorityWork(
+          std::make_unique<asio::io_context::work>(mHighPriorityIOContext))


Should mHighPriorityWork be nullptr iff !mConfig.EXPERIMENTAL_BACKGROUND_OVERLAY_PROCESSING?

Yep, updated.

SirTyson · 2024-04-12T17:37:51Z

src/overlay/FlowControl.h

-    Application& mApp;
+    OverlayMetrics& mOverlayMetrics;
+    OverlayManager& mOverlayManager;
+    Config const mConfig;


Why not Config const& here? We have references to "main thread" state already with mOverlayMetrics so I don't see why we need to copy here.

In general, in a multi-threaded environment, I prefer a copy to a reference to avoid auditing the code for thread-safety and prevent footguns. This works when performance isn't an issue of course: in this case I think it's fine since Config is a tiny const object stored in each peer.

I'd ideally like to do the same for OverlayMetrics (e.g. deal with copies and do message passing of sorts), but that's more involved, so maybe as a follow-up.

SirTyson · 2024-04-12T17:40:27Z

src/overlay/FlowControl.h

@@ -54,7 +72,12 @@ class FlowControl
    std::shared_ptr<FlowControlCapacity> mFlowControlCapacity;
    std::shared_ptr<FlowControlByteCapacity> mFlowControlBytesCapacity;

-    Application& mApp;
+    OverlayMetrics& mOverlayMetrics;


It might be worth auditing medida for thread safety. Unfortunately, the library is like 12 years old, the docs page is 404, and there are no comments, so I don't know for sure if it's thread safe. Doing a pass through it looks like all relevant medida state is protected with std::atomic or a mutex so we should be ok, but it might warrant a closer look.

SirTyson · 2024-04-12T21:22:01Z

src/overlay/OverlayManagerImpl.cpp

@@ -346,6 +366,8 @@ OverlayManagerImpl::start()
 OverlayManager::AdjustedFlowControlConfig
 OverlayManagerImpl::getFlowControlBytesConfig() const
 {
+    std::lock_guard<std::recursive_mutex> lock(mOverlayMutex);


Why do we need the lock here? It looks like the only value here that can change is HerderImpl::mMaxTxSize. That value is modified in HerderImpl::start and HerderImpl::maybeHandleUpgrade, neither of which acquire mOverlayMutex so currently this lock is useless. I think we need to acquire the mutex in maybeHandleUpgrade

SirTyson · 2024-04-12T21:28:33Z

src/overlay/OverlayManagerImpl.cpp

We should make OverlayManagerImpl::resolvePeers(std::vector<string> const& peers) static. It currently doesn't acquire the lock (and it doesn't need to), but we should make this static to further protect against unsafe member access.

I agree it should be static, but given that data safety isn't a concern here anymore (the overlay lock was removed in favor of more granular locking), let's make this change later as to not further expand the scope of this PR.

SirTyson · 2024-04-12T22:36:23Z

src/overlay/OverlayManagerImpl.cpp

@@ -101,6 +101,7 @@ Peer::pointer
 OverlayManagerImpl::PeersList::byAddress(PeerBareAddress const& address) const
 {
    ZoneScoped;
+


While it looks like all the calls into PeerList are properly guarded by the overlay lock, I'd still like the PeerList functions to acquire the lock too just to be on the safe side.

SirTyson · 2024-04-12T22:47:21Z

src/overlay/TCPPeer.h

@@ -102,6 +119,8 @@ class TCPPeer : public Peer

    virtual ~TCPPeer();

+    // Thread-safe peer drop (can be initiated from either main or overlay
+    // thread)
    virtual void drop(std::string const& reason, DropDirection dropDirection,


Why is drop called from the Overlay thread? I'm not quite sure I understand the main thread-background thread handoff when it comes to peer management. It seems like the mainthread-background cutoff should be main thread manages connections, background handles message in/out. This seems to muddy the water a bit.

Drop can be initiated from either thread, because there are different reasons for dropping a connection. E.g. we may encounter a super low-level ASIO error from background, or main thread deciding that peer doesn't follow the protocol at that (higher) layer of abstraction. That being said, in the updated version there is no need to synchronize peer lists, and the responsibilities are separated such that peer list is always handled on main, while socket shutdown is handled in the background.

SirTyson · 2024-04-12T23:41:53Z

src/overlay/FlowControl.cpp

@@ -281,6 +351,7 @@ FlowControl::endMessageProcessing(StellarMessage const& msg,
                                  std::weak_ptr<Peer> peer)
 {
    ZoneScoped;
+    releaseAssert(threadIsMain());


To be on the safe side, I'd like to acquire the overlay lock in beginMessageProcessing and endMessageProcessing

src/main/Config.cpp

marta-lokhova · 2024-05-09T19:12:30Z

Thanks all for the feedback! I pushed a refreshed version of the PR. Working through a few remaining items, like new unit tests and a small remaining todo to make peer's string address thread-safe, but here's an overview of the updates:

Updates to peer API: explicitly require most public functions to be main-thread only, audit the rest for thread-safety.
Synchronize peer state with an explicit fine-grained lock, remove global locking to make it easier to reason about synchronization. As part of this, add getters and setters for Peer’s state, that all require a lock guard, hinting to the caller that the lock is needed. Hide any other access to state outside of peer. If callers need to access it, they can pass a lambda and let Peer handle the synchronization.
Flow control: remove dependency on Peer completely by removing the send callback. The goal is to prevent flow control from ever calling any method in Peer. This is needed since we have two locks now: Peer’s state lock and flow control lock, they have to be ordered, so the order of locking should always be peer lock followed by flow control. This way we can guarantee the two new locks never deadlock.
Get rid of any locking at the peer lists layer: the problem with synchronizing peer lists is that it bleeds into too many subsystems past overlay, so it's hard to reason about and review. The updated design avoids the need to synchronize peer lists completely: only the main thread maintains peer lists and officially "drops" peers. The background thread asynchronously watches for any state changes, and closes the socket. Note that this pattern already exists in master today, where we delay the actual socket shutdown on the main thread, while the Peer is dropped from the OverlayManager point of view, so I'm just re-using it but in the background.

SirTyson

Thanks for doing all the refactoring work, this is significantly more clear and easier to follow! I left a few nit pics and and questions, and still probably want to walk through it together to make sure I understand everything before signing off, but it's looking good!

SirTyson · 2024-05-10T17:17:21Z

src/main/ApplicationImpl.cpp

-                             : nullptr)
+    , mEvictionIOContext(
+          mConfig.EXPERIMENTAL_BACKGROUND_EVICTION_SCAN
+              ? std::make_unique<asio::io_context>(DEFAULT_THREAD_COUNT)


I think this should stay hardcoded to 1. DEFAULT_THREAD_COUNT feels to me a lower bound value that might increase over time as we become more parallel and get beefier machines. For overlay, DEFAULT_THREAD_COUNT values of 2 or 4 would make sense in the future. However, we will only every need a single thread for background eviction, as it's a single task that is not parallizable itself, so I'd like to keep the hardcoded 1.

SirTyson · 2024-05-10T17:18:43Z

src/main/ApplicationImpl.cpp

+
+    if (mConfig.EXPERIMENTAL_BACKGROUND_OVERLAY_PROCESSING)
+    {
+        mOverlayThread = std::thread{[this]() { mOverlayIOContext->run(); }};


Do we need to set priority here? Or is it implicit that these threads maintain the same priority as main thread? If implicit, a comment would be nice.

As overlay processing is fairly high-priority and time-sensitive, I didn't modify priority. Added a comment though.

SirTyson · 2024-05-10T17:20:08Z

src/main/ApplicationImpl.cpp

@@ -981,6 +1008,13 @@ ApplicationImpl::joinAllThreads()
    }

    LOG_DEBUG(DEFAULT_LOG, "Joined all {} threads", mWorkerThreads.size());
+    if (mOverlayThread)


Nit: Change eviction thread logs to INFO for consistency.

SirTyson · 2024-05-10T17:21:07Z

src/main/ApplicationImpl.cpp

+        mOverlayThread->join();
+    }
+
+    LOG_INFO(DEFAULT_LOG, "Joined all {} threads", (mWorkerThreads.size() + 1));


You should remove LOG_DEBUG(DEFAULT_LOG, "Joined all {} threads", mWorkerThreads.size());, as right now we print two identical thread join messages with different numbers.

SirTyson · 2024-05-10T17:25:36Z

src/overlay/Peer.h

+        // Since MsgCapacityTracker is passed around between thread and stored
+        // by ASIO in lambdas, do not rely on RAII to trigger completion.
+        // Instead, explicitly call `finish`.
+        void finish();
    };

    OverlayAppConnector mAppConnector;

    Hash const mNetworkID;
    std::shared_ptr<FlowControl> mFlowControl;


std::shared_ptr<FlowControl> const

Good idea! there are some test dependencies sadly, which override mFlowControl. I can fix this in a follow-up though after this PR lands.

SirTyson · 2024-05-10T19:50:48Z

src/overlay/Floodgate.cpp

@@ -137,6 +139,9 @@ Floodgate::broadcast(std::shared_ptr<StellarMessage const> msg,
                mSendFromBroadcast.Mark();
                std::weak_ptr<Peer> weak(
                    std::static_pointer_cast<Peer>(peer.second));
+                // This is an async operation, and peer might get dropped by the
+                // time we actually try to send the message This is fine, as


Nit: Missing .

SirTyson · 2024-05-10T20:34:18Z

src/overlay/TCPPeer.cpp

+            auto result = weak.lock();
+            if (!result)
+            {
+                // If peer was rejected was reasons like no pending slots


Typo: for reasons

src/overlay/TCPPeer.h

SirTyson · 2024-05-10T20:39:17Z

src/overlay/TCPPeer.h

+            return mSocketShutdownScheduled;
+        }
+        void
+        scheduleSocketShutdown(bool value)


Remove bool value. This should only every be called once anyway and true is the only valid value, so remove parameter.

src/overlay/TCPPeer.cpp

bboston7

This is looking great! I just had a few small comments.

src/overlay/Peer.h

src/overlay/Peer.cpp

src/overlay/TCPPeer.cpp

src/overlay/test/LoopbackPeer.cpp

…s are called on main

…ibility of deadlocks via lock ordering

…p modes are gone

…ted peers to Dropped for consistency

…ket creation safety

…thout_hashing add an option to run overlay operations in the background Reviewed-by: graydon

graydon · 2024-05-29T23:42:29Z

r+ 894ef3b

graydon · 2024-05-29T23:52:01Z

@latobarita: retry

…thout_hashing add an option to run overlay operations in the background Reviewed-by: graydon

graydon · 2024-05-30T03:06:24Z

@latobarita: retry

marta-lokhova force-pushed the parallelize_ov_initial_without_hashing branch 7 times, most recently from bf80d96 to fc2cb17 Compare March 26, 2024 04:11

marta-lokhova mentioned this pull request Mar 29, 2024

Parallel bl db #4176

Merged

6 tasks

marta-lokhova commented Mar 29, 2024

View reviewed changes

src/simulation/LoadGenerator.cpp Outdated Show resolved Hide resolved

bboston7 reviewed Mar 29, 2024

View reviewed changes

marta-lokhova mentioned this pull request Apr 10, 2024

Overlay stability fixes #4279

Merged

marta-lokhova force-pushed the parallelize_ov_initial_without_hashing branch from e943fb8 to 01f6354 Compare April 11, 2024 03:51

marta-lokhova marked this pull request as ready for review April 11, 2024 05:14

marta-lokhova requested review from graydon and SirTyson April 11, 2024 05:14

marta-lokhova changed the title ~~add an option to run overlay operation in the background~~ add an option to run overlay operations in the background Apr 11, 2024

marta-lokhova force-pushed the parallelize_ov_initial_without_hashing branch from 32be063 to e53473d Compare April 12, 2024 23:40

SirTyson suggested changes Apr 13, 2024

View reviewed changes

marta-lokhova self-assigned this Apr 16, 2024

bboston7 mentioned this pull request Apr 22, 2024

Add new time sliced overlay survey #4275

Merged

13 tasks

marta-lokhova mentioned this pull request Apr 22, 2024

refactor some overlay interfaces #4293

Merged

marta-lokhova force-pushed the parallelize_ov_initial_without_hashing branch 2 times, most recently from f20f9e4 to b198d93 Compare May 9, 2024 00:54

marta-lokhova commented May 9, 2024

View reviewed changes

src/main/Config.cpp Outdated Show resolved Hide resolved

marta-lokhova force-pushed the parallelize_ov_initial_without_hashing branch from b198d93 to fb9b38e Compare May 10, 2024 18:58

SirTyson reviewed May 10, 2024

View reviewed changes

bboston7 reviewed May 13, 2024

View reviewed changes

marta-lokhova force-pushed the parallelize_ov_initial_without_hashing branch 2 times, most recently from 995461a to 70c649d Compare May 17, 2024 20:57

marta-lokhova and others added 22 commits May 29, 2024 16:15

Refresh interface of Peer, support new background flag

bf3d30e

OverlayManager: extend lifetime of dropped peers to ensure destructor…

4198a5a

…s are called on main

use thread-safe Peer's methods outside of Peer

49c5ea8

Update TCPPeer to support parallel processing

0c277fc

Update tests to support the new Peer API

d935e40

FlowControl: remove dependency on Peer completely to guarantee imposs…

626fb9e

…ibility of deadlocks via lock ordering

Fix rebase fallout and warnings

faa5bb7

Rename confusing Peer::shutdown, remove redundant mShuttingDown

619c84f

Remove redundant drop modes, rely on socket shutdown delay

814d66d

Resurrect interrupt-style shutdown as opposed to polling now that dro…

5f167f2

…p modes are gone

Remove dead code

4df89bf

Add a test for TCPPeer lifetime during initiate and accept; add rejec…

eff78dd

…ted peers to Dropped for consistency

Make IP string retrieval thread-safe

da11044

FlowControl: no need to use recursive_mutex, switch to regular mutex

e6e3188

Improvements, most importantly, synchronize access to PRNG in key cache

72f754f

Tracy: draw Peer state lock

656c43b

No need to hold the lock during consensus; also add commentary on soc…

18f7d20

…ket creation safety

Minor tweaks to Peer/TCPPeer

98e5fa6

factor out hmac machinery from overlay

9dddaf9

Use MsgCapacityTracker destructor to track capacity

b1b7293

Code improvements as per feedback and add @graydon's comments

da6b10c

Turn off experimental flag by default

894ef3b

marta-lokhova force-pushed the parallelize_ov_initial_without_hashing branch from 2904683 to 894ef3b Compare May 29, 2024 23:21

latobarita added a commit that referenced this pull request May 29, 2024

Merge pull request #4258 from marta-lokhova/parallelize_ov_initial_wi…

ff1acff

…thout_hashing add an option to run overlay operations in the background Reviewed-by: graydon

latobarita added a commit that referenced this pull request May 30, 2024

Merge pull request #4258 from marta-lokhova/parallelize_ov_initial_wi…

4e2eb6a

…thout_hashing add an option to run overlay operations in the background Reviewed-by: graydon

latobarita merged commit c891524 into stellar:master May 30, 2024
15 checks passed

marta-lokhova mentioned this pull request Jun 3, 2024

overlay: perf and error handling improvements #4343

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add an option to run overlay operations in the background #4258

add an option to run overlay operations in the background #4258

marta-lokhova commented Mar 21, 2024 •

edited

bboston7 left a comment

SirTyson left a comment

SirTyson Apr 11, 2024

SirTyson Apr 11, 2024

marta-lokhova May 9, 2024

SirTyson Apr 11, 2024

marta-lokhova May 9, 2024

SirTyson Apr 12, 2024

marta-lokhova May 9, 2024

SirTyson Apr 12, 2024

SirTyson Apr 12, 2024

SirTyson Apr 12, 2024

marta-lokhova May 9, 2024

SirTyson Apr 12, 2024

SirTyson Apr 12, 2024

marta-lokhova May 9, 2024

SirTyson Apr 12, 2024

marta-lokhova commented May 9, 2024

SirTyson left a comment

SirTyson May 10, 2024

SirTyson May 10, 2024

marta-lokhova May 20, 2024

SirTyson May 10, 2024

SirTyson May 10, 2024

SirTyson May 10, 2024

marta-lokhova May 20, 2024

SirTyson May 10, 2024

SirTyson May 10, 2024

SirTyson May 10, 2024

bboston7 left a comment

graydon commented May 29, 2024

graydon commented May 29, 2024 •

edited

graydon commented May 30, 2024

add an option to run overlay operations in the background #4258

add an option to run overlay operations in the background #4258

Conversation

marta-lokhova commented Mar 21, 2024 • edited

bboston7 left a comment

Choose a reason for hiding this comment

SirTyson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marta-lokhova commented May 9, 2024

SirTyson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboston7 left a comment

Choose a reason for hiding this comment

graydon commented May 29, 2024

graydon commented May 29, 2024 • edited

graydon commented May 30, 2024

marta-lokhova commented Mar 21, 2024 •

edited

graydon commented May 29, 2024 •

edited