-
Notifications
You must be signed in to change notification settings - Fork 199
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
Improve performance of answerGetBlockBodiesQuery #2198
Conversation
Avoid decoding and re-encoding of the block body by using available RLP-encoded block body from the database, combine it with the RLP-encoded block hash to avoid unnecessary processing and extra memory allocations.
Coverage from tests in coverage: 50.0% of statements across all listed packagescoverage: 63.2% of statements in consensus/istanbul coverage: 42.7% of statements in consensus/istanbul/announce coverage: 55.8% of statements in consensus/istanbul/backend coverage: 0.0% of statements in consensus/istanbul/backend/backendtest coverage: 24.3% of statements in consensus/istanbul/backend/internal/replica coverage: 62.1% of statements in consensus/istanbul/core coverage: 50.0% of statements in consensus/istanbul/db coverage: 0.0% of statements in consensus/istanbul/proxy coverage: 64.2% of statements in consensus/istanbul/uptime coverage: 51.8% of statements in consensus/istanbul/validator coverage: 79.2% of statements in consensus/istanbul/validator/random |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2198 +/- ##
==========================================
- Coverage 55.08% 46.58% -8.51%
==========================================
Files 681 229 -452
Lines 114413 29830 -84583
==========================================
- Hits 63028 13895 -49133
+ Misses 47496 15084 -32412
+ Partials 3889 851 -3038 ☔ View full report in Codecov by Sentry. |
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.
Looks good, maybe it's worthwhile to add the benchmarking code as well?
// Combine takes two RLP-encoded values one and two and produces a combined one | ||
// as if it was an RLP encoding of the [one, two] list | ||
// based on https://ethereum.org/en/developers/docs/data-structures-and-encoding/rlp/ | ||
func Combine(one []byte, two []byte) []byte { |
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.
Weird that there's no existing code for this. Could you add a simple test?
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.
Yeah, I was also surprised there is nothing like that. Added some.
Added. |
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.
Looks good, thanks for adding the test and benchmark!
Description
Avoid decoding and re-encoding of the block body by using available RLP-encoded block body from the database, combine it with the RLP-encoded block hash to avoid unnecessary processing and extra memory allocations.
Benchmarks:
3x more CPU-efficient and 1.5x more memory-efficient.
A larger change would be storing RLP-encoded blockBodyWithBlockHash structures instead of just block bodies but it would significantly increase the diff.
Other changes
No.
Tested
Existing tests pass, added unit tests and the benchmark.
Backwards compatibility
Yes.