Skip to content

buffer: improve byteLength performance - #58048

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

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

Conversation

@anonrig

@anonriganonrig commented Apr 27, 2025

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/

@nodejs-github-botnodejs-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
anonrigforce-pushed the yagiz/fast-byte-length branch 2 times, most recently from 32ec624 to 61ada9cCompareApril 27, 2025 02:35
@codecov

codecovBot commented Apr 27, 2025

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 linesPatch %Lines
src/node_buffer.cc41.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 linesCoverage Δ
src/node_external_reference.h100.00% <ø> (ø)
src/node_buffer.cc67.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.

@H4ad

This comment was marked as outdated.

@ronagronag 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.

Looks like some pretty bad performance regressions.

@anonrig
anonrigforce-pushed the yagiz/fast-byte-length branch from 61ada9c to e199141CompareApril 27, 2025 17:01
# Conflicts:
#	src/node_external_reference.h
@anonrig
anonrigforce-pushed the yagiz/fast-byte-length branch from e199141 to 5bf5393CompareApril 27, 2025 17:17
@anonrig

Copy link
Copy Markdown
MemberAuthor

@anonrig
anonrig requested a review from ronagApril 27, 2025 18:01
mcollina
mcollina previously approved these changes Apr 27, 2025
@mcollina
mcollina dismissed their stale reviewApril 27, 2025 19:00

waiting for bench resultd

@anonrig

Copy link
Copy Markdown
MemberAuthor

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
anonrig requested a review from mcollinaApril 27, 2025 19:38
@jasnell

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

Copy link
Copy Markdown
MemberAuthor

cc @ronag

@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

@anonriganonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 28, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Collaborator

Landed in 084a912

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
# 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 pushed a commit that referenced this pull request May 2, 2025
# 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
`Utf8Length()` is deprecated and was replaced during the V8 update, but
a subsequent commit introduced a new occurrence.
Refs: nodejs#58070
Refs: nodejs#58048
@aduh95aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label May 6, 2025
@aduh95

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

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
`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
`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>
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.backport-requested-v22.xPRs awaiting manual backport to the v22.x-staging branch.bufferIssues and PRs related to the buffer subsystem.c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@anonrig@H4ad@jasnell@nodejs-github-bot@aduh95@BridgeAR@mcollina@lemire@ronag