src: simplify toggling the buffer zero-fill flag

[nornagon]

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. This PR changes the
zero-fill toggle to be implemented without the use of an external ArrayBuffer.

cc @bnoordhuis who wrote this code back in 27e84dd

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[bnoordhuis]

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.

[nornagon]

My intuition is that it shouldn’t be too much slower, do you have a suggestion about what would exercise this path correctly to expose the performance difference?

On Wed, Jun 22, 2022 at 12:57 Ben Noordhuis ***@***.***> wrote: 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. — Reply to this email directly, view it on GitHub <#43537 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABKGACPX2KKBDJ63C5IILLVQNV3HANCNFSM5ZRDR6CQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- j

[joyeecheung]

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

[joyeecheung]

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

[addaleax]

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?

[nornagon]

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?

[RaisinTen]

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%

[RaisinTen]

externally provided ArrayBuffer

How common is it in addons?

[addaleax]

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?

[nornagon]

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 :)

[Nantris]

@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?

[RaisinTen]

@slapbox

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

#43594