multithreading benchmarking write-up #2207
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2207 +/- ##
=======================================
Coverage 94.65% 94.65%
=======================================
Files 102 102
Lines 23917 23917
=======================================
Hits 22638 22638
Misses 1279 1279 ☔ View full report in Codecov by Sentry. |
cpu
reviewed
Nov 12, 2024
f122b4f to
c19d1c1
Compare
cpu
approved these changes
Nov 14, 2024
1e4f979 to
377f09f
Compare
|
Looks like the charts didn't make it into your PR? (At least they don't show up in the Rendered for me.) |
377f09f to
09040e9
Compare
Fixed |
|
I think the log-scale x axis made things quite a bit better! For the right-hand key numbers in those distribution charts you currently show N, min, mean, stddev, 99.9% and max. I think percentiles are generally a better (and the commonly accepted) way of thinking about latencies (which don't usually really follow a normal distribution), so I'd (a) skip N -- which is the same for all implementations anyway and (b) report P5, P50, P75 and P99 (or something like that). Might also be nice to see if you can better align the numbers vertically so that they're easier to vertically compare. Also, still curious about OpenSSL numbers. All in all, rustls perf is awesome, great work! |
alex
reviewed
Nov 21, 2024
86a01bc to
eb64230
Compare
|
I've addressed comments thus far; tomorrow I will regenerate the data at specific versions. |
|
Oh, should this measure against 3.4.0, since its been out for about a month? |
7cdee57 to
a71c04d
Compare
a71c04d to
ba4d8f5
Compare
djc
approved these changes
Nov 29, 2024
There was a problem hiding this comment.
I'd still recommend getting rid of min, max (I think these are less relevant to the audience than P5/P99.9) and probably stddev. I'd probably also get rid of P75 and P99 (or P99.9) since these don't seem to add much information over the P50/P99.9 latencies that are already reported.
deleted min, max, stddev, P75 and P99.9. added P90. that leaves P5, P50, P90, P99. (this also avoids the formatting looking odd when where are fewer than 4 rows per table, heh) |
ba4d8f5 to
2a1d4dc
Compare
2a1d4dc to
88b8e83
Compare
alex
reviewed
Nov 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rendered