-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
buffer: speed up concat via TypedArray#set #60399
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60399 +/- ##
==========================================
- Coverage 88.56% 88.56% -0.01%
==========================================
Files 704 704
Lines 207774 207832 +58
Branches 40025 40048 +23
==========================================
+ Hits 184022 184066 +44
+ Misses 15807 15806 -1
- Partials 7945 7960 +15
🚀 New features to boost your workflow:
|
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.
lgtm
|
Benchmark GHA: https://github.com/aduh95/node/actions/runs/18809427089 Results (improvements across the board) |
|
@aduh95 the CI didn't pick up the benchmarks, I think the category is |
This reverts commit 34adb7c.
This comment was marked as resolved.
This comment was marked as resolved.
|
No, they claimed an improvement for However,
Funnily enough, we weren't using primordials here before, but anyway, that change is already made |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Nikita Skovoroda <chalkerx@gmail.com>
|
We might need a CI run to verify |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
|
The AIX and LinuxOne CI failures look like real issues with this PR and is most likely due to not handling endianness properly. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Yeah my fault it's the test, will fix it later today |
|
@richardlau thanks, should be fixed @aduh95 PTAL, it's ready for review again |
Commit Queue failed- Loading data for nodejs/node/pull/60399 ✔ Done loading data for nodejs/node/pull/60399 ----------------------------------- PR info ------------------------------------ Title buffer: speed up concat via TypedArray#set (#60399) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch gurgunday:buffer-concat -> nodejs:main Labels buffer, performance, author ready, needs-ci, commit-queue-squash Commits 10 - buffer: speed up concat via TypedArray#set - complete coverage - store bufLength in a variable - fix compat issue - lint - simplify - Revert "simplify" - alternative implementation - add type check to set - fix test Committers 1 - Gürgün Dayıoğlu <hey@gurgun.day> PR-URL: https://github.com/nodejs/node/pull/60399 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/60399 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 25 Oct 2025 00:47:41 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/60399#pullrequestreview-3380174635 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/60399#pullrequestreview-3380355020 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/60399#pullrequestreview-3405890961 ✔ Last GitHub CI successful ℹ Last Benchmark CI on 2025-10-27T22:47:38Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1748/ ℹ Last Full PR CI on 2025-10-27T23:11:11Z: https://ci.nodejs.org/job/node-test-pull-request/69912/ ⚠ Commits were pushed after the last Full PR CI run: ⚠ - fix test - Querying data for job/node-test-pull-request/69912/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/18983880877 |
|
@aduh95 it needs a CI run after the test fix |
|
Landed in 24bebd0 |
PR-URL: #60399 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Huge win by avoiding complex
_copyActuallogic when we can copy the whole source into target. The native copy implementation is faster only for partial copies.Before:
After: