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

Add minimal, multi stream support to minimal-download client #38

Merged
merged 15 commits into from
Jan 12, 2024

Conversation

stephen-soltesz
Copy link
Contributor

@stephen-soltesz stephen-soltesz commented Jan 11, 2024

Previously, the minimal-download client only operated on a single stream. To contrast this tool with other multi stream tools, this change adds a minimal support for multi stream downloads.

The final download rate is currently calculated from the "first-open to first-close" time window.

For example:

$ minimal-download -duration 1s -streams 3
Connecting: wss://msak-mlab3-lga04.mlab-oti.measurement-lab.org/throughput/v1/download?...
Download client #1 - Avg  437.05 Mbps, MinRTT  4.55ms, elapsed 0.4319s, application r/w: 0/23595261
Download client #1 - Avg  484.26 Mbps, MinRTT  4.45ms, elapsed 0.8316s, application r/w: 0/50340258
Download client #1 - Avg  483.72 Mbps, MinRTT  4.45ms, elapsed 0.9366s, application r/w: 0/56634811
Download client #1 - Avg  491.40 Mbps, MinRTT  4.45ms, elapsed 1.0415s, application r/w: 0/63975876
Download client #1 - Avg  486.51 Mbps, MinRTT  4.45ms, elapsed 1.2245s, application r/w: 0/74464752
------
Download client #1 - Avg  490.41 Mbps, MinRTT  4.45ms, elapsed 1.1292s, application r/w: 0/69219792
Download client #1 - Peak 500.53 Mbps, MinRTT  4.45ms, elapsed 1.1057s, application r/w: 0/69178832

This change is Reviewable

@coveralls
Copy link

coveralls commented Jan 11, 2024

Pull Request Test Coverage Report for Build 7496086894

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 70.216%

Totals Coverage Status
Change from base Build 7480605258: 0.2%
Covered Lines: 877
Relevant Lines: 1249

💛 - Coveralls

This change consolidates websocket logic into the download() method so
that connection start and shutdown can happen concurrently across
multiple streams. As such, we checkpoint firstStart firstClose and
lastStart and lastClose times as well as byte counts at significant
events. With these variables, we can calculate various avg rates or a
peak rates.
@stephen-soltesz stephen-soltesz marked this pull request as ready for review January 11, 2024 23:22
@stephen-soltesz
Copy link
Contributor Author

@robertodauria what do you think?

Copy link
Contributor

@robertodauria robertodauria left a comment

Choose a reason for hiding this comment

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

:lgtm:. "Peak" results are just marginally (but consistently) better for me - e.g. increase of 0.1 Mb/s over 60 Mb/s

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)


cmd/minimal-download/main.go line 314 at r2 (raw file):

				case streamCount > 1 && stream == 0:
					// Only do this for one stream.
					s.mu.Lock()

Is locking needed here? s.firstStartTime is only written once before this loop starts. Are there cases where this access can lead to a race?

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz 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!

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @robertodauria)


cmd/minimal-download/main.go line 314 at r2 (raw file):

Previously, robertodauria (Roberto D'Auria) wrote…

Is locking needed here? s.firstStartTime is only written once before this loop starts. Are there cases where this access can lead to a race?

I think you're right. I think I was imagining the contention around lastStartTime. But you're right we're guaranteed that someone (maybe this thread) has already written this value and it won't be written again.

@stephen-soltesz stephen-soltesz merged commit b0d0b68 into main Jan 12, 2024
8 checks passed
@stephen-soltesz stephen-soltesz deleted the sandbox-soltesz-multistream branch January 12, 2024 00:23
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.

3 participants