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.

Sign up for GitHub

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

Jump to bottom

feat!: measure transfer perf over time #2067

Merged
merged 16 commits into from
Nov 7, 2023
Merged

feat!: measure transfer perf over time #2067

merged 16 commits into from
Nov 7, 2023

Conversation

maschad
Copy link
Member

@maschad maschad commented Sep 21, 2023 •
edited by achingbrain
Loading

Closes #2064

BREAKING CHANGE: measurePerformance now returns an async generator that yields PerfOutputs and no longer accepts the startTime parameter

@maschad maschad changed the title feat!(@libp2p/protocol-perf): Continously measure perf feat@libp2p/protocol-perf): Continous perf Sep 21, 2023
@maschad maschad changed the title feat@libp2p/protocol-perf): Continous perf feat@libp2p/protocol-perf): continous perf Sep 21, 2023
@maschad maschad changed the title feat@libp2p/protocol-perf): continous perf feat!(@libp2p/protocol-perf): continuous perf Sep 21, 2023
@maschad maschad changed the title feat!(@libp2p/protocol-perf): continuous perf feat(@libp2p/protocol-perf)!: continuous perf Sep 21, 2023
mxinden
mxinden reviewed Sep 22, 2023
View reviewed changes
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.

Thank you for tackling this!

packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
@mxinden mxinden mentioned this pull request Sep 25, 2023
Merged
@mxinden
Copy link
Member

mxinden commented Oct 19, 2023

Friendly ping @maschad. Anything blocking this pull request?

@maschad maschad changed the title feat(@libp2p/protocol-perf)!: continuous perf feat!: continuously measure on single connection Oct 24, 2023
@maschad maschad marked this pull request as ready for review October 24, 2023 21:24
@maschad maschad requested a review from a team as a code owner October 24, 2023 21:24
@maschad
Copy link
Member Author

maschad commented Oct 24, 2023

Thanks for following up @mxinden I made the changes suggested in the feedback and this is ready for review now.

@maschad maschad self-assigned this Oct 24, 2023
mxinden added a commit to libp2p/test-plans that referenced this pull request Oct 25, 2023
@mxinden
fix(perf): remove js-libp2p
a3d3ca6 
Once libp2p/js-libp2p#2067 is merged, we can
re-introduce it.
SgtPooki
SgtPooki approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

a few comments, but looks mostly good to me. How could we run this locally to confirm? Can we add that info to README.md ?

packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
achingbrain
achingbrain requested changes Oct 27, 2023
View reviewed changes
packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
packages/protocol-perf/src/index.ts Outdated Show resolved Hide resolved
@achingbrain
Copy link
Member

achingbrain commented Nov 7, 2023 •
edited
Loading

I've made some changes here:

  1. The measurePerformance method now returns an AsyncGenerator that yields results - the caller can console.log them or do something else
  2. The client now is the thing that yields results, previously the server was writing read results to stdout which is not what the README says it should do
  3. Write/read 64 bit values into the stream for bytesToSendBack - previously it was writing 32 bit values which limits the max value to 4GB
  4. uploadBytes/downloadBytes are now numbers in the output as per the README.
  5. I've removed yargs etc - it will be useful to test browser performance too, so any CLI usage needs to be external from this module
  6. measurePerformance accepts a Multiaddr, not a Connection - the README says the final reported time should include connection establishment - if it accepts a Connection then the connection has already been established

@achingbrain achingbrain mentioned this pull request Nov 7, 2023
Closed
@achingbrain achingbrain changed the title feat!: continuously measure on single connection feat!: measure transfer perf over time Nov 7, 2023
@achingbrain achingbrain merged commit 78db573 into libp2p:master Nov 7, 2023
22 checks passed
maschad added a commit to maschad/js-libp2p that referenced this pull request Nov 10, 2023
@maschad @achingbrain
feat!: measure transfer perf over time (libp2p#2067)
15fe483 
Measures upload/download speed separately and also over time rather than in total.

Closes libp2p#2064

BREAKING CHANGE:  `measurePerformance` now returns an async generator that yields `PerfOutput`s and no longer accepts the `startTime` parameter

---------

Co-authored-by: Alex Potsides <[email protected]>
This was referenced Jan 18, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SgtPooki SgtPooki SgtPooki approved these changes

@achingbrain achingbrain achingbrain approved these changes

@mxinden mxinden Awaiting requested review from mxinden

@wemeetagain wemeetagain Awaiting requested review from wemeetagain

Assignees

@maschad maschad

Labels
None yet
Projects
js-libp2p
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(@libp2p/perf): Continously measure Performance on a Single Connection
5 participants
@maschad @mxinden @achingbrain @SgtPooki @wemeetagain