test: refactor BTreeMap benches to improve coverage and readability #285
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
|
maksymar
changed the title
maksymar
changed the title
dsarlis
reviewed
Apr 28, 2025
There was a problem hiding this comment.
This is a big change that includes a few things. I wonder if we should try to at least separate the refactoring of benchmarks from adding the script (I understand the script helps but it's not strictly necessary for the refactoring of benchmarks).
Re the refactoring: I wonder if we should be more aggressive with what set of benchmarks we want to keep. The V1 version can be argued that it's the old version of BTreeMap that ideally people shouldn't use anymore (of course we need to keep it for backward compatibility but I think as long as it works correctly, we don't necessarily need to make sure it's performant anymore).
The V2 version should cover all needs (from unbounded to bounded types, through the use of the right Bound type). So, maybe we can live without V1 benchmarks and that should help reduce the number of them by a lot.
Re the script: adding a dedicated python script that someone needs to run to get some readable form of benchmarks sounds like we need to fix how benchmarks are reported in the first place. canbench could be updated to report benchmarks in a more concise way, e.g. we only report improved/regressed but not list the ones that are within noise (it can already know what the diff is, so perhaps we should simply be more aggressive in what is presented in the end). Most of the time people would want to see the improvements/regressions anyway and we can always have some option that prints the full output if desired.
The benefit I see is that we wouldn't need to maintain a custom script (which has some maintenance overhead) and I would also like to avoid having to run some script that transforms the data in CSV so I can import them somewhere else to be able to read them. I believe that canbench should be self-contained and presenting the benchmarks in a way that makes the most sense, not requiring people to perform post-processing so that they can actually analyze their benchmarks' results.
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 PR refactors BTreeMap benchmarks to increase coverage and improve readability.
Current benchmark setup has some challenges:
New benchmark setup:
FixedVecfor v2)Still adding new benchmarks must be handled with care because pocket_ic has a limitation on the number of tests (10k) and the total length of test names (20k chars). This limit is going to be increased soon.
Getting closer to this limit increases running time up to 7 minutes and increases significantly the size of canbench report, which is currently is not very readable.
Therefore this PR also adds a
./benchmark/scripts/with some scripts for extracting reports into CSV-format.