-
Notifications
You must be signed in to change notification settings - Fork 0
perf improve #367
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
perf improve #367
Conversation
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.
The changes primarily focus on performance optimizations by replacing generic data processing functions with specialized, faster versions, and by caching expensive-to-create objects. However, some of these changes introduce safety risks by removing bounds checks. Additionally, there's a confusing caching implementation in the Rust code.
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.
Potential integer overflow in length check and unsafe decoders without length validation.
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.
- Potential out-of-bounds crash introduced by switching to Data.subdata(...) without bounds checks. 2) New decodeUInt* helpers rely on debug-only asserts, risking undefined behavior in release builds. 3) Rust ring_context caches a single value ignoring the size parameter, potentially returning params for the wrong size. 4) Possible unit regression in maxLookupAnchorAge configuration.
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.
The changes are mostly performance improvements, replacing generic data access with more specific, faster methods, and adding caching. However, a new caching mechanism in ring_context introduces a risky assumption that could lead to future bugs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #367 +/- ##
==========================================
+ Coverage 81.93% 81.96% +0.03%
==========================================
Files 381 381
Lines 33707 33758 +51
==========================================
+ Hits 27618 27670 +52
+ Misses 6089 6088 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
https://github.com/open-web3-stack/boka/actions/runs/17850052202 I guess the benchmarks aren't exactly right? also for some quick tasks, we could run it for 10 or 100 times to make difference more noticeable. |
which ones are not right? I can see a few anomaly in erasure and shuffle and fallback, maybe those are too fast, and the result fuctuates, and they have too few samples. sarfole, storage and preiamge seems fine |
No description provided.