Skip to content

util: fix TextEncoder.encodeInto underfilling - #65997

Open
XadillaX wants to merge 1 commit into
nodejs:mainfrom
XadillaX:fix-textencoder-encodeinto-underfill
Open

XadillaX wants to merge 1 commit into
nodejs:mainfrom
XadillaX:fix-textencoder-encodeinto-underfill

Conversation

@XadillaX

@XadillaX XadillaX commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

TextEncoder.encodeInto() could underfill narrow destination buffers on the optimized path because:

  • UTF-16 code units below U+0800 were treated as three-byte UTF-8 sequences starting at U+0400.
  • Latin-1 input was exposed as signed char while calculating the scalar tail.
  • Surrogate pairs in the scalar tail could be split at the destination boundary.

This change keeps the existing ASCII fast path and replaces the separate prefix-sizing and conversion passes with local bounded conversion helpers. They return both consumed input code units and written UTF-8 bytes, use simdutf for prefixes that are guaranteed to fit, and use scalar conversion at the destination boundary. Malformed UTF-16 is replaced with U+FFFD without creating a temporary well-formed copy.

The helpers are adapted from the APIs added in simdutf/simdutf#1035. They can be removed once Node's vendored simdutf provides those APIs.

Tests cover all destination capacities for Latin-1 and mixed UTF-16 input, including UTF-8 width boundaries, valid surrogate pairs, unpaired surrogates, and exact-fit SIMD chunks. No documentation changes are required.

Benchmark command:

out/Release/node benchmark/util/text-encoder.js \
  op=encodeInto n=1000000 len=256 type=<type>
Input Before After
ASCII 30.15M ops/s 30.93M ops/s
Latin-1 14.71M ops/s 19.22M ops/s
UTF-16 9.61M ops/s 10.54M ops/s

Fixes: #65994

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 12, 2026
@XadillaX
XadillaX force-pushed the fix-textencoder-encodeinto-underfill branch from 8afb2c6 to 6417dc3 Compare September 12, 2026 06:30
@XadillaX
XadillaX force-pushed the fix-textencoder-encodeinto-underfill branch from 6417dc3 to fe02f30 Compare September 12, 2026 06:47
@codecov

codecov Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.29%. Comparing base (565f69f) to head (94c65b8).
⚠️ Report is 77 commits behind head on main.

Files with missing lines Patch % Lines
src/encoding_binding.cc 96.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65997      +/-   ##
==========================================
+ Coverage   89.99%   90.29%   +0.30%     
==========================================
  Files         784      789       +5     
  Lines      268410   271453    +3043     
  Branches    51124    51804     +680     
==========================================
+ Hits       241562   245116    +3554     
+ Misses      17385    16825     -560     
- Partials     9463     9512      +49     
Files with missing lines Coverage Δ
src/encoding_binding.cc 89.80% <96.00%> (+28.69%) ⬆️

... and 119 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 requested a review from anonrig September 13, 2026 12:19
@XadillaX
XadillaX force-pushed the fix-textencoder-encodeinto-underfill branch 3 times, most recently from 90b0f2f to 90e9b90 Compare September 16, 2026 06:23
Replace the separate prefix-sizing and conversion passes with local
bounded conversion helpers adapted from:
simdutf/simdutf#1035

The helpers report both consumed input code units and written UTF-8
bytes while retaining SIMD conversion for guaranteed-fit chunks.

Keep Latin-1 input unsigned and preserve the ASCII fast path. Handle
surrogate pairs atomically and replace unpaired surrogates inline,
avoiding a temporary well-formed copy.

Add coverage across every destination size for Latin-1, mixed UTF-16,
malformed surrogates, and exact-fit SIMD chunks.

Fixes: nodejs#65994
Signed-off-by: XadillaX <i@2333.moe>
@XadillaX
XadillaX force-pushed the fix-textencoder-encodeinto-underfill branch from 90e9b90 to 94c65b8 Compare September 16, 2026 06:24
@XadillaX

Copy link
Copy Markdown
Contributor Author

@anonrig Could you review this when you have time? It now uses local bounded conversion helpers adapted from simdutf/simdutf#1035

@XadillaX

Copy link
Copy Markdown
Contributor Author

@jasnell @RafaelGSS Could either of you review this when you have time? This updates the encodeInto path introduced in #60843

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.

Copilot review overview

🔵 Needs a closer look

The optimized SIMD/scalar boundary logic is complex, and a misleading comment requires correction.

Review effort: Lite
Findings: None

What changed in this PR

Fixes TextEncoder.encodeInto() underfilling in optimized conversions.

Changes:

  • Adds bounded SIMD/scalar encoding helpers.
  • Handles UTF-8 boundaries and malformed surrogates safely.
  • Adds comprehensive destination-capacity regression tests.
File Summary
test/​parallel/​test-whatwg-encoding-textencoder-encodeinto.js Adds regression and boundary coverage.
src/​encoding_binding.cc Reworks bounded encoding and surrogate handling; update the misleading comment about the previous path.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@XadillaX

Copy link
Copy Markdown
Contributor Author

@legendecas @aduh95 Could either of you review this when you have time? It updates the TextEncoder encodeInto path, and the CI is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Development

Successfully merging this pull request may close these issues.

TextEncoder.encodeInto() underfills the destination for some non-ASCII text

3 participants