test: improve BTreeMap bench coverage and readability #285
+3,485
−1,199
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
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the BTreeMap benchmarks by introducing support for unbounded types via a new FixedVec, removing all legacy V1 benchmarks, and updating the main benchmark harness for coverage and readability improvements.
- Add a
FixedVectype withStorableimplementation to cover unbounded-size tests. - Remove the generated
canbench_results.csvfile to avoid stale benchmark data. - Update
benchmarks/src/main.rsto importFixedVecand provide aRandomimplementation for it.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/storable.rs | Introduced FixedVec, implemented Storable |
| canbench_results.csv | Deleted outdated benchmark results |
| benchmarks/src/main.rs | Imported FixedVec and added Random impl |
Comments suppressed due to low confidence (3)
src/storable.rs:201
- [nitpick] The method name
frommay be confusing and doesn't implement the standardFromtrait. Rename it tonewor implementFrom<&[u8]> for FixedVec<N>for consistency with Rust conventions.
pub fn from(slice: &[u8]) -> Self {
src/storable.rs:192
- [nitpick] Consider expanding the doc comment to explain that
FixedVecis always padded to lengthN, and document the behavior ofmax_sizeand the constructor.
/// Byte-vector for testing size N; otherwise just a Vec<u8>.
src/storable.rs:210
- There are no unit tests for
FixedVec. Adding tests to verify padding,max_size, serialization, and deserialization behavior would improve confidence in its correctness.
impl<const N: usize> Storable for FixedVec<N> {
|
@dsarlis I am reviving this PR again, removed V1 benchmarks and synced to the new canbench reporting format. PTAL |
maksymar
changed the title
dsarlis
approved these changes
May 19, 2025
There was a problem hiding this comment.
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.
From the PR description, is this still true? I suppose not because the new version of canbench should have lifted this restriction.
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.
Current setup supports:
CSV report in google sheets.