-
Notifications
You must be signed in to change notification settings - Fork 280
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
Introduce perf
spec
#478
Introduce perf
spec
#478
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal looks good to me. Thanks for the write-up!
Is there any value in merging this early? If not, I would suggest holding off until we have a first implementation.
perf/perf.md
Outdated
2. Read from the stream until we get an `EOF` (client's write side was closed). | ||
3. Send the number of bytes defined in step 1 back to the client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be a benefit of sending things back at the same time at least as an option rather than waiting til the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would opening two streams in parallel be enough to satisfy this use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe full-duplex throughput within a single stream is interesting as well? This could be an optional flag in the client's request.
The referenced draft does ask the question whether it should be possible to send the traffic from the server before FIN:
Note - Should the server wait for FIN before replying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get most of the value with using two streams. The full-duplex is interesting, but I'd hold off from doing that until we have a compelling use case since it adds a bit of complexity to support that option.
Wrote a draft implementation of this specification on top of rust-libp2p. See libp2p/rust-libp2p#3508.
|
perf: clarify that upload and download are run sequentially
Implementation of the libp2p perf protocol according to libp2p/specs#478. //CC @MarcoPolo as the author of the specification. **Don't (yet) expect this to produce reliable performance metrics.** Pull-Request: #3508.
libp2p/rust-libp2p#3508 just merged in case you were holding off merging here till there is at least one implementation @MarcoPolo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of stylistic notes. Take however you wish :)
|
||
# Protocol | ||
|
||
The `/perf/1.0.0` protocol (from here on referred to as simply `perf`) is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we wanted to stop the use of numbers in the protocols in their first version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this documented somewhere? I don't particularly care one way or the other, but we already have a couple implementations out and this would be a bit annoying of a change with little to gain so I would opt for not making the change.
- [Security Considerations](#security-considerations) | ||
- [Prior Art](#prior-art) | ||
|
||
# Context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Context | |
## Context |
Nit: Multiple L1 headings in this document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that a problem? This is a top level section. If we made this ##
then it would be a child of ToC, which is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see the argument now. This could all be a child of the # Perf
part. I think that's a good argument, but the counter argument is that then you would always lose one nesting level in this style since you'll always have that title heading.
Unless we have some specific templating system we are trying to make happy, I don't think it's necessary to only have a single #
level. This whole doc is about perf
, so it should be okay to have multiple #
levels.
Co-authored-by: Thomas Eizinger <[email protected]>
Merging this since we have 4 implementations out already (Rust, Go, JS, and Zig). I'll leave it as 1A for now since all of these are prototypes and we haven't merged anything yet (with the exception of the Rust impl). We can update that in a future PR. |
A simple protocol to run client-driven benchmarks inspired by https://datatracker.ietf.org/doc/html/draft-banks-quic-performance.