Repository navigation
Conversation
String flattening can allocate and invalidate the optimized caller during a byteLengthUtf8 call. V8 cannot lazily deoptimize a value-returning fast API call, including its slow fallback. Register byteLengthUtf8 as a regular callback so both flat and non-flat strings can be measured without the unsupported deoptimization. Keep the existing SIMD path for large two-byte strings. Add a GC regression and coverage for optimized calls with different string representations. Fixes: nodejs#66870 Assisted-by: Codex Signed-off-by: Robert Nagy <ronagy@icloud.com>
nodejs-github-bot
added
buffer
There was a problem hiding this comment.
🔵 Needs a closer look
The regression depends on V8 optimization and GC-triggered lazy deoptimization, requiring human runtime verification of pre-fix failure, post-fix success, and stability across supported CI builds.
No open issues 🎉
What changed in this PR
This PR addresses the Buffer.byteLength() process abort reported in #66870 by replacing its UTF-8 fast API binding with a regular callback, allowing garbage-collection-triggered deoptimization.
Changes:
- Removes the fast callback and its registration while retaining the existing SIMD path for large two-byte strings.
- Adds a regression test for garbage collection during string flattening.
- Adds string-representation coverage before and after optimization.
| File | Description |
|---|---|
test/parallel/test-buffer-bytelength-strings.js |
Checks UTF-8 byte lengths across string representations before and after optimization. |
test/parallel/test-buffer-bytelength-gc.js |
Exercises the GC-triggered deoptimization that caused the abort. |
src/node_buffer.cc |
Registers a regular UTF-8 byte-length callback and removes the fast API implementation. |
Review effort: Balanced
ronag
marked this pull request as ready for review
October 30, 2026 07:29
Member
Author
|
Investigating how to possibly re-enable the optimization. But this is pretty bad and should be fixed ASAP. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #67134 +/- ##
==========================================
+ Coverage 90.66% 90.68% +0.02%
==========================================
Files 797 797
Lines 278791 278743 -48
Branches 53620 53602 -18
==========================================
+ Hits 252768 252791 +23
+ Misses 16376 16306 -70
+ Partials 9647 9646 -1
🚀 New features to boost your workflow:
|
nodejs-github-bot
pushed a commit
that referenced
this pull request
Nov 1, 2026
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: #66870 Refs: #67134 Assisted-by: Codex Signed-off-by: Robert Nagy <ronagy@icloud.com> PR-URL: #67136 Refs: #66870 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
aduh95
pushed a commit
that referenced
this pull request
Nov 11, 2026
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: #66870 Refs: #67134 Assisted-by: Codex Signed-off-by: Robert Nagy <ronagy@icloud.com> PR-URL: #67136 Refs: #66870 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Buffer.byteLength() can abort when flattening a string triggers a GC that
lazily deoptimizes its optimized caller. V8 does not support that
deoptimization after a value-returning fast API call, including when the
call takes its slow fallback.
Register byteLengthUtf8 as a regular callback and remove its fast callback.
The existing SIMD path for large two-byte strings remains. This removes
the fast API optimization for this binding and avoids shared result state
or startup changes.
Add a GC regression and coverage for flat, Latin-1, cons, sliced, two-byte,
lone-surrogate and external strings before and after optimization.
Validation on Linux x64, release build of main:
Fixes: #66870
AI assistance: Codex helped prepare the change and regression tests.