Skip to content
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

verkle: Add tests for verkle bytes helper #3441

Merged
merged 3 commits into from
Jun 10, 2024
Merged

Conversation

scorbajio
Copy link
Contributor

This change helps increase test coverage of the verkle package bytes helper by adding some unit tests.

@holgerd77
Copy link
Member

I wonder if this method is rather something for our Util package or is even already present there in some form? 🤔

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked in the bytes part of util and don't see anything exactly like it. Given that this is a pretty niche use case (specifically for parsing verkle stems), I'm not sure if it makes sense to move it or not. I'm not sure if I can conceive of a use for it outside of verkle (at least not in Ethereum land where our util library might be imported) so would probably be happy to keep it in verkle.

@scorbajio
Copy link
Contributor Author

I was also able to optimize it a bit:

Current Approach x 630 ops/sec ±1.08% (93 runs sampled)
Optimized Approach x 1,059 ops/sec ±4.31% (93 runs sampled)
Fastest is Optimized Approach

Benchmark file: bench.ts.tar.gz

@holgerd77
Copy link
Member

@acolytec3 sure, then let's keep! 👍

const minLength = Math.min(bytes1.length, bytes2.length)

for (let i = 0; i < minLength; i++) {
// Unroll the loop for better performance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you do performance testing to show that this is faster? It's not obvious to me why this is any faster than iterating element by element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the benchmark file is in the above comment: #3441 (comment). Since multiple bytes are being compared at a time, the loop runs less times and handles the remaining comparisons outside of the first loop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, didn't see this. Thanks for the comparison!

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@acolytec3 acolytec3 merged commit 3f9be25 into master Jun 10, 2024
33 of 34 checks passed
@scorbajio scorbajio deleted the verkle-bytes-helper-tests branch June 10, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants