FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: improve byteLength performance by anonrig · Pull Request #58048 · nodejs/node · GitHub

/ node Public

buffer: improve byteLength performance - #58048

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:yagiz/fast-byte-length
Apr 29, 2025
ConversationCommits (1)ChecksFiles changed
Merged

buffer: improve byteLength performance#58048
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:yagiz/fast-byte-length

Conversation

anonrig commented Apr 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

This improves the performance for non one-byte strings.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1705/

anonrig requested review from H4ad, jasnell, lemire and ronag April 27, 2025 02:20
nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 27, 2025
anonrig force-pushed the yagiz/fast-byte-length branch 2 times, most recently from 32ec624 to 61ada9c Compare April 27, 2025 02:35

codecov Bot commented Apr 27, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 41.66667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 90.19%. Comparing base (e0cf8ae) to head (5bf5393).
Report is 102 commits behind head on main.

Files with missing lines Patch % Lines
src/node_buffer.cc 41.66% 5 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58048      +/-   ##
==========================================
+ Coverage   90.18%   90.19%   +0.01%     
==========================================
  Files         630      630              
  Lines      186393   186400       +7     
  Branches    36595    36601       +6     
==========================================
+ Hits       168103   168131      +28     
+ Misses      11090    11086       -4     
+ Partials     7200     7183      -17
Files with missing lines Coverage Δ
src/node_external_reference.h 100.00% <ø> (ø)
src/node_buffer.cc 67.63% <41.66%> (+0.12%) ⬆️

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This comment was marked as outdated.

Sign in to view
ronag requested changes Apr 27, 2025
View reviewed changes

ronag 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks like some pretty bad performance regressions.

anonrig force-pushed the yagiz/fast-byte-length branch from 61ada9c to e199141 Compare April 27, 2025 17:01
buffer: improve byteLength performance
5bf5393 
# Conflicts:
#	src/node_external_reference.h
anonrig force-pushed the yagiz/fast-byte-length branch from e199141 to 5bf5393 Compare April 27, 2025 17:17

anonrig commented Apr 27, 2025

Copy link
Copy Markdown
Member Author

One more shot: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1705/

anonrig requested a review from ronag April 27, 2025 18:01
mcollina previously approved these changes Apr 27, 2025
View reviewed changes
mcollina dismissed their stale review April 27, 2025 19:00

waiting for bench resultd

anonrig commented Apr 27, 2025

Copy link
Copy Markdown
Member Author

The results are in

15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='base64' type='four_bytes'                                                                      1.84 %       ±3.25%  ±4.32%  ±5.62%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='base64' type='latin1'                                                                         -1.79 %       ±3.18%  ±4.24%  ±5.51%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='base64' type='one_byte'                                                                       -0.58 %       ±2.71%  ±3.61%  ±4.69%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='base64' type='three_bytes'                                                                    -1.06 %       ±3.23%  ±4.30%  ±5.59%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='base64' type='two_bytes'                                                                       0.75 %       ±3.07%  ±4.09%  ±5.32%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='four_bytes'                                                                       -0.60 %       ±0.88%  ±1.18%  ±1.54%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='latin1'                                                                            0.83 %       ±1.00%  ±1.33%  ±1.73%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='one_byte'                                                                   *     -2.74 %       ±2.07%  ±2.77%  ±3.62%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='three_bytes'                                                              ***     -2.52 %       ±0.89%  ±1.18%  ±1.54%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=1 encoding='utf8' type='two_bytes'                                                                  *     -1.88 %       ±1.52%  ±2.02%  ±2.63%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='base64' type='four_bytes'                                                                     0.23 %       ±2.36%  ±3.14%  ±4.09%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='base64' type='latin1'                                                                         0.84 %       ±2.40%  ±3.19%  ±4.15%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='base64' type='one_byte'                                                                      -2.16 %       ±2.81%  ±3.73%  ±4.86%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='base64' type='three_bytes'                                                             *     -2.65 %       ±2.49%  ±3.32%  ±4.32%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='base64' type='two_bytes'                                                                     -2.01 %       ±2.63%  ±3.49%  ±4.55%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='utf8' type='four_bytes'                                                                       0.01 %       ±0.12%  ±0.16%  ±0.20%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='utf8' type='latin1'                                                                    *      0.12 %       ±0.11%  ±0.15%  ±0.19%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='utf8' type='one_byte'                                                                ***      3.05 %       ±0.80%  ±1.07%  ±1.39%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='utf8' type='three_bytes'                                                               *      0.29 %       ±0.27%  ±0.36%  ±0.47%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=16 encoding='utf8' type='two_bytes'                                                               ***      0.91 %       ±0.35%  ±0.47%  ±0.61%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='base64' type='four_bytes'                                                                      0.30 %       ±2.71%  ±3.61%  ±4.70%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='base64' type='latin1'                                                                         -0.26 %       ±2.60%  ±3.46%  ±4.51%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='base64' type='one_byte'                                                                       -1.51 %       ±3.12%  ±4.16%  ±5.41%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='base64' type='three_bytes'                                                                    -2.08 %       ±2.33%  ±3.10%  ±4.04%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='base64' type='two_bytes'                                                                      -0.71 %       ±2.41%  ±3.20%  ±4.17%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='four_bytes'                                                                 *     -0.89 %       ±0.70%  ±0.94%  ±1.22%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='latin1'                                                                   ***      2.30 %       ±0.40%  ±0.53%  ±0.69%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='one_byte'                                                                 ***     -5.70 %       ±2.12%  ±2.83%  ±3.70%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='three_bytes'                                                              ***      7.95 %       ±1.76%  ±2.34%  ±3.06%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='two_bytes'                                                                ***      3.18 %       ±1.50%  ±1.99%  ±2.60%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='four_bytes'                                                                   -0.37 %       ±2.43%  ±3.24%  ±4.23%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='latin1'                                                                       -1.11 %       ±2.77%  ±3.68%  ±4.79%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='one_byte'                                                                      1.55 %       ±2.98%  ±3.96%  ±5.16%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='three_bytes'                                                                   1.09 %       ±2.57%  ±3.42%  ±4.45%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='two_bytes'                                                                    -0.58 %       ±2.20%  ±2.93%  ±3.81%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='utf8' type='four_bytes'                                                             ***     -0.01 %       ±0.01%  ±0.01%  ±0.01%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='utf8' type='latin1'                                                                 ***     -0.20 %       ±0.03%  ±0.04%  ±0.06%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='utf8' type='one_byte'                                                                       -0.02 %       ±0.20%  ±0.27%  ±0.35%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='utf8' type='three_bytes'                                                                     0.02 %       ±0.03%  ±0.04%  ±0.05%
15:35:23 buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='utf8' type='two_bytes'                                                              ***     -0.78 %       ±0.09%  ±0.12%  ±0.16%

anonrig requested a review from mcollina April 27, 2025 19:38

jasnell commented Apr 27, 2025

Copy link
Copy Markdown
Member

From those results it appears like mostly a wash in terms of perf improvement but the change still looks worthwhile to me.

anonrig commented Apr 27, 2025

Copy link
Copy Markdown
Member Author

cc @ronag

mcollina approved these changes Apr 28, 2025
View reviewed changes

mcollina 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

anonrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 28, 2025
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2025

nodejs-github-bot commented Apr 28, 2025

Copy link
Copy Markdown
Collaborator

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

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 29, 2025
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 29, 2025
nodejs-github-bot merged commit 084a912 into nodejs:main Apr 29, 2025

nodejs-github-bot commented Apr 29, 2025

Copy link
Copy Markdown
Collaborator

Landed in 084a912

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
buffer: improve byteLength performance
5335158 
# Conflicts:
#	src/node_external_reference.h

PR-URL: #58048
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
buffer: improve byteLength performance
dc22890 
# Conflicts:
#	src/node_external_reference.h

PR-URL: #58048
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
tniessen added a commit to tniessen/node that referenced this pull request May 4, 2025
buffer: use Utf8LengthV2() instead of Utf8Length()
caf58f2 
`Utf8Length()` is deprecated and was replaced during the V8 update, but
a subsequent commit introduced a new occurrence.

Refs: nodejs#58070
Refs: nodejs#58048
tniessen mentioned this pull request May 4, 2025
Merged
aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label May 6, 2025

aduh95 commented May 6, 2025

Copy link
Copy Markdown
Contributor

This makes build fail on v22.x-staging, we'd need a backport PR if we want it to land there

BridgeAR commented May 6, 2025

Copy link
Copy Markdown
Member

@anonrig since this also regresses a lot of code, should we even do this at all?

nodejs-github-bot pushed a commit that referenced this pull request May 17, 2025
buffer: use Utf8LengthV2() instead of Utf8Length()
4edb139 
`Utf8Length()` is deprecated and was replaced during the V8 update, but
a subsequent commit introduced a new occurrence.

Refs: #58070
Refs: #58048
PR-URL: #58156
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 31, 2025
buffer: use Utf8LengthV2() instead of Utf8Length()
8b5ada4 
`Utf8Length()` is deprecated and was replaced during the V8 update, but
a subsequent commit introduced a new occurrence.

Refs: #58070
Refs: #58048
PR-URL: #58156
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
lemire lemire approved these changes
jasnell jasnell approved these changes
ronag ronag approved these changes
H4ad H4ad approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. 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.

9 participants

Back | FazBrowse Home | New Git URL