buffer: optimize Buffer.of with bufferPool #60372
Conversation
|
Review requested:
|
nodejs-github-bot
added
buffer
gurgunday
added
the
performance
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60372 +/- ##
==========================================
- Coverage 88.58% 88.56% -0.02%
==========================================
Files 704 704
Lines 207858 207871 +13
Branches 40054 40052 -2
==========================================
- Hits 184131 184105 -26
- Misses 15760 15807 +47
+ Partials 7967 7959 -8
🚀 New features to boost your workflow:
|
github-actions
bot
removed
the
request-ci
gurgunday
added
the
author ready
|
Benchmark GHA: https://github.com/aduh95/node/actions/runs/18809554526 Results (improvements across the board) |
|
@aduh95 the CI didn't pick up the benchmarks, I think the category is |
|
I feel like adding pooling to places which previously returned non-pooled buffers should be semver-major to be on the safe side Too much code in the ecosystem assuming non-pooled buffers and accessing |
|
Which could improve perf in some usecases significantly (~3x on allocations), and would be much more noticeable than But it would definitely be a semver-major And the only difference from |
|
Previously we have added pooling as semver minor (#31615), but I have no problem with semver major.
I think Buffer.alloc docs say it's a new buffer instance explicitly and it's one of the differences with unsafeAlloc, but maybe we still can? However it's less obvious. |
#31615 did not add pooling, see the diff there --
chalker@Nikitas-MacBook-Air % nvm use 4
Now using node v4.9.1 (npm v2.15.11)
chalker@Nikitas-MacBook-Air % node
> Buffer.from('x').buffer.byteLength
8192
> Buffer.from([1]).buffer.byteLength
8192 |
|
I'll let the collaborators decide – both are fine for me |
|
e.g. this will definitely break: https://github.com/website-local/website-scrap-engine/blob/6433593ee96fe2a386092984111a7c64599c9709/test/util.spec.ts#L80 but other places might use it indirectly, I have seen a lot of code assuming non-pooled uint8arrays, including in popular deps I vote semver-major 🙃 |
H4ad
added
the
semver-major
|
The commit message should also be changed to be more explicit, it's adding the buffer pool to @mcollina Do you think this change should be a topic for tsc due to the impact it might have be? |
Done |
|
Did you find something in Node.js code that would break? Or are you saying it just could? As you can see in the docs and the code most Buffer methods may already use it. I understand it's semver major and I'm OK with that, but for me one of the reasons to use Buffer methods to create a new Uint8Array is the buffer pool. Otherwise I'd just use Uint8Array static methods, etc. But yeah good idea to update the docs including .of as well. |
|
@gurgunday the first doc citation in my previous comment starts with
I trusted that means that it would break, but I did not recheck. |
|
Docs are added I think this can land as semver major This also gives people a good reason to use |
|
More data (not against landing this, just more data): 0 is main, 1 is this branch rebased on top of main, and last one is still non-pooled const of = (...items) => {
- const newObj = createUnsafeBuffer(items.length);
- for (let k = 0; k < items.length; k++)
+ const len = items.length;
+ const newObj = len <= 64 ? new FastBuffer(len) : createUnsafeBuffer(len);
+ for (let k = 0; k < len; k++)
newObj[k] = items[k];
return newObj;
};note: I increased n 10x for better consistency and added more small sizes (as that's on what Buffer.of is likely used) pooling improves the perf over that, but that patch is backportable and non semver-major more than half of the perf issue here was that Also note: it's important to not optimize |
|
Hm, I wonder if just always From top to bottom: const of = (...items) => {
- const newObj = createUnsafeBuffer(items.length);
- for (let k = 0; k < items.length; k++)
+ const len = items.length;
+ const newObj = new FastBuffer(len);
+ for (let k = 0; k < len; k++)
newObj[k] = items[k];
Perhaps it would make sense to land that in 24/25, and this in 26 on top. |
|
Is createUnsafeBuffer zero-filling in your benchmarks? |
|
@gurgunday no, this is rebased on main (also, I suggest rebasing this PR on main to get the non-zero-filling change in) |
|
I checked usage. |
|
@gurgunday I filed #60503. I also modified your benchmark to denoise it a little, but I didn't commit it to not conflict with this PR in benchmarks. |
6 participants
Continues the work of utilizing the bufferPool
After:
Before: