-
Notifications
You must be signed in to change notification settings - Fork 633
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(cbor): encodeCbor & encodeCborSequence #6311
base: main
Are you sure you want to change the base?
perf(cbor): encodeCbor & encodeCborSequence #6311
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6311 +/- ##
==========================================
+ Coverage 96.35% 96.36% +0.01%
==========================================
Files 547 547
Lines 41671 41693 +22
Branches 6315 6321 +6
==========================================
+ Hits 40151 40179 +28
+ Misses 1479 1473 -6
Partials 41 41 ☔ View full report in Codecov by Sentry. |
if (isNegative && input <= -(2 ** 64)) { | ||
throw new RangeError( | ||
`Cannot encode number: It (${input}) exceeds -(2 ** 64) - 1`, | ||
); | ||
} else if (input >= 2 ** 64) { | ||
throw new RangeError( | ||
`Cannot encode number: It (${input}) exceeds 2 ** 64 - 1`, | ||
); |
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.
IMO, these checks can lead to inaccurate results due to floating-point precision error
For example, 2 ** 64 - 10
is still equal to 2 ** 64
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.
While precision is lost with numbers larger than 2 ** 53, I've experienced they're still consistent in how the number is interpreted and removing them would just cause the error to be thrown later at DataView.setBigUInt64 instead and would be less clear on why they got an error.
I noticed an error in my benchmark code for objects and maps, so the above benchmark code and results has been updated to fix that. |
This pull request rewrites the
encodeCbor
andencodeCborSequence
functions to provide better performance via the unification of memory creation on the heap. Previously when it came to encoding arrays, objects and maps it required creating lots of little bits of memory for each value and then at the end creating additional memory to concatenate it all together. These changes take a new approach, instead with the trade off being double iteration of arrays, objects and maps to first calculate the size that the input would end up being to create one uniform piece of memory and then fill it linearly in on the second iteration.undefined
,null
, booleans, and dates are all doing better than previously.Benchmark Source Code
std/cbor/deno.json
std/bench.ts
Benchmark Results