Inbound governor performance improvement #5104
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crocodile-dentist
added
optimisation
crocodile-dentist
force-pushed
the
mwojtowicz/inbound-governor-turbo
branch
3 times, most recently
from
a472b00 to
9664dee
Compare
crocodile-dentist
force-pushed
the
mwojtowicz/inbound-governor-turbo
branch
3 times, most recently
from
b835675 to
aa7c0b3
Compare
bolt12
requested changes
Mar 31, 2025
There was a problem hiding this comment.
Some initial comments, I am not sure I fully understood the solution but I don't think I like the added complexity. Tracers shouldn't be used in this way. I don't think it is clearer what happens when the tracer is a null one, for example. Implicit behaviours hidden in tracers sounds like a nightmare to debug as well. Did I understand it right?
ouroboros-network-framework/src/Ouroboros/Network/ConnectionHandler.hs
Outdated
Show resolved
Hide resolved
ouroboros-network-framework/src/Ouroboros/Network/InboundGovernor.hs
Outdated
Show resolved
Hide resolved
ouroboros-network/testlib/Test/Ouroboros/Network/Diffusion/Testnet/Cardano/Simulation.hs
Outdated
Show resolved
Hide resolved
ouroboros-network-framework/src/Ouroboros/Network/ConnectionHandler.hs
Outdated
Show resolved
Hide resolved
ouroboros-network-framework/src/Ouroboros/Network/InboundGovernor.hs
Outdated
Show resolved
Hide resolved
crocodile-dentist
force-pushed
the
mwojtowicz/inbound-governor-turbo
branch
2 times, most recently
from
982787b to
0ea6769
Compare
We use tracers in this way for ranking peers. I think this is what tracers excel at. |
crocodile-dentist
force-pushed
the
mwojtowicz/inbound-governor-turbo
branch
11 times, most recently
from
d4381fe to
37e02b0
Compare
crocodile-dentist
force-pushed
the
mwojtowicz/inbound-governor-turbo
branch
4 times, most recently
from
0f9e65c to
025e2d0
Compare
crocodile-dentist
force-pushed
the
mwojtowicz/inbound-governor-turbo
branch
from
025e2d0 to
895a56b
Compare
coot
reviewed
May 15, 2025
ouroboros-network-framework/src/Ouroboros/Network/InboundGovernor.hs
Outdated
Show resolved
Hide resolved
This change replaces the mechanism heavily relying on STM
to track remote activity by the inbound governor.
Some performance tests indicate that there is a not-insignificant
performance penalty with that approach.
In the new approach, the inbound governor creates a tracer which is passed via the
connection manager to the connection handler. The handler joins
this tracer with the mux tracer such that the muxer main thread,
via various traces, writes to the information channel queue of events
such as remote promotions/demotions and mux stopping/erroring.
Each connection maintains a count of remote hot/non-hot responders
which are adjusted by the appropriate traces, and informing the
inbound governor of relevant state changes that need to be performed.
Change RemoteIdle tag:
* The change is motivated by the need of the new tracer
to ensure proper sequencing of events on the queue. In case
a connection is expired and responder startup is demanded, the tracer
will retry until the connection is RemoteCold to register the
promotion, or abort when the connection is dropped (CM CommitTr returned).
Moved 'InboundGovernorInfoChannel` to InboundGovernor Change IG information channel queue bound Previously, the queue was only used to communicate new connections to the inbound governor. The queue is now used to also notify the IG of muxer events so it will be busier.
Applies the connection handler builder to the new IG tracer and passes
the result to the withConnectionManager continuation. The connection
manager forks this tracer-augmented handler for new connections so
that the inbound governor can be efficiently notified of remote
activity by tracing performed by mux.
The information channel queue is drained one last time after the
inbound governor thread finishes. This is to avoid a deadlock where
the queue becomes full, potentially preventing connection handlers from
performing their cleanup routine. This is important when shutting down,
where the connection manager is waiting for all connection handlers to finish.
Refactor InboundGovernor interface
Rework IG loop
Process all the info channel events from the queue in one step
of the IG loop. The queue events arrive from the CM (new connection)
or from the tracer which tracks miniprotocol responder activity
and mux start/stop.
Move the remnants of the deleted Event module
* Most of the functionality of the Event is unneeded anymore
and the module is removed, with its still useful remnant moved
here. Updates reflecting changes to 'RemoteIdle' tag were applied.
Also reflects changes to 'RemoteIdle' tag
Remove unnecessary unregisterConnection call sites
InformationChannel was moved under InboundGovernor directory Update Args and API Integrates prior commits
Reordered some traces to be sequenced after some state was updated. Introduce Mux.Tracers type Contains mux tracer and separate channel tracer. The idea is that the channel tracer should not contain the inbound governor's information channel tracer which may be present in the muxTracer field for certain connections. The protocol channel send/receive trace tags are uniteresting from the IG tracer's perspective but there is a penalty for invoking it so frequently for every complete message.
crocodile-dentist
force-pushed
the
mwojtowicz/inbound-governor-turbo
branch
from
895a56b to
82beb03
Compare
coot
reviewed
May 19, 2025
| let traceWithBearer = contramap $ Mx.WithBearer connectionId | ||
| unmask $ Mx.run Mx.Tracers { | ||
| muxTracer = traceWithBearer (muxTracer <> inboundGovernorMuxTracer countersVar), | ||
| channelTracer = traceWithBearer muxTracer } |
There was a problem hiding this comment.
Is this change related to this branch/PR or it has more to do with #5121?
There was a problem hiding this comment.
It's related to 5121, but it makes sense to unmask async exceptions after delivering the negotiated connection values to the CM, which is blocked waiting for it.
Attach the IG tracer to Mux tracer before calling Mx.run The tracer is attached to the inbound handler and to the outbound handler only when a duplex mode was negotiated. Apply unmask only when calling Mx.run to guarantee in case of receiving an async exception that the promise of handshake negotiation result is delivered to the CM which is blocked waiting for it.
The bug was exposed when Mux.run signature was changed to accept the new MuxTracerBundle type instead of the Mux.Trace's Trace type. The latter Trace type mixes low level bearer tags with the higher level mux tags, so logically they are separate and in fact are traced by separate components. The headerTracer must go along with the bearer tracer otherwise it doesn't trace anything, but the types matched so the program was accepted.
Integrates prior commits
integrates prior commits
No other significant changes besides cosmetics.
This addresses the shrinker hang when attempting to shrink 'AbsBearerInfo' in some cases.
Fixes termination bug in testnet command shrinking by pruning duplicate commands. Added Skip command to diffusion testnet script. This is only generated for failed test cases by the shrinker. Sometimes we can generate a simpler command script by dropping a specific command, or a whole series of commands, and extend the delays (in effect, 'unshrinking' the delay of when the command is applied) of the remaining ones For eg. JoinNetwork 0 : Reconfigure 5 : Reconfigure 10 : Reconfigure 15 : Kill 2 Can be simplied to JoinNetwork 0 : Kill 32 when the intermediate reconfigures are skipped - the shrinker changes them to Skip and the candidate is passed here. It is important that the overall test duration is not shrunk at this stage, since a failed test might depend on some minimal run duration. If the property still fails, then the original commands are not contributing factors, but what is important is that the node has to run at least that long for the test case to fail. With such a simplified command script, the execution duration can be shrunk in the following shrinker iterations. Some commands may be just noise, and all they do is cause unnecessary work slowing down the shrinker as it attempts to shrink them individually. This only increases the search space without any benefit in the end. The skips are removed from the fixed up script, just the effect of their duration is applied to the following first non-skip command. If a specific command is in fact significant for the property to fail, for eg. the test passes if Reconfigure is changed to Skip, the shrinker will backtrack and keep that command and continue with shrinking the rest. The bottom line is that the minimal example and running time are both improved, significantly as observed in practice. For o-n-framework experiments shrinker, we shrink AcceptedConnectionLimits fields one by one, instead of simultaneously all at once, for a better minimal result.
This annotes each trace with a nice (node-x), where x is a number [1..max-node in simulation]. This facilitates grepping a trace for just the node that failed a test. In the mkTracers binding, one can either turn on all component tracers for general testing, and in case of failure of a test where only a subset of components are really needed, one can selectively toggle just the interesting sayTracers to further cut down the noise.
Comprehensibility improvements Integrates prior changes
crocodile-dentist
force-pushed
the
mwojtowicz/inbound-governor-turbo
branch
from
edff5fd to
2719dc4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The change is expected to reduce CPU load and is designed to scale with the number of inbound connections served by a node. In particular, bootstrap relays and Genesis peers are mostly affected, but all relays in general should benefit from these improvements.
Profiling a real loaded node revealed a CPU bottleneck in the inbound governor loop, which essentially single-threads the application. Specifically, the firstPeerTo*
FirstToFinishactions are CPU intensive in the amount of state checking performed for all active connections, repeatedly for each such action in a linear fashion. Casting aside the STM costs, there was in particular also duplication of filtering of hot/warm responder miniprotocols for each firstToFinish* activity. In terms of the the STM costs, especially the embeddedLastToFinishcomputations are expensive because of lack of short-circuiting of STMretrysemantics. For instance, whenLastToFinish 1 <> 2 <> 3is attempted, and1and2retry, but3succeeds, then2is attempted again, and if it succeeds then1is attempted again. From a performance perspective, it would be better to abort that peer's firstToFinish activity on the first retry and go try the next peer, but I'm not sure if that is supported.This PR introduces an event based system where each connection's muxer thread can concurrently notify the inbound governor of interesting peer activity such that it can perform its tasks. The inbound governor creates a tracer which enqueues muxer events from each peer that it knows about into its information channel queue. This queue is processed in batch where all ordering of events is preserved. In particular, muxer start, stop, miniprotocol clean & exception exits are tracked. Some additional state is also introduced which tracks the number of hot and non-hot (warm & established) responder miniprotocols such that remote promotions and demotions can be identified efficiently.
The commit history is sequenced in a bottom up fashion, and while showing the 'guts' of how the new event system operates, it masks at first sight how this is folded into the program as a whole and what other reorganizations are needed from the following commits. In particular, a difficulty manifested because currently the CM is instantiated at the front, but the idea behind the proposed change is that the new tracer should be baked into the connection handler by the inbound governor, which should be provided to the CM as the penultimate step before starting the server. When a new outbound or incoming connection is made, the CM forks this augmented connection handler, which then adjoins this new tracer to the mux tracer before calling
Mux.run. In this manner, the new tracing is only performed for connections served by the inbound handler, or the outbound handler only when a duplex data flow was negotiated. This reorganization is best viewed in the commit ofDiffusionmodule refactoring.Other changes include:
prop_mux_starvationtestChecklist
Quality
Maintenance
ouroboros-networkproject.