Assorted Refactorings Part 2 #536
Merged
Conversation
This file contains 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
nickhudkins
changed the title
|
Now that we have a module for benchmark, one could write a test for this part and compare before & after the change. |
|
Hey! It looks like maybe the reason Oleg had indicated the risk was that he experienced performance sensitivity in this area already. Master:This Branch:Happy to let these benchmarks run longer, but after the first pass through (5 forks, 5 iteration warmup, 5 test runs), the results look promising that we're not introducing a performance regression by accepting this PR. @yanns Happy to leave it up to you if you would like me to run longer benchmarks before accepting this is A.O.K. @nightscape I actually wonder if your hunch about collection methods being more optimized may play out here in the long run! |
nickhudkins
force-pushed
the
perf-senstive-refactorings
branch
from
98e8e39
to
8d39237
Compare
|
From 669.221 ops/s to 255.573 ops/s is quite a performance degradation, right? |
|
Maybe I am reading it wrong, but my understanding of this is that it's 33743.582 vs 34008.015 ? Based on this: https://stackoverflow.com/questions/33604084/understanding-jmh-output, I believe this says, with 99.9% confidence, the first run can be expected to perform The second run: |
|
I'll go ahead and get a full run running in the cloud so I can let it sit |
MasterThis BranchThe StringUtilBenchmark difference (presumably a baseline) is pretty bizzare, and it looks like maybe these machines were under very different conditions. I am re-running again, since I think this would only be a fair comparison with our baseline closer together. The benchmarking SHALL CONTINUE! |
|
A quick comparison in excel, this is showing this branch's performance (advantage/disadvantage) in the far right column. Any difference larger than 2% has been indicated with a green arrow, anything 0-2% are yellow, and any thing where it did not perform as well has a red arrow.
|
yanns
approved these changes
Jan 8, 2021
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.
This is a continuation of #404, with items that Oleg had indicated as potentially risky due to performance implications as mentioned here: #404 (review)
I'd like to keep the changes around, so I have split this into a separate PR in hopes that we can come up with a plan to validate, or disprove Oleg's concerns.