Skip to content
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.

Sign up for GitHub

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

Jump to bottom

buffer: use v8 fast API calls for Buffer.byteLength with sequential one-byte strings #46616

Merged
merged 2 commits into from
Feb 26, 2023
Merged

buffer: use v8 fast API calls for Buffer.byteLength with sequential one-byte strings #46616

merged 2 commits into from
Feb 26, 2023

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 11, 2023 •
edited
Loading

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-bot nodejs-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
@joyeecheung joyeecheung mentioned this pull request Feb 12, 2023
Open
@ronag
Copy link
Member

ronag commented Feb 13, 2023

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

@anonrig
Copy link
Member

anonrig commented Feb 13, 2023

@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

ronag
ronag reviewed Feb 13, 2023
View reviewed changes
src/node_buffer.cc Outdated Show resolved Hide resolved
@joyeecheung joyeecheung marked this pull request as ready for review February 13, 2023 15:56
addaleax
addaleax approved these changes Feb 13, 2023
View reviewed changes
Copy link
Member

@addaleax addaleax 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

CI: https://ci.nodejs.org/job/node-test-pull-request/49743/

@github-actions github-actions bot mentioned this pull request Feb 20, 2023
Open
24 tasks
@addaleax addaleax 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

CI: https://ci.nodejs.org/job/node-test-pull-request/49775/

@joyeecheung joyeecheung mentioned this pull request Feb 20, 2023
Closed
@github-actions github-actions bot mentioned this pull request Feb 21, 2023
Open
26 tasks
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 21, 2023
@nodejs-github-bot nodejs-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-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 21, 2023
@aduh95 aduh95 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
@joyeecheung @aduh95
benchmark: split Buffer.byteLength benchmark
43ddf76 
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]>
@joyeecheung @aduh95
buffer: use v8 fast API calls for Buffer.byteLength implementation
ee1ce18 
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]>
@aduh95 aduh95 force-pushed the byte-length-one-byte branch from 955cc52 to ee1ce18 Compare February 26, 2023 18:03
@aduh95
Copy link
Contributor

aduh95 commented Feb 26, 2023

Landed in 55dd283...ee1ce18

@aduh95 aduh95 merged commit ee1ce18 into nodejs:main Feb 26, 2023
targos pushed a commit that referenced this pull request Mar 13, 2023
@joyeecheung @targos
benchmark: split Buffer.byteLength benchmark
8a0620f 
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
@joyeecheung @targos
buffer: use v8 fast API calls for Buffer.byteLength implementation
34f8cf4 
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
@joyeecheung @targos
benchmark: split Buffer.byteLength benchmark
04166fe 
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
@joyeecheung @targos
buffer: use v8 fast API calls for Buffer.byteLength implementation
b3e1034 
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 targos mentioned this pull request Mar 14, 2023
Merged
@danielleadams
Copy link
Contributor

This is blocked by #45788

@danielleadams danielleadams added the backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. label Apr 3, 2023
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@joyeecheung @danielleadams
benchmark: split Buffer.byteLength benchmark
d4af671 
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]>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronag ronag ronag approved these changes

@anonrig anonrig anonrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v18.x PRs that should land on the v18.x-staging branch but are blocked by another PR's pending backport. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@joyeecheung @ronag @anonrig @nodejs-github-bot @aduh95 @danielleadams @jasnell @addaleax