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

refactor(perf): expose single latency measurement #4105

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Jun 23, 2023

Description

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.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

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 only.

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?
connect(&mut swarm, server_address.clone()).await?;
let start = Instant::now();

let (server_peer_id, _) = connect(&mut swarm, server_address.clone()).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not also refactor connect to not run the timers anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 done in ec8f513.

@mergify mergify bot merged commit 73dbde1 into libp2p:master Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants