GeezerVison

Review requested:

• @nodejs/startup

The reason it's an ArrayBuffer is performance. Changing it to a JS->C++ call likely tanks performance all over the place because buffers are used so pervasively.

I think you can try running the buffer benchmarks (see this doc) and see if the patch makes a difference.

I started a benchmark CI with this PR: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1159/

In Electron, and by default in V8, we enable the V8 memory cage, which
disallows pointers to memory outside of the cage. The zero-fill toggle is
currently implemented as an external ArrayBuffer, which violates this policy
and thus causes a crash when the memory cage is enabled.

Doesn’t that mean that Electron effectively won’t work with any externally provided ArrayBuffer? That seems like a big breaking change, if not for Node.js, then for any addon users?

Doesn’t that mean that Electron effectively won’t work with any externally provided ArrayBuffer? That seems like a big breaking change, if not for Node.js, then for any addon users?

Yep, unfortunately it does mean that. It's a tradeoff we've decided to make; I'm writing up our reasoning for it currently, will link when ready.

As for interpreting those benchmark results, I'm not totally sure what I'm looking at, but it seems to indicate that this makes things slower?

There seems to be a significant performance drop. Here are the double digit ones with 3 *s:

                                                                              confidence improvement accuracy (*)    (**)   (***)
buffers/buffer-copy.js n=6000000 partial='false' bytes=32768                         ***    -12.74 %       ±5.38%  ±7.20%  ±9.46%
buffers/buffer-copy.js n=6000000 partial='true' bytes=32768                          ***    -32.55 %      ±11.28% ±15.18% ±20.13%
buffers/buffer-creation.js n=600000 len=10 type='slow-allocUnsafe'                   ***    -42.54 %      ±12.06% ±16.13% ±21.14%
buffers/buffer-write-string.js n=1000000 len=2048 args='' encoding='utf16le'         ***    -12.57 %       ±6.85%  ±9.16% ±12.01%

externally provided ArrayBuffer

How common is it in addons?

It's a tradeoff we've decided to make; I'm writing up our reasoning for it currently, will link when ready.

I wonder if there’s a programmatic way to inform addon authors of this; this has a pretty big effect, I assume.

Practically speaking, are you using Node’s ArrayBufferAllocator? If you don’t/if you stop using it, the problem solved by this particular PR should go away altogether, right?

I'm closing this as the performance implications here are larger than I anticipated. This will still need to change in order to be compatible with V8 sandboxed pointers, but I'll take a different approach in a new PR.

As for the questions about impacts of enabling this in Electron, I don't think this is the appropriate place to dig into those, so I won't respond here. There will be a document soon with more details :)

@nornagon are there any more details on the alternative approach that was taken? The blog post appears to indicate that the V8 Memory Cage should increase performance:

This feature has performance and security benefits

Are the performance benefits of the pointer compression greater than the performance degradations caused by the changes it required to enable?

@slapbox

are there any more details on the alternative approach that was taken?

#43594