perf: Deserialize received nodes once and only in job queue#7420
Open
bthomee wants to merge 6 commits into
Open
perf: Deserialize received nodes once and only in job queue#7420bthomee wants to merge 6 commits into
bthomee wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves TMGetLedger SHAMap node-ID deserialization and integrity checking out of the I/O-thread onMessage handler and into the job-queue path, eliminating the prior “deserialize twice” behavior and reducing I/O-thread work during ledger-fetch traffic.
Changes:
- Removed per-node
SHAMapNodeIDdeserialization/validation from the I/O thread and deferred it into theJtLedgerReqjob. - Updated
processLedgerRequestto accept already-parsedstd::vector<SHAMapNodeID>and use it when buildingTMLedgerDatareplies. - Minor internal refactors in node reply assembly (loop structure, variable naming, reserving capacity once).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/xrpld/overlay/detail/PeerImp.h | Extends processLedgerRequest to accept a parsed std::vector<SHAMapNodeID> and adds required include. |
| src/xrpld/overlay/detail/PeerImp.cpp | Defers node-id parsing to the job queue and adjusts processLedgerRequest to use the parsed node IDs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7420 +/- ##
=========================================
- Coverage 82.3% 82.3% -0.0%
=========================================
Files 1011 1011
Lines 76913 76919 +6
Branches 8964 8968 +4
=========================================
- Hits 63336 63334 -2
- Misses 13568 13576 +8
Partials 9 9
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Collaborator
Author
|
The performance evaluations can be found in https://gist.github.com/bthomee/3a0cb89a9709dc00b3f3e3bc0c329736. I included an AI generated summary of the findings. |
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
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.
High Level Overview of Change
This change defers the deserialization of received nodes from the I/O thread to the job queue.
Context of Change
The changes here were originally developed as part of #6353, but it's better to pull them into a separate PR whose performance impact can then be evaluated individually.
Currently, received nodes in a
TMGetLedgermessage are deserialized in theOnMessagehandler, which runs in the I/O thread, purely as a sanity check before dispatching to the job queue. The nodes are then deserialized again after a thread picks up the task from the job queue. The proposed change will fully defer the deserialization and integrity checking to the job queue to free up to I/O thread and avoid double work.One new addition is that the peer will now be charged a fee when sending more than
Tuning::kSoftMaxReplyLimitnodes, which protects against a peer sending a request containing a very large number of node IDs. This limit was chosen because the existing code limits the number of nodes returned in the response to that amount* so there is little point in even sending more nodes to be processed.*) The
kSoftMaxReplyLimitis a soft limit; since a requested node ID can return multiple nodes in the response (via thegetNodeFatfunction, which returns the node itself and its children up to a certain depth), the total number of nodes returned may be larger than the soft limit. However, it is hard limited viakHardMaxReplyLimit.