Skip to content

Conversation

@joyeecheung
Copy link
Member

@joyeecheungjoyeecheung commented Feb 11, 2023

Depends on #46619

From a local run: I am a bit surprised by the last few numbers, theoretically there should be no difference for them. Curious about what the benchmark CI would show.

 confidence improvement accuracy (*) (**) (***) buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='base64' type='four_bytes' 1.39 % ±1.78% ±2.66% ±4.20% buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='base64' type='one_byte' 0.40 % ±0.62% ±0.95% ±1.56% buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='base64' type='three_bytes' 0.84 % ±3.13% ±5.01% ±8.85% buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='base64' type='two_bytes' -1.23 % ±1.66% ±2.40% ±3.56% buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='four_bytes' * 1.43 % ±1.42% ±2.18% ±3.58% buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='one_byte' *** 32.48 % ±1.42% ±2.05% ±3.04% buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='three_bytes' -0.46 % ±2.74% ±4.13% ±6.60% buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='two_bytes' -0.20 % ±2.23% ±3.21% ±4.74% buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='base64' type='four_bytes' -0.06 % ±1.80% ±2.64% ±4.01% buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='base64' type='one_byte' 0.47 % ±1.09% ±1.63% ±2.59% buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='base64' type='three_bytes' 0.41 % ±1.33% ±2.01% ±3.19% buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='base64' type='two_bytes' 1.26 % ±3.57% ±5.13% ±7.56% buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='utf8' type='four_bytes' *** 3.23 % ±0.78% ±1.17% ±1.84% buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='utf8' type='one_byte' 0.57 % ±5.18% ±7.99% ±13.25% buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='utf8' type='three_bytes' *** 21.65 % ±0.91% ±1.31% ±1.93% buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='utf8' type='two_bytes' *** 46.84 % ±2.73% ±4.37% ±7.68% buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='base64' type='four_bytes' -1.38 % ±2.36% ±3.67% ±6.16% buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='base64' type='one_byte' -0.17 % ±2.79% ±4.40% ±7.58% buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='base64' type='three_bytes' -0.31 % ±1.17% ±1.70% ±2.56% buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='base64' type='two_bytes' 0.04 % ±1.84% ±2.64% ±3.88% buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='four_bytes' 0.22 % ±1.54% ±2.24% ±3.38% buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='one_byte' 0.64 % ±1.49% ±2.14% ±3.15% buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='three_bytes' 2.56 % ±2.77% ±3.98% ±5.85% buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='two_bytes' * 8.26 % ±7.40% ±11.39% ±18.79% buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='four_bytes' -0.12 % ±0.67% ±0.98% ±1.47% buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='one_byte' 0.40 % ±1.44% ±2.08% ±3.05% buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='three_bytes' 0.17 % ±2.13% ±3.10% ±4.66% buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='two_bytes' 0.25 % ±1.49% ±2.29% ±3.76% buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='utf8' type='four_bytes' *** 4.59 % ±0.85% ±1.27% ±2.01% buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='utf8' type='one_byte' 0.10 % ±1.32% ±1.98% ±3.15% buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='utf8' type='three_bytes' *** 32.02 % ±0.44% ±0.67% ±1.10% buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='utf8' type='two_bytes' *** 74.78 % ±0.73% ±1.14% ±1.91% 

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 11, 2023
@ronag
Copy link
Member

@anonrig, do we even need to go into native functions for short buffers? e.g. 1 byte?

@anonrig
Copy link
Member

@anonrig, do we even need to go into native functions for short buffers? e.g. 1 byte?

In this context: One byte refers to buffers with one byte string representations such as Ascii text. Using native calls will always be faster.

"This is the way" - Mandalorian

@joyeecheungjoyeecheung marked this pull request as ready for review February 13, 2023 15:56
Copy link
Member

@addaleaxaddaleax left a comment

Choose a reason for hiding this comment

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

(re-approving to make sure the github bot is happy)

@nodejs-github-bot
Copy link
Collaborator

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 20, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonriganonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 21, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 21, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/46616 ✔ Done loading data for nodejs/node/pull/46616 ----------------------------------- PR info ------------------------------------ Title buffer: use v8 fast API calls for Buffer.byteLength with sequential one-byte strings (#46616) Author Joyee Cheung (@joyeecheung) Branch joyeecheung:byte-length-one-byte -> nodejs:main Labels c++, lib / src, author ready, needs-ci Commits 5 - src: add SetFastMethodNoSideEffect() - benchmark: split Buffer.byteLength benchmark - buffer: use v8 fast API calls for Buffer.byteLength with sequential o… - fixup! buffer: use v8 fast API calls for Buffer.byteLength with seque… - fixup! benchmark: split Buffer.byteLength benchmark Committers 2 - Joyee Cheung - GitHub PR-URL: https://github.com/nodejs/node/pull/46616 Reviewed-By: Anna Henningsen Reviewed-By: Robert Nagy Reviewed-By: Yagiz Nizipli Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46616 Reviewed-By: Anna Henningsen Reviewed-By: Robert Nagy Reviewed-By: Yagiz Nizipli Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 11 Feb 2023 18:32:19 GMT ✔ Approvals: 4 ✔ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/46616#pullrequestreview-1295966149 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/46616#pullrequestreview-1296865088 ✔ - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/46616#pullrequestreview-1296774718 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/46616#pullrequestreview-1304930817 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-02-20T10:26:36Z: https://ci.nodejs.org/job/node-test-pull-request/49775/ - Querying data for job/node-test-pull-request/49775/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 46616 From https://github.com/nodejs/node * branch refs/pull/46616/merge -> FETCH_HEAD ✔ Fetched commits as b85b5ba10cd7..955cc5215476 -------------------------------------------------------------------------------- Auto-merging src/util.h [main 72bc87f801] src: add SetFastMethodNoSideEffect() Author: Joyee Cheung Date: Sat Feb 11 17:47:46 2023 +0100 3 files changed, 29 insertions(+), 2 deletions(-) [main 63a2cfe78e] benchmark: split Buffer.byteLength benchmark Author: Joyee Cheung Date: Sat Feb 11 17:48:17 2023 +0100 3 files changed, 62 insertions(+), 49 deletions(-) create mode 100644 benchmark/buffers/buffer-bytelength-buffer.js create mode 100644 benchmark/buffers/buffer-bytelength-string.js delete mode 100644 benchmark/buffers/buffer-bytelength.js Auto-merging src/node_external_reference.h [main ce060a1e41] buffer: use v8 fast API calls for Buffer.byteLength with sequential one-byte strings Author: Joyee Cheung Date: Sat Feb 11 17:49:32 2023 +0100 2 files changed, 28 insertions(+), 3 deletions(-) [main e2f3d9f5fb] fixup! buffer: use v8 fast API calls for Buffer.byteLength with sequential one-byte strings Author: Joyee Cheung Date: Mon Feb 13 16:57:01 2023 +0100 1 file changed, 1 insertion(+), 1 deletion(-) [main 24e347e7f3] fixup! benchmark: split Buffer.byteLength benchmark Author: Joyee Cheung Date: Sun Feb 19 21:53:12 2023 +0100 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 5 commits in the PR. Attempting autorebase. Rebasing (2/8) 

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
src: add SetFastMethodNoSideEffect()

PR-URL: #46616
Reviewed-By: Anna Henningsen [email protected]
Reviewed-By: Robert Nagy [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD d7d1c966f8] src: add SetFastMethodNoSideEffect()
Author: Joyee Cheung [email protected]
Date: Sat Feb 11 17:47:46 2023 +0100
3 files changed, 29 insertions(+), 2 deletions(-)
Rebasing (3/8)
Rebasing (4/8)
Rebasing (5/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
benchmark: split Buffer.byteLength benchmark

PR-URL: #46616
Reviewed-By: Anna Henningsen [email protected]
Reviewed-By: Robert Nagy [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD d6ab33d600] benchmark: split Buffer.byteLength benchmark
Author: Joyee Cheung [email protected]
Date: Sat Feb 11 17:48:17 2023 +0100
3 files changed, 62 insertions(+), 49 deletions(-)
create mode 100644 benchmark/buffers/buffer-bytelength-buffer.js
create mode 100644 benchmark/buffers/buffer-bytelength-string.js
delete mode 100644 benchmark/buffers/buffer-bytelength.js
Rebasing (6/8)
Rebasing (7/8)
Rebasing (8/8)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
buffer: use v8 fast API calls for Buffer.byteLength with sequential one-byte strings

PR-URL: #46616
Reviewed-By: Anna Henningsen [email protected]
Reviewed-By: Robert Nagy [email protected]
Reviewed-By: Yagiz Nizipli [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD 01a8e5500b] buffer: use v8 fast API calls for Buffer.byteLength with sequential one-byte strings
Author: Joyee Cheung [email protected]
Date: Sat Feb 11 17:49:32 2023 +0100
2 files changed, 28 insertions(+), 3 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

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

@nodejs-github-botnodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 21, 2023
@aduh95aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 26, 2023
PR-URL: nodejs#46616 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
Use v8 fast API calls for Buffer.byteLength with sequential one-byte strings. PR-URL: nodejs#46616 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
@aduh95aduh95force-pushed the byte-length-one-byte branch from 955cc52 to ee1ce18CompareFebruary 26, 2023 18:03
@aduh95
Copy link
Contributor

Landed in 55dd283...ee1ce18

@aduh95aduh95 merged commit ee1ce18 into nodejs:mainFeb 26, 2023
targos pushed a commit that referenced this pull request Mar 13, 2023
PR-URL: #46616 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Mar 13, 2023
Use v8 fast API calls for Buffer.byteLength with sequential one-byte strings. PR-URL: #46616 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Mar 14, 2023
PR-URL: #46616 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Mar 14, 2023
Use v8 fast API calls for Buffer.byteLength with sequential one-byte strings. PR-URL: #46616 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
@targostargos mentioned this pull request Mar 14, 2023
@danielleadams
Copy link
Contributor

This is blocked by #45788

danielleadams pushed a commit that referenced this pull request Apr 11, 2023
PR-URL: #46616 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
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.c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@joyeecheung@ronag@anonrig@nodejs-github-bot@aduh95@danielleadams@jasnell@addaleax