node::Buffer::New is 4x slower than V8 APIs

[kvakil]

Version

v19.0.0-pre

Platform

Linux 19-Ubuntu SMP Wed Jun 22 17:44:56 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

buffer

What steps will reproduce the bug?

1. Clone this repo.
2. Run npm install.
3. Run node v8_buffers.js, which uses v8::ArrayBuffer::NewBackingStore + v8::ArrayBuffer::New.
4. Run node node_buffers.js, which uses node::Buffer::New.

How often does it reproduce? Is there a required condition?

always

What is the expected behavior?

Performance of node::Buffer::New should be comparable to the native V8 APIs.

What do you see instead?

It's around 4x slower:

$ node node_buffers.js 
1000000
nodeBuffers: 1.649s
$ node v8_buffers.js 
1000000
rawArrayBuffers: 432.881ms

It is even worse (~6x) if you use time to include the GC finalizer time:

$ \time node node_buffers.js 
1000000
nodeBuffers: 1.657s
2.99user 0.41system 0:02.68elapsed 127%CPU (0avgtext+0avgdata 648340maxresident)k
0inputs+0outputs (0major+332498minor)pagefaults 0swaps
$ \time node v8_buffers.js 
1000000
rawArrayBuffers: 430.123ms
0.54user 0.12system 0:00.60elapsed 111%CPU (0avgtext+0avgdata 308632maxresident)k
0inputs+0outputs (0major+71241minor)pagefaults 0swaps

Additional information

I know that node::Buffer::New does more than the V8 APIs, so of course it is slower. Especially a lot of the finalizer stuff seems to be pretty high overhead. But 4x feels quite a bit slower, and I suspect that there is some room for improvement.

Attached are two perf script profiles:

v8_buffers.gz
node_buffers.gz

[addaleax]

I would be reluctant to put a lot of effort into improving performance of a feature when there's a better, faster language built-in already available. We probably should and can deprecate the native Buffer API(s). 🙂

That being said, for the comparison to be a bit fairer, I think the V8 benchmark should create Uint8Arrays, not just ArrayBuffers, because that's the direct equivalent of Buffer.

[kvakil]

I would be reluctant to put a lot of effort into improving performance of a feature when there's a better, faster language built-in already available. We probably should and can deprecate the native Buffer API(s). slightly_smiling_face

Any thoughts about how a deprecation should interact with napi? There is napi_create_external_arraybuffer, but it just calls out to node::Buffer::New.

I looked at some Github search results, and I haven't actually been able to find anyone relying on the finalizer to call back into Javascript. I wonder if it would be possible to expose a different function like napi_create_external_arraybuffer except that it doesn't have the same finalization guarantees?

[addaleax]

I would be reluctant to put a lot of effort into improving performance of a feature when there's a better, faster language built-in already available. We probably should and can deprecate the native Buffer API(s). slightly_smiling_face

Any thoughts about how a deprecation should interact with napi?

I think deprecating or discouraging the use of napi_create_buffer[_copy]() would be fine. It’s not ever going to go away and it’s not exactly wrong to use the method, so maybe documenting that where Uint8Array suffices, people should use it, is the right thing to do.

There is napi_create_external_arraybuffer, but it just calls out to node::Buffer::New.

Yeah … not great. I introduced this in c1ee70e, with a commit message that hopefully explains the choice well enough.

I looked at some Github search results, and I haven't actually been able to find anyone relying on the finalizer to call back into Javascript.

I’m pretty sure we’ve had bug reports about this here in the Node.js repository. But either way, this can be a pretty subtle breaking change that can’t easily be put through a deprecation cycle.

I wonder if it would be possible to expose a different function like napi_create_external_arraybuffer except that it doesn't have the same finalization guarantees?

I think this would be a good idea anyway, yes 👍 One could call it napi_create_fast_external_arraybuffer or similar, people love things that sound fast (regardless of actual performance).

[legendecas]

For node-api finalizers to be able to calling into JavaScript or how can we improve the condition here by providing an "eager finalization" without the ability to evaluate JavaScript, this is the problem tracked at #44071. I believe the problem is not limited to the finalizer of external_array_buffer in node-api.

[kvakil]

I think deprecating or discouraging the use of napi_create_buffer[_copy]() would be fine. It’s not ever going to go away and it’s not exactly wrong to use the method, so maybe documenting that where Uint8Array suffices, people should use it, is the right thing to do.

I'll open a PR to add a note in the documentation.

I’m pretty sure we’ve had bug reports about this here in the Node.js repository. But either way, this can be a pretty subtle breaking change that can’t easily be put through a deprecation cycle.

Right, completely agreed that changing/deprecating the existing semantics is a non-starter. But I suspect that most people aren't using the delayed finalizer, and they would benefit from the eager finalizer.

As another data point, I believe both napi-rs and neon completely prevent the user from calling into Javascript from the finalizer via Rust's borrow checker, so addons built on top of those could be transparently transitioned over.

For node-api finalizers to be able to calling into JavaScript or how can we improve the condition here by providing an "eager finalization" without the ability to evaluate JavaScript, this is the problem tracked at #44071. I believe the problem is not limited to the finalizer of external_array_buffer in node-api.

Thanks for pointing this out. I guess that adding napi_create_fast_external_arraybuffer here is closest to "Adding a new type of finalizers which disallows JavaScript execution." in your document. If we do that, we would be able to provide specialized versions of the functions that are faster when the user requests "eager finalizers".

[jimmywarting]

no more than just me that think we should stop using Buffer: #41588
Buffer is not so cross env friendly and requires polyfill/dependencies
the browser version is eg 10x slower at Buffer.from(str) then using TextEncoder + uint8array #39301 (comment)

[Julusian]

On a related note, napi_create_external_buffer (and I guess by extension napi_create_external_arraybuffer) are no longer possible to use in Electron (source). Using them results in a crash.

So to me, deprecating and eventually removing these methods makes sense, as right now they are problematic to use if you want a cross runtime compatible native module