Skip to content

buffer: limit UTF-8 fast calls to safe one-byte strings - #67136

Open
ronag wants to merge 2 commits into
nodejs:mainfrom
ronag:codex/buffer-bytelength-safe-fast-20261030
Open

ronag wants to merge 2 commits into
nodejs:mainfrom
ronag:codex/buffer-bytelength-safe-fast-20261030

Conversation

@ronag

@ronag ronag commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

Alternative to #67134. This is a
standalone change against main; it does not depend on that PR.

Keep a fast UTF-8 byte-length path for sequential one-byte strings.
The fast binding's slow fallback returns the Smi sentinel -1 without
reading or flattening the string. JavaScript then calls the regular
binding separately. Both the fast callback and its fallback avoid JS-heap
allocation; allocating string handling uses an ordinary API continuation.

Validation on Linux x64, GCC 14.2.0, release builds of v28.0.0-pre:

  • Candidate 44bd1bfa65f. The local regular-callback baseline
    35cf29a820a is main 250037174ee with the removal fix from buffer: avoid fast API calls when computing UTF-8 byte length #67134,
    used only for the benchmark comparison.
  • A negative control using a one-byte signature with an allocating slow
    fallback aborts both the direct and HTTP reproducers in 3/3 runs each.
  • The candidate passes the direct reproducer 10/10 and HTTP 5/5. Traces
    show actual lazy deoptimization after weak objects are cleared, followed
    by normal completion, including end() and maybePrepareFinalChunk().
  • All 531 buffer, HTTP, string-decoder and snapshot tests pass, including
    explicit optimized fast-acceptance and rejection checks.
  • Main thread, four workers, and a user-land startup snapshot pass the
    string-representation corpus before and after optimization.
  • Additional GC stress, ESLint, focused binding type checking, cpplint,
    import, C++ formatting and whitespace checks pass.

Benchmark tradeoff versus the regular-callback baseline, ten alternating
runs per binary on one fixed logical CPU, two million calls per case:

UTF-8 input repeat=1 repeat=16 repeat=256
one-byte ASCII +65.03% +/-12.95 -9.70% +/-12.50 -12.64% +/-6.43
Latin-1 +33.18% +/-10.41 -7.12% +/-6.46 -2.15% +/-5.28
two-byte -8.04% +/-4.37 -7.07% +/-4.67 -1.60% +/-5.64

Values are throughput changes with 95% confidence intervals in percentage
points. The repeated one-byte strings take the fallback. Eligible flat
inputs improve, while rejected inputs pay for the additional call.
Latin-1 and base64 encoding controls were also measured; raw results cover
27 configurations and 540 samples. Four comparisons remain significant
at 5% after the benchmark tool's Holm-Bonferroni adjustment.

./node benchmark/compare.js --old ./node-baseline --new ./node-candidate \
  --runs 10 --filter buffer-bytelength-string \
  --set encoding=utf8 --set encoding=latin1 --set encoding=base64 \
  --set type=one_byte --set type=latin1 --set type=two_bytes \
  --set repeat=1 --set repeat=16 --set repeat=256 --set n=2000000 \
  --set CPUSET=1 --no-progress --analyze --csv benchmark.csv buffers

Refs: #66870

AI assistance: Codex helped prepare the implementation and tests.

Replace the allocating UTF-8 fast call with a fast binding for
sequential one-byte strings. Its slow fallback only returns a Smi
sentinel. JavaScript sends rejected strings to a separate regular
binding that can flatten strings and handle lazy deoptimization.

Both native callbacks used by the fast API avoid JS-heap allocation.
Add GC regression and string-representation coverage, including
optimized fast acceptance and rejection checks.

Fixes: nodejs#66870
Refs: nodejs#67134
Assisted-by: Codex
Signed-off-by: Robert Nagy <ronagy@icloud.com>
@ronag
ronag requested a balanced review from Copilot October 30, 2026 08:16
@nodejs-github-bot 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. typings Issues and PRs related to internal TypeScript declarations. labels Oct 30, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

V8-aware maintainer review and supported-platform GC-stress CI are needed to validate the allocation-free callback/fallback contract and fast-call lazy-deoptimization behavior.

No open issues 🎉

What changed in this PR

This PR addresses the Buffer.byteLength() abort reported in #66870 while preserving a fast UTF-8 byte-length path for sequential one-byte strings. Rejected strings use a separate regular binding call so allocating string handling avoids the unsupported fast-call deoptimization path.

Changes:

  • Restricts the native fast callback to one-byte strings and makes its fallback return -1 without inspecting the string.
  • Adds a JavaScript wrapper and binding type declaration for the separate fast path.
  • Adds coverage for optimized acceptance and rejection, string representations, and garbage-collection-triggered deoptimization.
File Description
typings/​internalBinding/​buffer.d.ts Declares the new fast byte-length binding.
test/​parallel/​test-buffer-bytelength-strings.js Checks byte lengths across string representations before and after optimization.
test/​parallel/​test-buffer-bytelength-safe-fast.js Tests fast-path acceptance, rejection, and public API results.
test/​parallel/​test-buffer-bytelength-gc.js Adds regression coverage for deoptimization during string flattening.
src/​node_buffer.cc Implements the restricted fast callback and non-allocating sentinel fallback.
lib/​buffer.js Routes rejected fast calls to the regular UTF-8 binding.

Review effort: Balanced

@ronag
ronag marked this pull request as ready for review October 30, 2026 08:20
@ronag
ronag marked this pull request as draft October 30, 2026 08:24
Keep the existing loop unrolling, variable names and scalar tail. Limit the fast-path changes to the safe input and fallback contract.

Assisted-by: Codex
Signed-off-by: Robert Nagy <ronagy@icloud.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

V8-aware maintainer review and GC/deoptimization stress validation are needed to confirm the fast-call continuation and no-GC invariants across supported configurations.

No open issues 🎉

Review effort: Balanced

@ronag
ronag marked this pull request as ready for review October 30, 2026 08:38
@ronag
ronag requested review from anonrig and mcollina October 30, 2026 08:42

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

lgtm

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 30, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 30, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronag ronag added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 30, 2026
@codecov

codecov Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.69%. Comparing base (2500371) to head (44bd1bf).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #67136      +/-   ##
==========================================
+ Coverage   90.67%   90.69%   +0.01%     
==========================================
  Files         797      797              
  Lines      278806   278803       -3     
  Branches    53621    53619       -2     
==========================================
+ Hits       252813   252861      +48     
+ Misses      16341    16305      -36     
+ Partials     9652     9637      -15     
Files with missing lines Coverage Δ
lib/buffer.js 99.74% <100.00%> (+<0.01%) ⬆️
src/node_buffer.cc 74.86% <100.00%> (+3.19%) ⬆️

... and 23 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.

@ronag ronag added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Oct 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. needs-ci PRs that need a full CI run. typings Issues and PRs related to internal TypeScript declarations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants