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

feat(perf): add (automation, provision, build, run) tooling #184

Merged
merged 146 commits into from
Jun 22, 2023
Merged

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented May 26, 2023

Description

libp2p performance benchmarking

This project includes the following components:

  • terraform/: a Terraform scripts to provision infrastructure
  • impl/: implementations of the libp2p perf protocol 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.

See perf/README.md and overarching tracking issue #63.

Continuation of #163.

Outstanding work in this pull request:

  • Remove libp2p specific concepts (e.g. multiaddr) e.g. to no longer require the https example to parse a multiaddr.
  • Rename public ssh key
  • Remove --n-times flag from binaries. Potentially introduce --n-parallel-requests flag in the future. Multiple sequential runs are now done in the runner implementation.
  • Transfer and run binaries instead of docker images / containers. Or build on the machines.
  • Configurable Server port. See feat(perf): add (provision, build, run) tooling  #163.
  • Consider adding benchmark parameters to output json file. See feat(perf): add (provision, build, run) tooling  #163 (review).
  • Move to TLS only (for now). Easier to compare with HTTPs and no additional security protocol negotiation.
  • Remove the need for static peer IDs, i.e. remove --secret-key-seed.

### Outstanding work for future pull requests:
Outstanding / follow-up work tracked in #63.

@mxinden
Copy link
Member Author

mxinden commented Jun 21, 2023

We should probably run iperf on TCP and on UDP. It's far from guaranteed that we're getting the same result here.

Good point. I don't think we need to block this pull request on it. Added to the list of follow-up items. Let me know what you think.

Where's that list? I think this should be resolved rather sooner than later (let's say within the next 2 weeks). Don't care if it's in this PR, as long as it makes it in soon.

See pull request description @marten-seemann.

Missed that. Would probably make sense to move to a new issue, so it doesn't get lost when this PR is merged.

I will consolidate the outstanding follow-ups once this is merged on #63.

@mxinden
Copy link
Member Author

mxinden commented Jun 21, 2023

The biggest concern from me (which we can handle in a future PR) is to only use a single seconds measurement vs trying to separate this out between download and upload.

Sounds good to me. Added to the top of follow-ups to this pull request.


@MarcoPolo thank you for the review! I addressed all your comments. Would you mind taking another look? Anything else missing?

Comment on lines 65 to 69
type customReader struct {
downloadBytes uint64
uploadBytes uint64
position uint64
}
Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

if c.position < 8 {
binary.BigEndian.PutUint64(p, c.downloadBytes)
c.position += 8
return 8, nil
Copy link
Contributor

@MarcoPolo MarcoPolo Jun 21, 2023

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
}

Copy link
Member Author

@mxinden mxinden Jun 22, 2023

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.

@MarcoPolo
Copy link
Contributor

Looks good mod last comment about the reader.

Copy link
Contributor

@marten-seemann marten-seemann left a 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

@mxinden mxinden merged commit 53ff8b5 into master Jun 22, 2023
mxinden added a commit that referenced this pull request Jun 23, 2023
mxinden added a commit that referenced this pull request Jun 23, 2023
mxinden added a commit to mxinden/perf that referenced this pull request Jun 23, 2023
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.
marten-seemann pushed a commit to quic-go/perf that referenced this pull request Jun 23, 2023
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.
mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Jun 26, 2023
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 <mail@max-inden.de>
@mxinden mxinden deleted the perf branch June 26, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants