perf(p2p): parallelize catalog peer lookups - #107
Open
catyans wants to merge 4 commits into
Open
Conversation
Contributor
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
Comment on lines
+832
to
+833
| first_started.notified().await; | ||
| second_finished.notified().await; |
Contributor
There was a problem hiding this comment.
[test · low]
Bound these synchronization waits (and the barrier waits in the following test) with tokio::time::timeout(TEST_TIMEOUT, ...). If a regression prevents a candidate from being polled or changes the initial concurrency window, the test currently deadlocks indefinitely rather than producing an actionable failure; this file already defines TEST_TIMEOUT for that purpose.
Suggestion:
Suggested change
| first_started.notified().await; | |
| second_finished.notified().await; | |
| tokio::time::timeout(TEST_TIMEOUT, async { | |
| first_started.notified().await; | |
| second_finished.notified().await; | |
| }) | |
| .await | |
| .expect("initial buffered lookups should complete"); |
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.
Summary
Why
lookup_peerscurrently waits for every candidate serially. With the default5-second per-peer timeout, stale peers at the front of the discovery result can
make lookup latency grow as
N * lookup_timeout.The new ordered buffer starts up to four lookups concurrently but consumes their
results in candidate order. This bounds connection fan-out and reduces timeout
accumulation without changing scheduler priority.
Lingjun validation
Validation was run on the four-node Lingjun cluster, not on the development
machine.
cargo fmt --all -- --checkp2p::iroh::transport::tests: 14 passedFour-node latency experiment
192.168.0.42)All endpoints were produced by real
IrohBlobsP2pTransportinstances. The firsttwo providers were then stopped before measurement; the third continued serving
the published artifact.
mainThe change reduced mean lookup latency by approximately 50.6%; all 10/10
lookups returned the descriptor from lingjun-102.
The branch was subsequently rebased onto v0.1.1
main;git range-diffconfirmed that all four patch commits remained identical.
Fixes #95