Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[qs] dedup requests to batch fetcher and simplify payload manager #15766

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ibalajiarun
Copy link
Contributor

@ibalajiarun ibalajiarun commented Jan 17, 2025

Description

This PR aims to simplify the PayloadManager logic to retrieve batch payload from quorum store. To reduce complexity, we no longer cache transactions in PayloadManager. As long as quorum store stores the payload in memory, retrieval should be very fast and does not require caching in payload manager.

  • Splits payload_manager.rs into separate modules one each for direct mempool, consensus observer, and quorum store.
  • QuorumStorePayloadManager always requests QuorumStore for payload which will return a future that resolves into the payload upon completion. On prefetch, this future is ignored and BatchReaderImpl will spawn a task to drive the future.
  • BatchReaderImpl implements deduplication logic to ensure there is only one inflight fetch request per batch. It returns a shared future that the caller can await on to get the payload. The future is driven by a tokio task, so the caller don't need to .await immediately to drive the future.

TODO: add counters to measure retrieval duration

Copy link

trunk-io bot commented Jan 17, 2025

⏱️ 36m total CI duration on this PR
Job Cumulative Duration Recent Runs
test-target-determinator 16m 🟩🟩🟩
check-dynamic-deps 9m 🟩🟩🟩🟩
rust-cargo-deny 5m 🟩🟩🟩
semgrep/ci 2m 🟩🟩🟩🟩
general-lints 1m 🟩🟩🟩
file_change_determinator 45s 🟩🟩🟩🟩
file_change_determinator 37s 🟩🟩🟩
permission-check 16s 🟩🟩🟩🟩🟩
permission-check 13s 🟩🟩🟩🟩🟩
permission-check 7s 🟩🟩🟩
determine-docker-build-metadata 6s 🟩🟩🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

@ibalajiarun ibalajiarun added the CICD:run-forge-e2e-perf Run the e2e perf forge only label Jan 17, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@ibalajiarun ibalajiarun changed the title Balaji/optqs fetch [qs] dedup requests to batch fetcher and simplify payload manager Jan 17, 2025
@ibalajiarun ibalajiarun marked this pull request as ready for review January 17, 2025 23:02

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 580c7946be44a8f68e99b2968f9bfc93f58ad767

two traffics test: inner traffic : committed: 13497.42 txn/s, submitted: 13498.26 txn/s, expired: 0.84 txn/s, latency: 2940.67 ms, (p50: 3000 ms, p70: 3000, p90: 3200 ms, p99: 3900 ms), latency samples: 5131980
two traffics test : committed: 99.97 txn/s, latency: 1386.83 ms, (p50: 1300 ms, p70: 1400, p90: 1600 ms, p99: 2100 ms), latency samples: 1720
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.819, avg: 1.725", "ConsensusProposalToOrdered: max: 0.313, avg: 0.307", "ConsensusOrderedToCommit: max: 0.346, avg: 0.331", "ConsensusProposalToCommit: max: 0.654, avg: 0.638"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.25s no progress at version 21466 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.96s no progress at version 2110574 (avg 0.96s) [limit 16].
Test Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-forge-e2e-perf Run the e2e perf forge only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant