fix: faster hex to byte implementation #6596
Conversation
Signed-off-by: Marin Petrunic <[email protected]>
mpetrunic
requested review from
avkos,
jdevcs,
luu-alex and
Muhammad-Altabba
as code owners
Bundle StatsHey there, this message comes from a github action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Deploying with
|
| Latest commit: |
e3e188a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5decc4d6.web3-js-docs.pages.dev |
| Branch Preview URL: | https://fix-optimize-hex-2-bytes.web3-js-docs.pages.dev |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 4.x #6596 +/- ##
==========================================
+ Coverage 87.37% 89.56% +2.18%
==========================================
Files 197 215 +18
Lines 7548 8288 +740
Branches 2059 2245 +186
==========================================
+ Hits 6595 7423 +828
+ Misses 953 865 -88
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Thanks @mpetrunic for your contribution.
I have just small suggestions.
Co-authored-by: Muhammad Altabba <[email protected]>
|
Any chance there should be a comment in the code here that this was adapted from Viem? |
@jxom Sure, I can open PR to add that note. But tbh, I wasn't sure where it originated from. I thought it came from https://github.com/paulmillr/noble-hashes/blob/main/src/utils.ts#L50 |
|
Ah true! Paul made the PR to Viem for this. Didn’t realise it was also in hashes! Might make a reference note for this in Viem repo then. |
#6646 :) |
| const charCodeMap = { | ||
| zero: 48, | ||
| nine: 57, | ||
| A: 65, | ||
| F: 70, | ||
| a: 97, | ||
| f: 102, | ||
| } as const | ||
|
|
||
| function charCodeToBase16(char: number) { | ||
| if (char >= charCodeMap.zero && char <= charCodeMap.nine) | ||
| return char - charCodeMap.zero | ||
| if (char >= charCodeMap.A && char <= charCodeMap.F) | ||
| return char - (charCodeMap.A - 10) | ||
| if (char >= charCodeMap.a && char <= charCodeMap.f) | ||
| return char - (charCodeMap.a - 10) | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
@mpetrunic Is this function not equivalent to the native parseInt with difference take it take integer parameter while parseInt take string parameter as charCodeToBase16(97) is same as parseInt('a', 16).
The native parseInt in that use case is almost 5 times more faster.
There was a problem hiding this comment.
Well yes, but for parseInt, I need to substring 2 characters which is expensive (memory allocation and all).
Funny enough, benching on runkit yields totally different results than locally:
Description
Noticed this implementation in couple of crypto repos, apparently this implementation is 6x faster.
Type of change
Checklist:
npm run lintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:coverageand my test cases cover all the lines and branches of the added code.npm run buildand testeddist/web3.min.jsin a browser.CHANGELOG.mdfile in the root folder.