-
Notifications
You must be signed in to change notification settings - Fork 73
feat(perf): add (automation, provision, build, run) tooling #184
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
Conversation
See `perf/README.md`.
perf/impl/https/v0.1/main.go
Outdated
| type customReader struct { | ||
| downloadBytes uint64 | ||
| uploadBytes uint64 | ||
| position uint64 | ||
| } |
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.
Can we just copy-paste the perf implementation we have in the v0.27 folder? The Read implementation here looks quite complicated.
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.
The impl/go-libp2p/v0.27 implementation leverages the io.Writer interface on network.Stream interface. I don't know how to have the Go std http library provide an object that implements io.Writer interface for an HTTP request. Instead all the methods that I am aware of require passing an io.Reader for an outgoing HTTP request, i.e. client.Post and http.NewRequest.
@marten-seemann are you aware of an alternative?
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 left a comment below.
perf/impl/https/v0.1/main.go
Outdated
| if c.position < 8 { | ||
| binary.BigEndian.PutUint64(p, c.downloadBytes) | ||
| c.position += 8 | ||
| return 8, nil |
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.
This isn't correct. You're assuming the input slice is always at least 8 bytes.
Use io.MultiReader to combine a bytes.NewReader(<buf-with-u64-download-bytes>) and a zeroReader.
The zeroReader could look something like:
type zeroReader struct {
n int64
}
func (z *zeroReader) Read(p []byte) (int, error) {
if z.n <= 0 {
return 0, io.EOF
}
for i := range p {
if z.n <= 0 {
return i, nil
}
p[i] = 0
z.n--
}
return len(p), nil
}
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.
Wasn't aware of io.MultiReader. That simplifies things. Thanks!
Fixed with 2296d8c. Note that I am still using copy instead of zeroing every byte individually in the (maybe premature) hope that Go can optimize some of it. Let me know in case you prefer individual zeroing for each byte instead @MarcoPolo.
Edit:
Slight change of plan, instead of 2296d8c I merged @marten-seemann's suggestion through #201.
|
Looks good mod last comment about the reader. |
marten-seemann
left a comment
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.
LGTM, let's ship it!
There's a few follow-up items to this PR, but what we have here is an excellent basis to start iterating on.
Thank you for this clean setup @mxinden, I'm really happy how this turned out
Instead of exposing the time to establish a connection, the time to upload the bytes and the time to download the bytes, expose a single time including all three. The rational here is, that differentiation of the three is flawed. E.g. when does one stop the upload timer and start the download timer? When the last byte is sent? When the last byte is flushed? When the first byte is received? See libp2p/test-plans#184 (review) for past discussion.
Instead of exposing the time to establish a connection, the time to upload the bytes and the time to download the bytes, expose a single time including all three. The rational here is, that differentiation of the three is flawed. E.g. when does one stop the upload timer and start the download timer? When the last byte is sent? When the last byte is flushed? When the first byte is received? See libp2p/test-plans#184 (review) for past discussion.
Instead of exposing the time to establish a connection, the time to upload the bytes and the time to download the bytes, expose a single time including all three. The rational here is, that differentiation of the three is flawed. E.g. when does one stop the upload timer and start the download timer? When the last byte is sent? When the last byte is flushed? When the first byte is received? See libp2p/test-plans#184 (review) for past discussion. Pull-Request: #4105. Co-Authored-By: Max Inden <[email protected]>
This project includes the following components: - `terraform/`: a Terraform scripts to provision infrastructure - `impl/`: implementations of the [libp2p perf protocol](https://github.com/libp2p/specs/blob/master/perf/perf.md) running on top of e.g. go-libp2p, rust-libp2p or Go's std-library https stack - `runner/`: a set of scripts building and running the above implementations on the above infrastructure, reporting the results in `benchmark-results.json` Benchmark results can be visualized with https://observablehq.com/@mxinden-workspace/libp2p-performance-dashboard. Co-authored-by: Marco Munizaga <[email protected]> Co-authored-by: Marten Seemann <[email protected]> Co-authored-by: Piotr Galar <[email protected]>
Description
See
perf/README.mdand overarching tracking issue #63.Continuation of #163.
Outstanding work in this pull request:
--n-timesflag from binaries. Potentially introduce--n-parallel-requestsflag in the future. Multiple sequential runs are now done in the runner implementation.--secret-key-seed.### Outstanding work for future pull requests:Outstanding / follow-up work tracked in #63.