-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
buffer: avoid unnecessary copy in Buffer.concat for single FastBuffer #58266
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
buffer: avoid unnecessary copy in Buffer.concat for single FastBuffer #58266
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58266 +/- ##
=======================================
Coverage 90.18% 90.18%
=======================================
Files 629 629
Lines 186657 186640 -17
Branches 36658 36669 +11
=======================================
- Hits 168328 168324 -4
- Misses 11126 11128 +2
+ Partials 7203 7188 -15
🚀 New features to boost your workflow:
|
list[0] instanceof FastBuffer && | ||
list[0].length === length | ||
) { | ||
return Buffer.from(list[0]); |
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.
If list[0]
is a TypedArray (like another Buffer
) then this still copies.
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.
I’m wondering if it makes sense to return list[0] directly. I understand this avoids copying, so I guess I should update the test accordingly?
https://github.com/nodejs/node/blob/main/test/parallel/test-buffer-concat.js#L43
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.
Buffer.concat
should always return a new buffer object:
Lines 1033 to 1034 in 91d2400
Returns a new `Buffer` which is the result of concatenating all the `Buffer` | |
instances in the `list` together. |
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.
As I understand it, at the end of the day we should always create a buffer.
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.
I will try an optimization in the inner regions, I am now closing this pr, thank you very much for your comments
If there's only one FastBuffer and its length matches the total length, we don't need to manually allocate and copy — we can just return Buffer.from(buf) to keep the semantics (new buffer) but avoid extra work.
my local result: