Skip to content
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

Merged
merged 12 commits into from Jul 6, 2022

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Jun 22, 2022

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

  • Ran npm run lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

src/parser/serializer.ts Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 22, 2022
@aditi-khare-mongoDB
Copy link
Contributor Author

aditi-khare-mongoDB commented Jun 28, 2022

Serialization - Perf Improvement:

- cpu: Apple M1
- cores: 8
- os: darwin
- ram: 16GB
- iterations: 1,000,000
testing: Double Serialization

BEFORE making DV ALLOCATION GLOBAL: local    - v 37c8edb - avg 0.00041684ms
AFTER making DV ALLOCATION GLOBAL:  local    - v 37c8edb - avg 0.00024913ms

released       - v 4.6.4                                 - avg 0.00036335ms
previous major - v 1.1.6                                 - avg 0.00036459ms
bson-ext       - v 4.0.2                                 - avg 0.00163558ms

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 serializeNumber() and serializeDouble() functions, serialization is slower than before DataView implementation.

When dv is allocated as a global variable (how it is now), serialization is actually faster than before DataView Implementation.

@aditi-khare-mongoDB
Copy link
Contributor Author

aditi-khare-mongoDB commented Jun 28, 2022

Deserialization - Perf Slight Decline:

(specs are same as previous comment)

testing: Double Deserialization
local             - v e45f7d0  - avg 0.00025402ms
released          - v 4.6.4    - avg 0.00020516ms
previous major    - v 1.1.6    - avg 0.00017792ms
bson-ext          - v 4.0.2    - avg 0.00036723ms

^^ 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.

testing: Many Doubles Deserialization
ALLOCATING ONCE                local          - v e45f7d0  - avg 0.00026782ms
ALLOCATING FOR EACH DOUBLE     local          - v e45f7d0  - avg 0.00026212ms
released       - v 4.6.4    - avg 0.00022623ms
previous major - v 1.1.6    - avg 0.00021576ms
bson-ext       - v 4.0.2    - avg 0.00040693ms

^^ 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.

etc/benchmarks/main.mjs Outdated Show resolved Hide resolved
@nbbeeken nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 28, 2022
nbbeeken
nbbeeken previously approved these changes Jun 29, 2022
test/node/bson_corpus.spec.test.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/tools/utils.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
Copy link
Contributor

@dariakp dariakp left a 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!

test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved

function serializeThenDeserialize(value) {
const serializedDouble = BSON.serialize({ d: value });
const deserializedDouble = BSON.deserialize(serializedDouble, { promoteValues: false });
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor

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

test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
test/node/double_tests.js Outdated Show resolved Hide resolved
Copy link
Contributor

@dariakp dariakp left a 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

@nbbeeken nbbeeken changed the title fix(NODE-3630): Delete float parser and add edge cases for Double fix(NODE-3630): remove float parser and test edge cases for Double Jul 6, 2022
@nbbeeken nbbeeken merged commit 54ca603 into main Jul 6, 2022
@nbbeeken nbbeeken deleted the NODE-3630-dlt-float-parser branch July 6, 2022 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
4 participants