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

src: refactor EncodeValidUtf8 to reuse non-ASCII UTF-8 path by araujogui · Pull Request #63587 · nodejs/node · GitHub

/ node Public

src: refactor EncodeValidUtf8 to reuse non-ASCII UTF-8 path - #63587

Open
araujogui wants to merge 4 commits into
nodejs:mainfrom
araujogui:string-bytes-encode-validated-utf8
ConversationCommits (4)ChecksFiles changed
Open

src: refactor EncodeValidUtf8 to reuse non-ASCII UTF-8 path#63587
araujogui wants to merge 4 commits into
nodejs:mainfrom
araujogui:string-bytes-encode-validated-utf8

Conversation

araujogui commented May 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Upstream PR #63231 added StringBytes::EncodeValidUtf8 but duplicated the EncodeTwoByteString block already present in StringBytes::Encode.

This PR refactors that by extracting a shared EncodeValidNonAsciiUtf8 helper, called from both EncodeValidUtf8 and the existing Encode UTF-8 path.

Copilot AI review requested due to automatic review settings May 26, 2026 17:30
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 26, 2026
araujogui force-pushed the string-bytes-encode-validated-utf8 branch from 92897bc to a2ccfb5 Compare May 26, 2026 17:31
Copilot AI reviewed May 26, 2026
View reviewed changes

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.

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR optimizes UTF-8 decoding in the encoding binding by avoiding redundant UTF-8 validation when input has already been validated.

Changes:

  • Added StringBytes::EncodeValidatedUTF8() API for encoding already-validated UTF-8.
  • Refactored the fast UTF-8→UTF-16 conversion into a shared helper (EncodeKnownValidNonAsciiUTF8).
  • Updated BindingData::DecodeUTF8() to use the new API after performing UTF-8 validation.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/string_bytes.h Declares a new EncodeValidatedUTF8() entry point for already-validated UTF-8.
src/string_bytes.cc Implements EncodeValidatedUTF8() and factors out the fast conversion helper used by both code paths.
src/encoding_binding.cc Uses EncodeValidatedUTF8() after explicit UTF-8 validation to avoid re-validation overhead.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/string_bytes.h Outdated
Comment thread src/string_bytes.cc Outdated
Comment thread src/string_bytes.cc Outdated

codecov Bot commented May 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.35%. Comparing base (37779fd) to head (f08ad6f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/string_bytes.cc 66.66% 2 Missing and 3 partials ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #63587   +/-   ##
=======================================
  Coverage   90.34%   90.35%           
=======================================
  Files         732      732           
  Lines      236689   236681    -8     
  Branches    44586    44584    -2     
=======================================
+ Hits       213839   213842    +3     
+ Misses      14550    14548    -2     
+ Partials     8300     8291    -9
Files with missing lines Coverage Δ
src/string_bytes.h 80.00% <ø> (ø)
src/string_bytes.cc 75.73% <66.66%> (+2.17%) ⬆️

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

araujogui force-pushed the string-bytes-encode-validated-utf8 branch 2 times, most recently from 727b683 to 6a919f6 Compare May 26, 2026 23:01
araujogui changed the title encoding: remove double encoding validation on fatal path src: remove double encoding validation on fatal path May 26, 2026

araujogui commented May 27, 2026

Copy link
Copy Markdown
Member Author

CC @nodejs/performance

araujogui force-pushed the string-bytes-encode-validated-utf8 branch from 6a919f6 to 183258a Compare May 27, 2026 01:33

mertcanaltin commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

I also tried to fix this in #63231, but this pr more generally, good work!

araujogui commented May 27, 2026

Copy link
Copy Markdown
Member Author

I also tried to fix this in #63231, but this pr more generally, good work!

Uh, my bad, I haven't seen it. I'm okay with closing this PR in favour of yours.

mertcanaltin commented May 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

No problem at all, thanks for your effort. This PR seems like a better solution to me, @addaleax, Do you have any thoughts?

araujogui force-pushed the string-bytes-encode-validated-utf8 branch 3 times, most recently from b711577 to e41315d Compare May 30, 2026 19:35

araujogui commented May 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

@mertcanaltin @addaleax I rebased the branch, it's just a small DRY refactoring now

araujogui changed the title src: remove double encoding validation on fatal path src: refactor EncodeValidUtf8 to reuse non-ASCII UTF-8 path May 30, 2026
araujogui force-pushed the string-bytes-encode-validated-utf8 branch from aabbe55 to c02c51d Compare May 31, 2026 00:34
araujogui force-pushed the string-bytes-encode-validated-utf8 branch from c02c51d to f361b77 Compare June 2, 2026 00:19
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

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
+1 more reviewer
mertcanaltin mertcanaltin approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. 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.

4 participants

Back | FazBrowse Home | New Git URL