buffer: optimize Buffer.copy

[ronag]

This removes some of the overhead by extending the V8 api.

buffers/buffer-copy.js n=6000000 shared='false' partial='false' bytes=1024 ***  39.65 %   ±1.24%  ±1.66%  ±2.20%
buffers/buffer-copy.js n=6000000 shared='false' partial='false' bytes=128  ***  62.40 %   ±2.66%  ±3.55%  ±4.64%
buffers/buffer-copy.js n=6000000 shared='false' partial='false' bytes=8    ***  75.07 %   ±1.82%  ±2.42%  ±3.15%
buffers/buffer-copy.js n=6000000 shared='false' partial='true'  bytes=1024 *** 166.82 %   ±2.74%  ±3.69%  ±4.90%
buffers/buffer-copy.js n=6000000 shared='false' partial='true'  bytes=128  *** 224.72 %   ±2.15%  ±2.90%  ±3.83%
buffers/buffer-copy.js n=6000000 shared='false' partial='true'  bytes=8    *** 253.17 %   ±1.71%  ±2.29%  ±3.00%
buffers/buffer-copy.js n=6000000 shared='true'  partial='false' bytes=1024 *** 178.14 %  ±13.28% ±17.89% ±23.75%
buffers/buffer-copy.js n=6000000 shared='true'  partial='false' bytes=128  *** 105.29 %   ±1.83%  ±2.44%  ±3.18%
buffers/buffer-copy.js n=6000000 shared='true'  partial='false' bytes=8    ***  75.70 %   ±1.72%  ±2.29%  ±2.98%
buffers/buffer-copy.js n=6000000 shared='true'  partial='true'  bytes=1024 ***  91.38 %  ±10.05% ±13.54% ±17.98%
buffers/buffer-copy.js n=6000000 shared='true'  partial='true'  bytes=128  *** 230.69 %   ±2.03%  ±2.73%  ±3.62%
buffers/buffer-copy.js n=6000000 shared='true'  partial='true'  bytes=8    *** 251.20 %   ±1.84%  ±2.47%  ±3.24%

// PR
node$ out/Release/node benchmark/buffers/buffer-copy.js
buffers/buffer-copy.js n=6000000 shared="true" partial="true" bytes=8: 121,402,298.52590495
buffers/buffer-copy.js n=6000000 shared="false" partial="true" bytes=8: 150,598,474.5729912
buffers/buffer-copy.js n=6000000 shared="true" partial="false" bytes=8: 158,209,945.17735347
buffers/buffer-copy.js n=6000000 shared="false" partial="false" bytes=8: 146,067,437.3395647
buffers/buffer-copy.js n=6000000 shared="true" partial="true" bytes=128: 157,367,588.5504798
buffers/buffer-copy.js n=6000000 shared="false" partial="true" bytes=128: 146,945,070.7569905
buffers/buffer-copy.js n=6000000 shared="true" partial="false" bytes=128: 145,550,697.1271931
buffers/buffer-copy.js n=6000000 shared="false" partial="false" bytes=128: 137,749,082.19508398
buffers/buffer-copy.js n=6000000 shared="true" partial="true" bytes=1024: 86,024,692.29827811
buffers/buffer-copy.js n=6000000 shared="false" partial="true" bytes=1024: 82,854,617.76403005
buffers/buffer-copy.js n=6000000 shared="true" partial="false" bytes=1024: 65,355,531.58653537
buffers/buffer-copy.js n=6000000 shared="false" partial="false" bytes=1024: 57,637,071.98025907

// v25.6.1
node$ node benchmark/buffers/buffer-copy.js
buffers/buffer-copy.js n=6000000 shared="true" partial="true" bytes=8: 42,373,280.25128937
buffers/buffer-copy.js n=6000000 shared="false" partial="true" bytes=8: 44,768,824.757315286
buffers/buffer-copy.js n=6000000 shared="true" partial="false" bytes=8: 68,458,244.01386568
buffers/buffer-copy.js n=6000000 shared="false" partial="false" bytes=8: 86,251,974.36160062
buffers/buffer-copy.js n=6000000 shared="true" partial="true" bytes=128: 43,440,905.782851204
buffers/buffer-copy.js n=6000000 shared="false" partial="true" bytes=128: 46,963,775.47802432
buffers/buffer-copy.js n=6000000 shared="true" partial="false" bytes=128: 53,035,118.51192226
buffers/buffer-copy.js n=6000000 shared="false" partial="false" bytes=128: 85,299,162.71621366
buffers/buffer-copy.js n=6000000 shared="true" partial="true" bytes=1024: 33,336,852.223290235
buffers/buffer-copy.js n=6000000 shared="false" partial="true" bytes=1024: 40,153,531.31000715
buffers/buffer-copy.js n=6000000 shared="true" partial="false" bytes=1024: 16,149,801.988758216
buffers/buffer-copy.js n=6000000 shared="false" partial="false" bytes=1024: 49,143,503.67479149

[nodejs-github-bot]

Review requested:

• @nodejs/security-wg
• @nodejs/v8-update

[ronag]

@nodejs/performance

[mcollina]

lgtm

[github-actions]

Failed to start CI

   ⚠  Commits were pushed since the last approving review:
   ⚠  - buffer: optimize Buffer.copy
   ⚠  - Update v8-array-buffer.h
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/23705961064

[ChALkeR]

This is essentially a free unchecked memmove exposed to user world through Buffer API
Which can be used to freely read/write process memory

It should at least crash the process if proper error handling is impossible, not write/read arbitrary process mem

Without such check this, like #59985, is degrading whatever fragile protections e.g. --permission offers even further

Would look good with a check though, nice perf improvement!

[ronag]

The check is done in js. Doing checks in fast api context is difficult since as far as I'm aware you can't throw from there.

Please unblock.

[ronag]

Either way I don't see how this is worse than it was before. Feels like scope creep:

[ronag]

If you feel strongly about the check please open a pr and fix it properly across all the other methods as well. As you wrote in the issue. Messing around with prototypes is not supported.

[joyeecheung]

Can you upstream the V8 changes first? In general floating patches is reserved for toolchains/platforms V8 doesn't support, that adds maintenance burden to rebase the patches during V8 updates especially when V8 does any architectural changes (e.g. they are constantly changing how handles should be used internally, which can easily turns patches like this an upgrade blocker), so it's always better to merge them first in the upstream.

[ronag]

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1815

[ronag]

an you upstream the V8 changes first?

I don't think it's likely to land and I don't know how to upstream to V8. If that's a blocker for you then this PR is moot. I appreciate the maintenance burden and if those that have to deal with it object then I have no objection to closing this PR.

[joyeecheung]

I think this needs to be upstreamed first. I could catch the pointer issue from a glance. But I don't think I know enough to gauge the internal API uses are safe and along the lines of where V8 is going without review from people in the V8 team. Floating patches can both add maintenance burden to our V8 upgrades as well as V8's integration CI (they build Node.js with different configurations there e.g. with pointer compression and could get hit by a different set of problems). They are generally reserved for things that V8 defer to others (MSVC support or niche platform support - even in those cases we still try to upstream to minimize the burden, like the big endian patches), not things like features or performance optimizations.

[ronag]

Understood. Unfortunate. Was a significant perf boost.

[ronag]

Benchmark results:

buffers/buffer-copy.js n=6000000 shared='false' partial='false' bytes=1024 ***  39.65 %   ±1.24%  ±1.66%  ±2.20%
buffers/buffer-copy.js n=6000000 shared='false' partial='false' bytes=128  ***  62.40 %   ±2.66%  ±3.55%  ±4.64%
buffers/buffer-copy.js n=6000000 shared='false' partial='false' bytes=8    ***  75.07 %   ±1.82%  ±2.42%  ±3.15%
buffers/buffer-copy.js n=6000000 shared='false' partial='true'  bytes=1024 *** 166.82 %   ±2.74%  ±3.69%  ±4.90%
buffers/buffer-copy.js n=6000000 shared='false' partial='true'  bytes=128  *** 224.72 %   ±2.15%  ±2.90%  ±3.83%
buffers/buffer-copy.js n=6000000 shared='false' partial='true'  bytes=8    *** 253.17 %   ±1.71%  ±2.29%  ±3.00%
buffers/buffer-copy.js n=6000000 shared='true'  partial='false' bytes=1024 *** 178.14 %  ±13.28% ±17.89% ±23.75%
buffers/buffer-copy.js n=6000000 shared='true'  partial='false' bytes=128  *** 105.29 %   ±1.83%  ±2.44%  ±3.18%
buffers/buffer-copy.js n=6000000 shared='true'  partial='false' bytes=8    ***  75.70 %   ±1.72%  ±2.29%  ±2.98%
buffers/buffer-copy.js n=6000000 shared='true'  partial='true'  bytes=1024 ***  91.38 %  ±10.05% ±13.54% ±17.98%
buffers/buffer-copy.js n=6000000 shared='true'  partial='true'  bytes=128  *** 230.69 %   ±2.03%  ±2.73%  ±3.62%
buffers/buffer-copy.js n=6000000 shared='true'  partial='true'  bytes=8    *** 251.20 %   ±1.84%  ±2.47%  ±3.24%

[codecov]

Codecov Report

❌ Patch coverage is 58.33333% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.70%. Comparing base (cc96741) to head (7da38b1).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_buffer.cc	50.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62491      +/-   ##
==========================================
- Coverage   91.55%   89.70%   -1.85%     
==========================================
  Files         351      692     +341     
  Lines      147653   213981   +66328     
  Branches    23224    41050   +17826     
==========================================
+ Hits       135179   191948   +56769     
- Misses      12217    14110    +1893     
- Partials      257     7923    +7666     

Files with missing lines	Coverage Δ
lib/buffer.js	99.16% <100.00%> (+12.66%)	⬆️
src/node_buffer.cc	68.07% <50.00%> (ø)

... and 461 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jasnell]

I like it, but yeah, I agree with @joyeecheung ... an attempt to upstream this into v8 should be made first.

[ChALkeR]

@ronag

The check is done in js.

It isn't robust (as I mentioned above).

Doing checks in fast api context is difficult since as far as I'm aware you can't throw from there.

You can crash the process (again, as mentioned), which is better than reading/writing arbitrary process memory.

[ronag]

I've funded work to upstream this to V8. Hopefully this will be able to land in the future without blocking nits.