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-6552): remove cache and use toStringTag in type helpers #740

Merged
merged 7 commits into from
Nov 27, 2024

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Nov 21, 2024

This partially reverts commit 61537f5.

Description

What is changing?

  • revert "Improve serialization performance"
Is there new documentation needed for these changes?

No

What is the motivation for this change?

  • This commit appears to be the source of failure observed in the driver performance test: "ldjsonMultiFileUpload"

Release Highlight

Fix issue with the internal unbounded type cache

As an optimization, a previous performance improvement stored the type information of seen objects to avoid recalculating type information. This caused an issue in the driver under extreme load and high memory usage as the cache grew. The assumption was that garbage collection would clear it enough to sustain normal operation. The cache is now removed and other optimal type checking is used in its place.

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

addaleax
addaleax previously approved these changes Nov 22, 2024
src/parser/utils.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken changed the title fix(NODE-6552): revert "Improve serialization performance" fix(NODE-6552): remove cache and use toStringTag in type helpers Nov 22, 2024
src/parser/utils.ts Outdated Show resolved Hide resolved
src/parser/utils.ts Outdated Show resolved Hide resolved
src/parser/utils.ts Outdated Show resolved Hide resolved
@W-A-James W-A-James self-assigned this Nov 26, 2024
@W-A-James W-A-James self-requested a review November 26, 2024 18:48
@W-A-James W-A-James added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Nov 26, 2024
W-A-James
W-A-James previously approved these changes Nov 26, 2024
@W-A-James W-A-James added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Nov 26, 2024
addaleax
addaleax previously approved these changes Nov 26, 2024
@nbbeeken nbbeeken dismissed stale reviews from addaleax and W-A-James via fd2e7dd November 26, 2024 20:29
@W-A-James W-A-James merged commit 3ede13e into main Nov 27, 2024
8 checks passed
@W-A-James W-A-James deleted the NODE-6552-fix-perf-test branch November 27, 2024 17:33
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
Development

Successfully merging this pull request may close these issues.

5 participants