fix: reuse Buffer instance #7016
Merged
Conversation
This file contains 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
Performance Report✔️ no performance regression detected Full benchmark results
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #7016 +/- ##
============================================
+ Coverage 49.23% 49.24% +0.01%
============================================
Files 578 578
Lines 37426 37438 +12
Branches 2165 2172 +7
============================================
+ Hits 18426 18437 +11
- Misses 18960 18961 +1
Partials 40 40 |
|
this is on the test mainnet node, 
forkchoice |
twoeths
commented
Aug 13, 2024
packages/utils/package.json
Outdated
| @@ -27,7 +27,7 @@ | |||
| "build:watch": "yarn run build --watch", | |||
| "build:release": "yarn clean && yarn build", | |||
| "check-build": "node -e \"(async function() { await import('./lib/index.js') })()\"", | |||
| "check-types": "tsc && vitest --run --typecheck --dir test/types/", | |||
There was a problem hiding this comment.
@nflaig somehow this vitest command scans through the new test/perf folder and it caused error, even after I added --exclude option. Note that the benchmarks go with mocha
I guess it's still ok to drop this check because we also scan through unit test
There was a problem hiding this comment.
We need to run type checks separately because of --typecheck flag and those are not checked as part of unit tests. This seems to be an issue on vitest side, it shouldn't even scan files in this folder, I tried all possible options from here but none worked other than setting ignoreSourceErrors=true which I think is ok here and only affect typecheck tests.
nflaig
reviewed
Aug 13, 2024
There was a problem hiding this comment.
There are few more places we could use toRootHex should those be updated as well?
Eg.
lodestar/packages/beacon-node/src/api/impl/beacon/blocks/index.ts
Lines 100 to 102 in 64fe1db
| // Shared buffer to convert root to hex | ||
| const rootBuf = Buffer.alloc(32); | ||
|
|
||
| export function toRootHex(root: Uint8Array): string { |
There was a problem hiding this comment.
what happens if you pass in a root > 32 bytes? I would assume it just cuts of the end but we could consider throwing a more explicit error if this function is misused
wemeetagain
reviewed
Aug 13, 2024
wemeetagain
approved these changes
Aug 13, 2024
|
it was confirmed in docker deployment (unstable mainnet node) 
for feat4 mainnet node and all other nodes like in unstable-64 
|
This was referenced Aug 14, 2024
|
the unstable mainnet node 
also it works well with beta mainnet node and other nodes |
philknows
pushed a commit
that referenced
this pull request
Sep 3, 2024
* fix: improve message id to string conversion * fix: use shared Buffers for sszBytes util * feat: implement toRootHex() * fix: lint and check-types * Fix type checks * Add comment to vitest config * Update packages/utils/src/bytes.ts --------- Co-authored-by: Nico Flaig <[email protected]> Co-authored-by: Cayman <[email protected]>
|
🎉 This PR is included in v1.22.0 🎉 |
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.
Motivation
Buffer.from()and then convert to hex/base64. We can just reuse same Buffer in that case that would help thegca lotDescription
msgIdToStrFn()function by using the same BuffertoRootHex()function using the same Buffer instance to compute RootHex from a RootsszBytesutil, for example to extract AttData base64gc