-
Notifications
You must be signed in to change notification settings - Fork 253
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
fix(NODE-3630): remove float parser and test edge cases for Double #502
Conversation
f931f12
to
fb76f7d
Compare
Serialization - Perf Improvement:
Summary: By replacing the old custom float parser logic with DataView allocation, NaN with payloads are now preserved in serialization-deserialization round-trips. When dv is allocated in When dv is allocated as a global variable (how it is now), serialization is actually faster than before DataView Implementation. |
Deserialization - Perf Slight Decline:
^^ Deserialization with DV Allocation does show some slower performance when compared to previous versions. Since there must be a new allocation every time, this not resolvable with a global variable.
^^ With the deserialization of an array of many doubles, if we change the allocation to only happen once in the function, rather than once for every double, the performance does not change significantly. |
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.
Just a bit of extra clean up on the tests - thanks a lot for making the other changes, much easier to read now!
|
||
function serializeThenDeserialize(value) { | ||
const serializedDouble = BSON.serialize({ d: value }); | ||
const deserializedDouble = BSON.deserialize(serializedDouble, { promoteValues: false }); |
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.
what's the significance of promoteValues: false
here? what happens with promoteValues: true
?
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.
This is so we get the raw BSON instead of the value as a double; this lets us check for payload in the BSON buffer
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.
could you add a comment above this line explaining that? also @nbbeeken is there a more precise option we can use than the general promoteValues?
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.
Unfortunately Doubles do not have a dedicated promote option, promoteValues: false is the only way to get the deserializer to return instanceof Double. chaos
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.
I resolved the addressed comments, but see my replies on two others
Description
What is changing?
Is there new documentation needed for these changes?
No
What is the motivation for this change?
The old custom float parsing (in float_parser.ts) did not support NaN with payload preservation, while native JS does.
Double check the following
npm run lint
script<type>(NODE-xxxx)<!>: <description>