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-5578): improve objectId deserialisation by skipping buffer allocation #615

Closed

Conversation

billouboq
Copy link
Contributor

@billouboq billouboq commented Aug 24, 2023

Description

Improve ObjectId deserialisation by around 20% by skipping one buffer allocation.
Also add a benchmark that deserialise lot's of ObjectIds

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken nbbeeken changed the title Improve objectId deserialisation fix(NODE-5578): improve objectId deserialisation by skipping buffer allocation Aug 24, 2023
@W-A-James
Copy link
Contributor

Hi @billouboq, thanks so much for this PR. Taking a look in the debugger, with this change, the ObjectId will not own the Uint8Array that is passed in. Because of this the buffer could be changed later on and mutate the value of the ObjectId, potentially leading to data corruption. So while this does improve performance, it breaks an underlying assumption we try to maintain with our BSON types.

@W-A-James W-A-James closed this Aug 29, 2023
@billouboq
Copy link
Contributor Author

Yeah no problem @W-A-James , I can understand. Thanks for the heads up 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants