bundle: defer publish to after_credit via pending deque#8763
bundle: defer publish to after_credit via pending deque#8763cmoyes-jump merged 12 commits intofiredancer-io:mainfrom
Conversation
4a7245c to
25d8077
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Firedancer bundle tile's publish path to separate gRPC I/O (before_credit) from stem publishing (after_credit). Instead of publishing directly to the tango message bus during gRPC callbacks, transactions are written to the dcache and their metadata is buffered in a zero-copy dynamic deque (pending_pubs). The after_credit callback then drains the deque: publishing complete bundles atomically (up to STEM_BURST) or batches of up to STEM_BURST individual packets per call. Two new metrics expose the pending buffer depth and any backpressure-induced drops.
Changes:
- New
fd_bundle_pending_pubdeque in the bundle tile's private state, used to stage publish metadata between gRPC I/O andfd_stem_publish. - New
before_credit/after_creditsplit: I/O is gated on the deque being empty and at least 1 credit being available; draining is gated onmin_cr_avail >= STEM_BURST. - New metrics (
PendingTransactionsgauge,TransactionDroppedBackpressurecounter) and associated generated code and documentation.
Reviewed changes
Copilot reviewed 7 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/disco/bundle/fd_bundle_tile.c |
Adds before_credit, moves STEM_BURST earlier, splits after_credit into drain loop + plugin update |
src/disco/bundle/fd_bundle_tile_private.h |
Adds fd_bundle_pending_pub struct, deque template, depth field in fd_bundle_out_ctx, backpressure_drop_cnt metric |
src/disco/bundle/fd_bundle_client.c |
Replaces direct fd_stem_publish calls with deque pushes; adds per-publish backpressure checks |
src/disco/bundle/test_bundle_common.c |
Allocates/destroys the deque in the test harness, adds depth to test verify_out |
src/disco/bundle/test_bundle_client.c |
Updates existing tests to check deque count instead of mcache; adds three new drain-logic unit tests |
src/disco/metrics/metrics.xml |
Adds two new bundle metrics and two unrelated snapct gossip metrics |
src/disco/metrics/generated/fd_metrics_bundle.h |
Regenerated header reflecting new metric offsets |
src/disco/metrics/generated/fd_metrics_bundle.c |
Regenerated source with new DECLARE_METRIC entries |
book/api/metrics-generated.md |
Documentation updated for new bundle and snapct metrics |
You can also share your feedback on Copilot code review. Take the survey.
3487482 to
0445c02
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 11 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review. Take the survey.
jherrera-jump
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
| <histogram name="MessageRxDelayNanos" min="100000" max="1000000000"> | ||
| <summary>Message receive delay in nanoseconds from bundle server to bundle client</summary> | ||
| </histogram> | ||
| <gauge name="PendingTransactions" summary="Number of transactions buffered and waiting to be published" /> |
There was a problem hiding this comment.
PendingTransactions here is probably less useful as a metric due to how slow consumers poll metrics compared to the rate of change of the metric itself.
Maybe instead this should be the number of times pending_transaction_cnt exceeds pending_max/2?
Also don't feel too strongly about this so fine to leave it as-is.
There was a problem hiding this comment.
if others also feel this is worth doing, I will change, how does that sound?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review. Take the survey.
6d8ae29 to
3a9cf13
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
| ulong pending_max = tile->bundle.buf_sz / FD_BUNDLE_MIN_GRPC_WIRE_SZ; | ||
| ulong l = FD_LAYOUT_INIT; | ||
| l = FD_LAYOUT_APPEND( l, alignof(fd_bundle_tile_t), sizeof(fd_bundle_tile_t) ); | ||
| l = FD_LAYOUT_APPEND( l, fd_grpc_client_align(), fd_grpc_client_footprint( tile->bundle.buf_sz ) ); | ||
| l = FD_LAYOUT_APPEND( l, fd_alloc_align(), fd_alloc_footprint() ); | ||
| l = FD_LAYOUT_APPEND( l, pending_txn_align(), pending_txn_footprint( pending_max ) ); |
There was a problem hiding this comment.
The pending transaction deque is sized as buf_sz / FD_BUNDLE_MIN_GRPC_WIRE_SZ entries, but each fd_bundle_pending_txn_t entry occupies approximately 1304 bytes (1232 bytes payload + metadata). For the default buffer_size_kib = 2048 (fdctl), this yields ~131,072 entries × 1304 bytes ≈ 171 MB just for the deque. For buffer_size_kib = 16384 (firedancer), it yields ~1.37 GB.
The sizing rationale is correct for overflow prevention (the gRPC frame buffer limits inbound transactions), but the deque entries are ~81x larger than the minimum wire format assumption (16 bytes). In practice, a much smaller deque (e.g., capped at a few hundred entries, since at most STEM_BURST * N transactions are needed to keep the pipeline full) would suffice and avoid this large memory footprint in the tile's scratch space.
There was a problem hiding this comment.
need guidance on whether we can safely size down the theoretical upper bound
There was a problem hiding this comment.
I think you can just size to match the bundle_verif link, i.e. config->tiles.verify.receive_buffer_size / FD_TPU_PARSED_MTU
There was a problem hiding this comment.
I agree, that makes sense, I made your suggested change
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
fea47fe to
c728d5f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
ea82ac1 to
790db69
Compare
unit tests zero copy design more hardening
790db69 to
41dedb4
Compare
Co-authored-by: ripatel-fd <ripatel+git@jumptrading.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review. Take the survey.
Split into
before_credit(I/O, gated on deque empty andcr_avail) andafter_credit(drain one bundle or up toSTEM_BURSTpackets per call). Publish metadata is buffered in a zero-copy deque. dcache writes are capped atcr_availto prevent overwriting data the downstream consumer hasn't read.Fixes #8654