perf: Deserialize received nodes once and only in job queue#7420
perf: Deserialize received nodes once and only in job queue#7420bthomee wants to merge 11 commits into
Conversation
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
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.
bthomee
added
QE test desired
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7420 +/- ##
=========================================
- Coverage 82.3% 82.3% -0.0%
=========================================
Files 1024 1024
Lines 78456 78466 +10
Branches 8973 8980 +7
=========================================
- Hits 64575 64566 -9
- Misses 13872 13891 +19
Partials 9 9
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
The performance evaluations can be found in https://gist.github.com/bthomee/3a0cb89a9709dc00b3f3e3bc0c329736. I included an AI generated summary of the findings. |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
@ximinez In this PR I removed the deserialization of received nodes from the code before the message is added to the job queue, since the deserialization was also being done inside the job queue, and I assumed that it would be healthier for the node to check the validity of the message inside the job queue (since it has many threads consuming from it) as opposed to where the inbound messages are received (which may only use one thread or fewer). However, if someone were to send a message containing many (intentionally) invalid nodes, would it be better to prevent such messages from being added to the job queue in the first place? I can rewrite the logic to only deserialize the nodes before inserting the message and those deserialized nodes into the jq. |
ximinez
left a comment
ximinez
left a comment
There was a problem hiding this comment.
The changes are good. I suggested a couple of tweaks.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
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.