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(@libp2p/protocol-perf): Implement perf protocol #1604

Merged
merged 38 commits into from
Aug 11, 2023
Merged

Conversation

MarcoPolo
Copy link
Collaborator

Implements libp2p/specs#478

@MarcoPolo MarcoPolo changed the title Implement perf protocol feat: Implement perf protocol Mar 1, 2023
@MarcoPolo MarcoPolo force-pushed the marco/perf branch 2 times, most recently from ae5fbfd to b76cc80 Compare March 1, 2023 05:37
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

src/perf/index.ts Outdated Show resolved Hide resolved
@BigLep
Copy link
Contributor

BigLep commented Jun 3, 2023

What are the next steps on this? I'd like to make sure js-libp2p isn't getting left behind on the perf dashboarding.

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic and stylistic concerns lgtm

src/perf/index.ts Outdated Show resolved Hide resolved
src/perf/index.ts Outdated Show resolved Hide resolved
@maschad maschad self-assigned this Jun 5, 2023
@maschad
Copy link
Member

maschad commented Jun 5, 2023

What are the next steps on this? I'd like to make sure js-libp2p isn't getting left behind on the perf dashboarding.

Agreed @BigLep . I am going to refactor the PerfService to allow this to be configured the way configure our other optional services now following @achingbrain 's suggestion, as well as incorporate @wemeetagain 's feedback and then that should return this PR to a ready state.

@mxinden
Copy link
Member

mxinden commented Jun 6, 2023

Thank you @maschad.

and then that should return this PR to a ready state.

I will then integrate it into libp2p/test-plans#163.

@maschad maschad marked this pull request as draft June 9, 2023 20:41
@maschad maschad requested a review from achingbrain June 9, 2023 21:10
src/perf/index.ts Outdated Show resolved Hide resolved
@maschad maschad marked this pull request as ready for review June 9, 2023 21:30
@maschad maschad requested a review from wemeetagain June 9, 2023 21:30
@maschad
Copy link
Member

maschad commented Jun 9, 2023

CI check failure is fixed by #1801

@maschad
Copy link
Member

maschad commented Jun 9, 2023

Since this is our first benchmark tests there isn't really a precedent, but do we want to run these types of tests in our regular PR workflow or would it be better suited to be run on the main branch ? cc @achingbrain @wemeetagain @MarcoPolo

@mxinden
Copy link
Member

mxinden commented Jun 11, 2023

What are you referring to with "these types of tests"? For libp2p/test-plans#184 we plan to run it before every release for now.

@maschad
Copy link
Member

maschad commented Jun 11, 2023

What are you referring to with "these types of tests"? For libp2p/test-plans#184 we plan to run it before every release for now.

I was referring to benchmark tests there (as opposed to our other types of tests such as unit or integration tests). But if the plan is to run it before every release then the answer to my question would be to run these post-merge on the main branch.

@MarcoPolo
Copy link
Collaborator Author

In the ideal world, this should run on every PR to highlight any perf regressions or improvements. It should fail CI if perf degrades by more than X% relative to the main branch.

@p-shahi
Copy link
Member

p-shahi commented Jun 13, 2023

Can we pull in https://github.com/ipfs-shipyard/js-libp2p-transfer-performance into test/perf as a followup? This would allow us to close libp2p/test-plans#65

@p-shahi
Copy link
Member

p-shahi commented Jun 14, 2023

@achingbrain suggestion: add this as a package to the monorepo (in a folder called perf/ in packages/)

@maschad
Copy link
Member

maschad commented Jun 16, 2023

Currently blocked by a huge refactor in #1833

@maschad maschad marked this pull request as draft June 16, 2023 23:02
@achingbrain achingbrain force-pushed the master branch 2 times, most recently from ea8a063 to d853d12 Compare June 19, 2023 11:23
@maschad maschad marked this pull request as ready for review August 4, 2023 03:44
@maschad
Copy link
Member

maschad commented Aug 4, 2023

Update:

Running these perf tests locally i.e. running npm run start with a server and dialler will output a latency measurement to stdout.

I had spent a significant amount of time trying to run the perf tests using the terraform provisioned infrastructure but to no avail, it ended up being a lot of AWS wrangling which I think is tangential to the essence of this PR, more is outlined here

These tests will be ran on this workflow but given this PR outputs the measurement of latency to stdout as required by the test plans implementation setup this PR should be ready for review.

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking promising, comments inline.

packages/perf/src/renderResults.ts Outdated Show resolved Hide resolved
packages/perf/previousPerf.json Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
.github/workflows/perf-test.yml Outdated Show resolved Hide resolved
packages/perf/package.json Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/package.json Outdated Show resolved Hide resolved
packages/perf/src/main.ts Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can only comment. I'd request changes if I could.

I think we can get rid of some of the code here 😄

.github/workflows/perf-test.yml Outdated Show resolved Hide resolved
packages/perf/previousPerf.json Outdated Show resolved Hide resolved
packages/perf/src/constants.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/test/index.spec.ts Outdated Show resolved Hide resolved
packages/perf/src/renderResults.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
@maschad maschad requested a review from achingbrain August 5, 2023 19:08
@maschad
Copy link
Member

maschad commented Aug 5, 2023

I've made the requested changes and I've removed the perf workflow as discussed here for now, once libp2p/test-plans#244 is merged we can introduce a workflow to run on release / main in a follow up PRs.

@mxinden
Copy link
Member

mxinden commented Aug 7, 2023

@maschad please ping me once you need another review here.

packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
packages/perf/src/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would approve if I could (my own PR).

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Just a few changes before merge:

  1. Update the @packageDocumentation in index.ts to show examples for perf not circuit relay - this will appear on the API docs page for the package
  2. Rename the packages/perf folder to packages/protocol-perf - to be consistent with peer-discovery-mdns/peer-discovery-bootstrap, transport-tcp/transport-webrtc/transport-websockets/etc

@maschad maschad changed the title feat: Implement perf protocol feat(@libp2p/protocol-perf): Implement perf protocol Aug 10, 2023
@maschad maschad merged commit 3345f28 into master Aug 11, 2023
@maschad maschad deleted the marco/perf branch August 11, 2023 02:16
achingbrain pushed a commit that referenced this pull request Aug 11, 2023
Co-authored-by: Marco Munizaga <[email protected]>
Co-authored-by: Chad Nehemiah <[email protected]>
This was referenced Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants