perf(NODE-6246): Use buffer pool for ObjectId to significantly improve memory usage

[SeanReece]

Description

This PR significantly improves memory usage for ObjectId by introducing a buffer/Uint8Array pool. This is similar to how NodeJS Buffer uses an internal pool when using allocUnsafe, but the difference here is instead of instantiating a Buffer/Uint8Array for each ObjectId, each ObjectId holds an offset to it's pool.

Why?

Buffer (or TypedArray) is inefficient at storing small amount of data Buffer of size 12 takes 96 bytes in v8! An ObjectId with buffer is 128 bytes A pointer and a Number take 16 bytes. An ObjectId using pool is 48 bytes + pool

1000 ObjectId using pool = 60,048 bytes 1000 ObjectIds without pool = 128,000 bytes

~53% reduction in memory usage

Lots more discussion in https://github.com/mongodb/js-bson/pull/703

Memory improvements

Using MFlix sample database. 1 Million documents

Before: 655MB Now: 369MB

~44% reduction

Performance tests

Since we're still making use of Buffer/Uint8Array the performance is mostly the same. There is a slight regression creating from Buffer (since we are copying instead of just saving the reference), but an improvement when creating from Uint8Array (since we do not need to convert to Buffer).

Performance ideas

Now that ObjectId allocates its own Buffer/Uint8Array, we technically never need to call toLocalBufferType since we know we're always operating on the correct buffer type. Removing that check is outside the scope of this change but would increase performance.

Notes

• the pool is lazily initialized so that services that don't use ObjectId do not incur a penalty here
• Setting ObjectId.poolSize = 1 essentially disables the pool (Each ObjectId will have its own buffer)
• I tried to keep the API as consistent as possible, including exposing buffer property which returns a Buffer/Uint8Array.

Is there new documentation needed for these changes?

Adding ObjectId.poolSize to allow users to set their preferred ObjectId pool size. This references the # of ObjectIds that will be stored in each pool, the actual pool allocated with be 12 * poolSize. This defaults to 1000 as that seemed reasonable.

What is the motivation for this change?

Release Highlight

Improve memory usage by pooling ObjectId buffers

@SeanReece brought to our attention the fact that Uint8Array requires a surprising amount of overhead. The 12 bytes of ObjectId data actually takes up 96 bytes of memory. You can find an excellent breakdown of what all that overhead is here.

In this release, @SeanReece has significantly improved the memory used for each ObjectId by introducing a Uint8Array pool of memory. The idea is similar to how Node.js' Buffer uses an internal pool when using allocUnsafe or from APIs that overwrite the returned memory. Each ObjectId uses the current shared pool and stores an offset to where its bytes begin, all operations internal to the class use this offset so there is no impact to the public API.

Crunching the numbers

The default ObjectId.poolSize is 1000. Since it is measured in ObjectIds this means it has a default size of 12,000 bytes plus the same overhead that each ObjectId was incurring.

• Before pool:
  • 1 ObjectId uses 128 B
  • 1000 ObjectIds use 128 KB
  • 1,000,000 ObjectIds use: 128 MB
• After pool:
  • 1 ObjectId uses 40 B + ~12,000 B
  • 1000 ObjectIds use 40 KB + ~12 KB
  • 1,000,000 ObjectIds use: 40 MB + ~12.08 MB

As you can see the new cost for a single ObjectId is higher than before but as your data set grows the shared pools lead to huge savings in memory. If the initial cost is too high or your data sets are even larger the pool's size is configurable!

ObjectId.poolSize

The shared pool is not created until the first ObjectId is constructed so you can modify the poolSize to fit your needs:

import { ObjectId } from 'bson';
// or import { ObjectId } from 'mongodb';
ObjectId.poolSize = 1;

[!TIP] Setting the poolSize to 1 essentially disables this change.

[!TIP] You can change the poolSize at any time during your program, the next time the current pool's size runs out a new one will be created using the current poolSize value.

Thank you so much to @SeanReece for a very thorough and informative bug report and for working so hard on this improvement!

A note about deep equality

ObjectIds will no longer respond to naive deep equality checks by default. APIs like util.deepStrictEqual and lodash.isEqual ignore what is public or private API and make assumptions about what properties are considered a part of two values' equality.

Workarounds

Set ObjectId.poolSize = 1; disabling the optimization so now each ObjectId will have its own buffer.

Use objectid.equals(otherOid) wherever possible. lodash.isEqualWith allows you to define a customizer that can be used to call the ObjectId's equals method.

Use BSON.serialize if applicable. If you pass the two objects you wish to compare to the BSON serializer, the bytes returned will be exactly equal if the documents are the same on the server.

Double check the following

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

[SeanReece]

@nbbeeken I've made some more updates with your suggestions.

• Removed Symbol properties. Now we just have private properties pool and offset
• Add getPool() and incrementPool() functions.
• Add setter for poolSize to enforce valid value.
• Fix for buffer offset checks.
• Fix all tests + added more coverage.

As for deep equality with ObjectIds, what I've done is added a test function assertDeepEqualsWithObjectId which you can pass any 2 objects/arrays into and it will first deeply find and convert all ObjectIds into their hex string representations, then it'll perform the chai deep equality assertion.

Let me know what you think 😄 🚀

[SeanReece]

@nbbeeken Thanks for bearing with me on this marathon 😄 As always, I appreciate the thorough reviews. Hope we're looking good now 🚀

[nbbeeken]

LGTM! I will have the team take a final look over then I think we're good to go!

[nbbeeken]

Hey @SeanReece, just wanted to update we haven't forgotten about this, we are considering a couple of ways around the deep equality changes and weighing the performance implications of each while still working on our planned work so we appreciate you bearing with us.

If it is of any interest we're looking at making the pool/offset non-enumerable, javascript # private, potentially setting the poolSize to 1 by default to make the change opt-in rather than opt-out, or making ObjectId subclass Uint8Array. Will keep you posted! 🙂👍🏻

[SeanReece]

@nbbeeken Thanks for the heads up! Just an FYI I noticed a significant hit to memory usage and performance when I tried using private (#) properties for pool/offset. It looks like that's caused by us targeting ES2021 (since # was added in ES2022) and tslib rewriting all private properties accessors to a getter/setter function which uses a WeakMap. If that's the way we want to go then we might also want to look at bumping our target to ES2022.

FWIW I think defaulting poolSize to 1 is a good choice. Still allows users to configure based on use case, but leaves the deep equality by default. 😃

[nbbeeken]

Thanks for looking into that, we were considering real # private properties but that has the potential to impact users of bundlers who will downlevel the code to the WeakMap implementation anyway. Lots to consider as maintainers!

poolSize=1 is also showing some regressions in performance from current on main, which is surprising since I would've expected it to generally be the same as before. I need to rerun and look closer into why that is. If you've still got your benchmarking setup and can check as well let me know if you're seeing the same. TIA 🙂

[SeanReece]

@nbbeeken I believe the performance impact is from some extra operations required for the pool implementation, namely having to copy the buffer into the pool when creating ObjectId from buffer, then when retrieving the ObjectId id, we need to call subArray on the pool. I think we can improve performance in the poolSize=1 case by making some changes.

If poolSize=1

1. Do not store offset (this saves us some memory)
2. When creating from buffer (size=12), just persist the buffer
3. When retrieving the ObjectId buffer, return entire buffer

Let me run some benchmarks and see what we can do.

[nbbeeken]

When you get a chance can you rebase this on main so we can get the same benchmark fixes here? TIA :)

[SeanReece]

@nbbeeken Updated 👍 I've been able to get the benchmarks running locally in docker, but haven't had the time to do proper comparison of the results yet. Let me know what the results you're seeing in the benchmarks :)

Edit: I can now run bson-bench natively after rebasing on main. Seeing the regressions on my end - think I've got a good fix, just doing some cleanup so I should be able to commit soon.

In the meantime @nbbeeken what do you think about adding a valueOf method to ObjectId that returns the hex string or the 12 byte buffer. This works in our jest tests with pool > 1, but I believe chai deep equality would still fail.

[nbbeeken]

Hey @SeanReece sorry about the radio silence (it has been a busy week). Thanks for continuing to get the benchmarks running on your end, those results do corroborate what I am seeing; I also still saw a regression in our objectid performance tests but it is smaller than before so we're on the right track.

I wasn't aware of how valueOf() affects the results of deep eq 🤔 am I understanding correctly that jest invokes that if it exists? I think chai not working is a blocker here, we have vast amount of tests that would need to be repaired if we don't make sure that we can continue to deep equality check after this is all said and done.

[nbbeeken]

Just want to share the current concerns from the team here

This is a great contribution, and we are super interested in incorporating it. However, our team has a fully planned quarter ahead, so we won't be able to conduct a thorough review or make final design decisions until next quarter (begins Nov).

The PR is currently held up on the following points:

• Ensuring default performance remains optimal for users who upgrade without making changes.
• Guaranteeing deep equality is maintained by default without breaking existing functionality. We're hoping poolSize=1 gets us this without costing us point 1.
• Encapsulation of code, for which the team needs time to design a more maintainable approach, but we’d welcome your input.
• Minor improvements to testing. Mainly ensuring we have reset global state (poolSize) in before/after hooks

We intend to tackle these issues from our end and don’t expect you to continue work on this if you no longer feel inclined to unless the feedback inspires specific ideas you’d like to propose/implement.

We appreciate your patience and continued work on this.

cc @dariakp @baileympearson @W-A-James @durran @aditi-khare-mongoDB @addaleax

[nbbeeken]

Hey @SeanReece, can you rebase/merge (either is fine) main when you get the chance? I have some changes proposed in https://github.com/SeanReece/js-bson/pull/1

[SeanReece]

@nbbeeken Awesome! Does this mean we can enable the pool by default? :) Or we still want it to be opt-in?

[nbbeeken]

Great question! Let me pull in poolSize=1000 to the driver's benchmarks to get a holistic picture of the perf results

[billouboq]

What's the blocking point of this PR ?

[SeanReece]

@billouboq We just need to bump the tsconfig target to es2022 in order to support private class members. My understanding is that we need to wait for the next major release (v7) in order to make that change.

We want to use private class members to store the extra variables needed for the buffer for 2 main reasons:

• To ensure deep equality checks continue to work (with this change, 2 "equal" OIDs would have different offsets)
• To hide the implementation details

Without native support for private class members, tslib has to shim private members and there is a memory and performance regression.

[KJ1126007]

@SeanReece we are waiting for this :)

[SeanReece]

@nbbeeken Has there been any movement on v7? Even if we had a v7 branch we could work against in the meantime would be great.

[dariakp]

@SeanReece Thanks for reaching out!

To answer your question, yes, we have V7 planned for the upcoming quarter. Unfortunately, the team is currently operating at reduced capacity and we have a number of high priority commitments we have to deliver, but we expect to be able to shift focus to V7 related work in September.

We do not currently have a branch for it (and once we do start making the breaking changes, they will happen on main); however, you are welcome to update your PR to make use of the JS private properties along with any supporting es target changes if you'd like to keep this work moving.

We really appreciate your patience and continued engagement with the project - thank you for helping us improve this library!

[baileympearson]

Hey @SeanReece - just a heads up that we're wrapping up v7 work. main contains all the changes we plan to make and this work should be unblocked

[billouboq]

Really hope it will make it in v7 😢, performance and memory are very important in Node JS

[baileympearson]

@SeanReece Okay, so, an update on where we're at. Thanks for your patience on this issue!

Two weeks ago we released bson v7. Notably, private properties did not make it into the release.

We originally introduced private properties into the ObjectId in https://github.com/mongodb/js-bson/pull/830. After some internal testing, we realized that this has the effect of breaking lodash's deepClone() function and we decided that this breakage was too large (we discussed with the maintainer of Mongoose, who indicated that many Mongoose users could be broken by this change). We reverted the PR for v7.

Where does this leave us for this PR? We're unfortunately between a rock and a hard place: using private properties breaks lodash's deepClone, not using private properties breaks lodash's deepEquals. Both of these we consider supported usages.

We've decided on the following:

1. First, we'd like to investigate the performance implications of storing the ObjectId's data as a latin1 string instead of a buffer. This would improve memory usage of ObjectIds, but we'd need to confirm this doesn't negatively impact the performance of ObjectId creation (a critical codepath in BSON).
2. If storing a string doesn't work, we'd consider alternatives (such as allowing for different ObjectId storage representations depending on a user's desired performance characteristics or something like this).

If this is something you are still interested in working on, we're happy to work with you on this (although the direction is admittedly a bit vague). But we don't have the capacity to commit much more to this right now. We will continue working on this in the future though because we do believe this is an important feature.