rdisp: add support for moving LtHash off the critical path - #11527
Open
ptaffet-jump wants to merge 1 commit into
Open
ptaffet-jump wants to merge 1 commit into
ptaffet-jump wants to merge 1 commit into
Conversation
┌─ ⚡ PERF · f86485d vs main@7c0e042 ─────────────────────────────────
│ SUITE BASELINE NEW Δ
│ replay tps, mainnet 29,279 tps 29,256 tps · -0.08%
│ bench tps, localnet 834,333 tps 832,168 tps · -0.26%
│ snapshot load, testnet 15.53 s 15.50 s · -0.21%
!│ mem total, mainnet 164.38 GiB 165.38 GiB ▲ +0.61%
-│ mem total, testnet 95.38 GiB 96.38 GiB ▲ +1.05%
│ clean compile, firedancer 335.2 cpu·s 336.2 cpu·s · +0.30%
│ binary size, firedancer 68.43 MB 68.50 MB · +0.10%
├─────────────────────────────────────────────────────────────────────
@@ 1 REGRESSION · 1 WARNING · 0 IMPROVED · 5 NOISE @@
└─────────────────────────────────────────────────────────────────────history · 2 pushes ┌─ HISTORY · Δ vs main, per push, newest first ─────────────────────────
│ HEAD TPS BENCH SNAP MEM·M MEM·T COMPILE BINARY
-│ f86485d -0.08% -0.26% -0.21% +0.61% +1.05% +0.30% +0.10%
-│ e86ce2f -0.25% +0.04% -0.24% +0.61% +1.05% +3.34% +0.10%
└─────────────────────────────────────────────────────────────────────── |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Pending-queue sizing, scratch initialization, aliasing, and test-input defects can cause incorrect or undefined behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds LtHash pseudo-transactions to replay dispatching so account hashing can run outside the critical path.
Changes:
- Tracks writable accounts and schedules LtHash pseudo-transactions.
- Adds a per-lane priority queue and tests.
- Updates dispatcher APIs, callers, memory sizing, and verification.
File summaries
| File | Description |
|---|---|
src/discof/replay/fd_rdisp.c |
Implements LtHash tracking and pseudo-transaction scheduling. |
src/discof/replay/fd_rdisp.h |
Exposes and documents the new APIs. |
src/discof/replay/fd_rdisp_mq.h |
Adds the account priority queue. |
src/discof/replay/test_rdisp.c |
Tests LtHash scheduling behavior. |
src/discof/replay/test_rdisp_mq.c |
Tests priority-queue operations. |
src/discof/replay/fd_sched.c |
Updates the scheduler call site. |
src/discof/replay/fuzz_sched_rdisp.c |
Updates the fuzz mirror call site. |
src/discof/replay/Local.mk |
Builds and runs the new test. |
Review details
Suppressed comments (1)
src/discof/replay/test_rdisp_mq.c:23
- This repeats the zero-based calculation from insertion, so the first pop also uses the contractually invalid account index 0. Compute the same one-based cyclic index here.
ulong acct_idx = fd_ulong_if( outer+inner<=20UL, outer+inner, outer+inner-19UL );
- Files reviewed: 8/8 changed files
- Comments generated: 12
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+202
to
+206
| struct fd_rdisp_ptxn { | ||
| /* These fields have the same meaning as in fd_rdisp_txn_t */ | ||
| uint in_degree; | ||
| float score; | ||
| union { |
| /* Test the LtHash support */ | ||
| FD_TEST( 0==fd_rdisp_add_block( disp, tag( 0UL ), 1UL ) ); | ||
|
|
||
| ulong w[1]; |
| hand, when adding a transaction, rdisp optionally populates a bitmask | ||
| indicating which writable accounts have not been marked as writable | ||
| in a transaction inserted since the most recent pseudo-transaction | ||
| was created for it. That's a little obtuse, but essenitally creating |
Comment on lines
+501
to
+504
| must be in the PENDING, READY, or DISPATCHED state, which means | ||
| txn_idx must have been returned from get_next_ready and must have | ||
| FD_RDISP_LTHASH_PSEUDO_TXN set. Note that this means once you call | ||
| complete_txn on a txn_idx, you cannot call this function on it any |
|
|
||
| IMPORTANT: if insert_block is staged, add_pseudo_txn must be called | ||
| until it returns 0 prior to inserting another block in the same | ||
| staging lane. You may also use fd_rdisp_add_all_psuedo_txn for this |
| char const * readonly, /* in [A-z] */ | ||
| int serializing, | ||
| ulong * writers_bitset ) { | ||
| char categorized[3][2][128]; /* (signer, nonsigner, alt) x (writeble, readonly) x accts */ |
ptaffet-jump
force-pushed
the
ptaffet/rdisp-lthash
branch
from
September 21, 2026 20:52
e86ce2f to
f86485d
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Promotion can lose required LtHash work, the fuzz verifier buffer now overflows, and newly added tests contain deterministic failures.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (9)
fd_rdisp_ptxn_tis a distinct struct type, but the new code repeatedly casts its objects to…add_txn3reads and updates*w, but this first declaration leaves it indeterminate. The first…get_next_ready;fd_rdisp_add_pseudo_txn…
Resolved since last review (6)
outer==0 && inner==0, this produces account index 0 even though the queue contract requires…max_writers_per_block > acct_depthis effectively clamped, but both…+2ULis currently added as bytes, while the verifier subsequently indexes two additional…full_depthallocation, but the queue immediately above is still…
Comment on lines
+1126
to
+1131
| fd_rdisp_ptxn_t * ptxn = disp->ptxn_pool + EDGE_SPECIAL_IDX( ele->ptxn_idx ); | ||
| ptxn->in_degree = IN_DEGREE_FREE; | ||
| ptxn_pool_ele_release( disp->ptxn_pool, ptxn ); | ||
|
|
||
| ustlt_dlist_ele_remove ( block->ustlt_created, ele, disp->ustlt_pool ); | ||
| ustlt_dlist_ele_push_tail( disp->ustlt_free, ele, disp->ustlt_pool ); |
| for( ulong inner=1UL; inner<10UL; inner++ ) { | ||
| ulong acct_idx = fd_ulong_if( outer+inner<=20UL, outer+inner, outer+inner-19UL ); | ||
| fd_rdisp_mq_insert( mq, acct_idx, 0UL, (float)inner ); | ||
| FD_TEST( fd_rdisp_mq_cnt( mq, 0UL )==inner+1UL ); |
Comment on lines
+590
to
+592
| with at least depth*(FD_RDISP_MAX_ACCT_PER_TXN+1)+2 elements that | ||
| will be clobbered (its contents at the time of the function call are | ||
| ignored). */ |
This branch has not been deployed
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.
No description provided.