refactor: encapsulate pooling helpers and state modification in tests

[nbbeeken]

Description

What is changing?

• Move the various variables that support the pooling to an object for code organization
• Use before/after hooks to ensure state is reset even when tests fail
• Move pool and offset to WeakMap style private variables

Is there new documentation needed for these changes?

No

What is the motivation for this change?

See mongodb#707

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

[SeanReece]

@nbbeeken I've rebased SeanReece:buffer-pool against main. I think you may also have to rebase/merge main here and we can merge this in 👍

[nbbeeken]

Rebased! @SeanReece should be all set here for your review. Working on getting the perf metrics we discussed on the other PR today 🎉 🤞🏻

[nbbeeken]

test	measurement	main	ObjectId pool 1000	Percent Change
singleBench	megabytes_per_second	18.83159911	16.7602144	-11.640%
multiBench	megabytes_per_second	160.8840654	148.3919854	-8.078%
parallelBench	megabytes_per_second	331.1638301	275.2237984	-18.450%
readBench	megabytes_per_second	321.7084202	274.2944344	-15.911%
writeBench	megabytes_per_second	139.2066326	125.3788738	-10.452%
driverBench	megabytes_per_second	230.4575264	199.8366541	-14.233%
ldjsonMultiFileUpload	megabytes_per_second	43.81457333	39.25755288	-10.971%
ldjsonMultiFileExport	megabytes_per_second	29.0683101	24.99832947	-15.055%
gridfsMultiFileUpload	megabytes_per_second	426.6074013	372.1347064	-13.640%
gridfsMultiFileDownload	megabytes_per_second	825.1650355	664.504605	-21.570%
returnDocument	megabytes_per_second	8187.165751	7616.915429	-7.216%
ping	megabytes_per_second	0.1129724661	0.1117610689	-1.078%
runCommand	megabytes_per_second	0.0827125336	0.0822814054	-0.523%
findOne	megabytes_per_second	4.993603288	5.046535575	1.054%
smallDocInsertOne	megabytes_per_second	1.203603759	1.173723034	-2.514%
largeDocInsertOne	megabytes_per_second	50.29759027	44.0603846	-13.220%
findManyAndEmptyCursor	megabytes_per_second	46.10949174	51.28812546	10.634%
smallDocBulkInsert	megabytes_per_second	32.91579311	28.33842022	-14.945%
largeDocBulkInsert	megabytes_per_second	32.66216984	30.68003353	-6.259%
gridFsUpload	megabytes_per_second	386.9452967	362.0072963	-6.659%
gridFsDownload	megabytes_per_second	703.2056602	625.6345767	-11.675%
findManyAndToArray	megabytes_per_second	42.89678305	44.38073259	3.401%
aggregateAMillionDocumentsAndToArray	megabytes_per_second	9.389312926	9.353244208	-0.385%
aggregateAMillionTweetsAndToArray	megabytes_per_second	32.94801597	35.45345392	7.326%

Maybe the WeakMap cost is too high for something as "main path" as every OID (as opposed to just the hex cache case we recently merged)

I'm going to switch back to TS private with a default of 1 and we can measure the perf of that and changing it to 1000

[nbbeeken]

I moved a bit quickly in the last post, "returnDocument" is a no-op test that we're supposed to normalize against to remove variation in the runtime's speed. In this screenshot, the percent differences are calculated by normalizing against the "returnDocument" result at the top of each column.

Pooling is certainly a win but I think we need to hold off on the WeakMap method of making it private and enabled by default in this major version. Our next major is not in a distant future so we may be able to apply this change with real private properties sooner than later.

[SeanReece]

@nbbeeken Sorry for the delay, things got busy around the holidays :)

I'm seeing similar performance regressions in my local benchmarks as well. I'm ok with us waiting for the next major version so we can use real private properties. Any idea when we can start working on v7? 😄

[nbbeeken]

Same here with the holidays, hope it was a good one! 🙂 thanks for taking a look over it. You can see my colleague's comment on the ticket here we concur it would be best to hold off for v7.

I think I will pull out the src changes from this PR and we can at least take in the test before/after changes so the pool size state change is always reset even when a test fails.

I hope to get back to you soon on a timeline for v7 (I'm also looking forward to what we can adopt with a runtime bump!)