Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.4k
lib: use slice to compact Readable buffer in streams#59676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Aug 29, 2025
Review requested:
|
codecovbot commented Aug 29, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #59676 +/- ## ======================================== Coverage 89.89% 89.89% ======================================== Files 667 667 Lines 196675 196852 +177 Branches 38612 38652 +40 ======================================== + Hits 176796 176966 +170 + Misses 12332 12312 -20 - Partials 7547 7574 +27
🚀 New features to boost your workflow:
|
daeyeon commented Sep 2, 2025
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1726/ Details |
ronag left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause a copy and an allocation. I don't see how this is an improvement. Slicing in JavaScript is not like in go, it will copy the elements, not just create a view.
wlgh1553 commented Sep 2, 2025
@ronag Thanks for the review! 😀 My primary intention was to avoid the O(N) element shifting cost incurred by This approach is also a proven optimization pattern used in PR #59406 to solve the same issue in I see the Jenkins CI benchmark results as data that empirically validates this hypothesis in a real-world scenario. |
ronag commented Sep 2, 2025
Why is it more efficient? |
ronag commented Sep 2, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Here is an illustration of what needs to happen in both cases: splice does the exact same thing as slice but without the extra allocation. Unless there is some V8 magic involved that I am unaware of. I think the benchmark results of both this and the other PR is within the margin of error. |
wlgh1553 commented Sep 2, 2025
Thank you for the detailed illustration with the code examples. It was very helpful for clarifying the logical operations. You are correct, and I agree that splice uses a smaller temporary allocation since slice must allocate for the larger, remaining portion of the array. I believe the 'V8 magic' we're discussing is the most likely explanation for the consistent benchmark improvements we see for slice in both the CI for this PR and the previous one (#59406). To isolate this performance difference more clearly, I ran a more targeted local microbenchmark with a larger array. "use strict";functionbench(label,fn){constt0=performance.now();fn();constt1=performance.now();console.log(`${label}:`.padEnd(25),`${(t1-t0).toFixed(2)} ms`);}functionmakeArray(size){constarr=newArray(size);for(leti=0;i<size;i++){arr[i]=i;}returnarr;}constARRAY_SIZE=20_000_000;constREMOVE_COUNT=100;constRUNS=100;bench("Array.prototype.splice()",()=>{for(leti=0;i<RUNS;i++){constarr=makeArray(ARRAY_SIZE);arr.splice(0,REMOVE_COUNT);}});bench("Array.prototype.slice()",()=>{constarr=makeArray(ARRAY_SIZE);for(leti=0;i<RUNS;i++){constnewArr=arr.slice(REMOVE_COUNT);}});The results are consistently and significantly in favor of Test Environment:
This data strongly supports the idea that the high CPU cost of shifting millions of elements for This is the core rationale for my proposal: I believe trading the one-time memory allocation for a significant gain in CPU performance is a worthwhile improvement for this hot-path in the streams implementation. |
ronag commented Sep 2, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I believe your benchmark is flawed. V8 is probably entirely optimizing away newArr and slice since it has no side effect. clk: ~3.43GHz cpu: AppleM3Pro runtime: node24.5.0(arm64-darwin)benchmarkavg(min…max)p75/p99(min…top1%)--------------------------------------------------------------------------Array.prototype.splice()42.12µs/iter36.21µs█(24.92µs…1.39ms)192.87µs█(14.66kb…285.54kb)157.71kb▂█▃▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁Array.prototype.slice()56.49µs/iter56.07µs███(48.54µs…73.75µs)71.57µs█▅▅██▅▅▅(77.85b…6.09kb)701.80b█████▁▁█▁▁▁▁▁▁▁▁▁█▁▁█import{bench,run}from'mitata'functionmakeArray(size){constarr=newArray(size);for(leti=0;i<size;i++){arr[i]=i;}returnarr;}constARRAY_SIZE=20_000;constREMOVE_COUNT=100;letcounter=0;bench("Array.prototype.splice()",()=>{constarr=makeArray(ARRAY_SIZE);counter+=arr.splice(0,REMOVE_COUNT).length;});bench("Array.prototype.slice()",()=>{constarr=makeArray(ARRAY_SIZE);counter+=arr.slice(REMOVE_COUNT).length;});awaitrun();console.log(counter); |
wlgh1553 commented Sep 3, 2025
Thank you for all the helpful feedback. You've helped me clear up my misunderstanding of the performance characteristics. Based on our discussion, I'll go ahead and close this PR. Thanks again! |
Replace
splice(0, idx)withstate.buffer = ArrayPrototypeSlice(state.buffer, idx)when compacting the Readable buffer.Rationale:
spliceBenchmark (local):