Closed ghost closed 4 years ago
Merging #33 into master will increase coverage by
0.07%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #33 +/- ##
==========================================
+ Coverage 86.99% 87.07% +0.07%
==========================================
Files 29 29
Lines 723 727 +4
==========================================
+ Hits 629 633 +4
Misses 94 94
Impacted Files | Coverage Δ | |
---|---|---|
lib/util.js | 97.56% <100%> (+0.12%) |
:arrow_up: |
api/put_changes.js | 96% <0%> (+0.16%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 2705845...2466f4e. Read the comment docs.
I was curious if using Node's built-in parseInt
and toString()
functions would be any faster:
function another_hex2dec (hex) {
return parseInt(hex, 16).toString(10)
}
I benched it against your new algorithm and the old one:
old_hex2dec: 15906ms
new_hex2dec: 1632ms
another_hex2dec: 1467ms
It's a small difference, but I like the higher readability.
Any thoughts on merging this @noffle ?
@okdistribute I think using parseInt(hex, 16).toString(10)
would be faster, according to my benchmark above.
@noffle okay cool, if you think this is critical perhaps we could update the code to do that or merge as is -- since both of these are an order of magnitude better than the old hex2dec.
6bbec31c9213436aaf770b47091d9b9e83b8217d
As part of some ongoing performance improvements we identified hex2dec as a piece of code that gets called a lot and could be improved. This doesn't move the needle much on total time for bulk imports, but this patch should improve hex2dec perf about 4-fold from 58ms to 14ms: