-
Notifications
You must be signed in to change notification settings - Fork 777
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
Conversation
I wonder if this method is rather something for our Util package or is even already present there in some form? 🤔 |
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.
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
.
I was also able to optimize it a bit:
Benchmark file: bench.ts.tar.gz |
@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 |
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.
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.
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.
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.
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.
Ah, sorry, didn't see this. Thanks for the comparison!
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.
LGTM!
This change helps increase test coverage of the
verkle
package bytes helper by adding some unit tests.