Skip to content

buffer: faster type check - #54088

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
nxtedition:buffer.copy-check
Aug 3, 2024
Merged

buffer: faster type check#54088
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
nxtedition:buffer.copy-check

Conversation

@ronag

@ronagronag commented Jul 28, 2024

Copy link
Copy Markdown
Member

Also add support for any TypedArray as target.

buffers/buffer-copy.js n=6000000 partial='true' bytes=128 *** 50.28 % ±4.26% ±5.68% ±7.42%
buffers/buffer-copy.js n=6000000 partial='true' bytes=8 *** 45.58 % ±4.53% ±6.04% ±7.89%

@ronag

Copy link
Copy Markdown
MemberAuthor

@nodejs/performance @nodejs/buffer

@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Jul 28, 2024
@ronag
ronag requested a review from mcollinaJuly 28, 2024 22:24
@mcollinamcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronagronag added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronag
ronagforce-pushed the buffer.copy-check branch from 0db82cb to 1235ae8CompareJuly 29, 2024 08:45
@ronag
ronagforce-pushed the buffer.copy-check branch from 1235ae8 to e993fc6CompareJuly 29, 2024 08:46
@ronag
ronagforce-pushed the buffer.copy-check branch from e993fc6 to 21a5831CompareJuly 29, 2024 08:46
@ronagronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 29, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronag
ronagforce-pushed the buffer.copy-check branch from 21a5831 to 4f088e6CompareJuly 29, 2024 09:16
@ronagronag changed the title buffers: faster type checkbuffer: faster type checkJul 29, 2024
@codecov

codecovBot commented Jul 29, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.07%. Comparing base (76c8ba9) to head (2d75eba).
Report is 519 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #54088 +/- ##
=======================================
Coverage 87.06% 87.07% =======================================
Files 643 643 Lines 181581 181582 +1 Branches 34888 34886 -2 =======================================
+ Hits 158097 158107 +10 + Misses 16766 16758 -8 + Partials 6718 6717 -1 
Files with missing linesCoverage Δ
lib/buffer.js96.42% <100.00%> (+<0.01%)⬆️

... and 25 files with indirect coverage changes

@tniessen

Copy link
Copy Markdown
Member

The commit message has to be amended to comply with our guidelines (prefix: imperative verb) before this can land :)

@tniessen

Copy link
Copy Markdown
Member

Is this semver-minor since it changes the public API? In any case, I think it makes sense to document the change in behavior. I assume the right place is to add a changes: block here:

node/doc/api/buffer.md

Lines 1711 to 1715 in 269175b

### `buf.copy(target[, targetStart[, sourceStart[, sourceEnd]]])`
<!-- YAML
added: v0.1.90
-->

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronagronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 2, 2024
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 2, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/54088
✔ Done loading data for nodejs/node/pull/54088
----------------------------------- PR info ------------------------------------
Title buffer: faster type check (#54088)
Author Robert Nagy <ronagy@icloud.com> (@ronag)
Branch ronag:buffer.copy-check -> nodejs:main
Labels buffer, author ready, needs-ci
Commits 1
- buffer: faster type check
Committers 1
- Robert Nagy <ronagy@icloud.com>
PR-URL: https://github.com/nodejs/node/pull/54088
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54088
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
--------------------------------------------------------------------------------
ℹ This PR was created on Sun, 28 Jul 2024 22:18:55 GMT
✔ Approvals: 4
✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54088#pullrequestreview-2210972720
✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54088#pullrequestreview-2204005543
✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54088#pullrequestreview-2205608335
✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/54088#pullrequestreview-2206203933
⚠ This PR has conflicts that must be resolved
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2024-08-02T05:56:32Z: https://ci.nodejs.org/job/node-test-pull-request/60820/
- Querying data for job/node-test-pull-request/60820/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/10213732908

Also add support for any TypedArray as target.
PR-URL: nodejs#54088
@ronag
ronag requested a review from anonrigAugust 2, 2024 11:13
@ronag
ronagforce-pushed the buffer.copy-check branch from 324f2c6 to 2d75ebaCompareAugust 2, 2024 11:14
@ronag
ronag requested review from jasnell and mcollinaAugust 3, 2024 11:33
@ronagronag added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 3, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronagronag added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronagronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 3, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 3, 2024
@nodejs-github-bot
nodejs-github-bot merged commit acbd0be into nodejs:mainAug 3, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in acbd0be

targos pushed a commit that referenced this pull request Aug 14, 2024
Also add support for any TypedArray as target.
PR-URL: #54088
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Aug 19, 2024
@ChALkeR

ChALkeR commented Oct 26, 2025

Copy link
Copy Markdown
Member

This was clearly a semver-minor change

> Buffer.of(1,2,3,4).copy(x=new Uint16Array(4));xUint16Array(4) [ 513, 1027, 0, 0 ]

Do we have a test for that so it won't regress causing an unnoticed breaking change?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.bufferIssues and PRs related to the buffer subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@ronag@nodejs-github-bot@tniessen@ChALkeR@mcollina@jasnell@benjamingr@anonrig@targos