-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[benchmark] Janitor Duty: Sweep Quadratic #22673
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
[benchmark] Janitor Duty: Sweep Quadratic #22673
Conversation
This was obsoleted by DictionaryCopy.
Split the composite tests from `DictionatyCopy` and `DictionaryFilter` into individual benchmarks by dictionary size. Lowered the workloads to run faster (more stable results).
bfd3c74
to
8a19a98
Compare
@swift-ci please benchmark Running with 8a19a985c61cce4dd1964b522049b6204f0c41e4 ( |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
8a19a98
to
8e67642
Compare
@swift-ci benchmark |
@swift-ci please smoke test |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview
|
Change the test to work after removal of `HashQuadratic`.
@swift-ci please test Running full tests to verify the adjusted Benchmark_O tests pass after the removal of |
@eeckstein @lorentey Please review 🙏 |
This comment has been minimized.
This comment has been minimized.
@swift-ci smoke test linux platform |
@eeckstein I believe this is now mergeable: the full test that runs As for the new benchmarks, the first benchmark run with disabled |
I'll let @lorentey review this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code reorganization looks good to me! I don't much mind losing the old variants here -- although I guess they did have some value in release-to-release comparisons, I expect we have other benchmarks for insertion at least.
I have one question about the results, though: If I read the benchmark results correctly, the Quadratic Surprise doesn't show up in the filter benchmarks. I can see the bump in Dict.CopyKeyValue.20k
, but there is no corresponding increase for Dict.FilterAllMatch.20k
-- all the filter results seem to be roughly the same.
The implementation of filter will change at some point soon, but I'm wondering if 16k--28k is large enough to trigger the issue.
I have also noticed the filter has been much flatter in response, but the quadratic behavior is still fully visible in the |
Ah, that makes sense 👍 |
@lorentey Thank you! |
To enable robust performance measurements by minimizing the accumulated error, this splits the composite tests from
DictionatyCopy
andDictionaryFilter
into smaller individual benchmarks by dictionary size. These benchmarks guard against the quadratic behavior (SR-3268). Also removes the older disabledHashQuadratic
benchmark that was covering the same issue.