Skip to content

Conversation

@anonrig
Copy link
Member

@anonrig anonrig commented Oct 21, 2025

<SharedFunctionInfo encodeInto>}: big function, size (117) >= max-size (100)

It seems encodeInto function is too big to inline with v8. This is an attempt to reduce the size of the function by eliminating validateEncoder() calls, and replacing them with private attribute calls.

This is a semver-major because it changes the errors thrown if this is different.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1743/

@anonrig anonrig requested a review from jasnell October 21, 2025 14:26
@anonrig anonrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 21, 2025
@nodejs-github-bot nodejs-github-bot added encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. needs-ci PRs that need a full CI run. labels Oct 21, 2025
@anonrig anonrig force-pushed the yagiz/reduce-encodeInto-size branch 3 times, most recently from edabae3 to b7f7334 Compare October 21, 2025 14:32
@anonrig
Copy link
Member Author

anonrig commented Oct 21, 2025

cc @nodejs/tsc due to semver-major

@anonrig anonrig force-pushed the yagiz/reduce-encodeInto-size branch from b7f7334 to ef0b9c2 Compare October 21, 2025 14:36
@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.59%. Comparing base (94cbb77) to head (ef0b9c2).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60339      +/-   ##
==========================================
- Coverage   92.20%   88.59%   -3.62%     
==========================================
  Files         345      704     +359     
  Lines      138924   208473   +69549     
  Branches    22203    40066   +17863     
==========================================
+ Hits       128094   184691   +56597     
- Misses      10601    15798    +5197     
- Partials      229     7984    +7755     
Files with missing lines Coverage Δ
lib/internal/encoding.js 99.51% <100.00%> (+18.06%) ⬆️

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

@anonrig anonrig requested a review from a team October 21, 2025 18:48
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

should the same changes be made for TextDecoder?

@anonrig
Copy link
Member Author

anonrig commented Oct 21, 2025

should the same changes be made for TextDecoder?

Yes, I'll open a separate pull-request for TextDecoder.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2025
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM, I always feel that ERR_INVALID_THIS isn't a super useful specialized error as usually that just indicates that there's a bug in your program and it should be fixed; there isn't really much of a point to catch it and try again (how is one supposed to retry, anyway?)

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2025
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 21, 2025
@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 23, 2025
@nodejs-github-bot nodejs-github-bot merged commit ddbe136 into nodejs:main Oct 23, 2025
69 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ddbe136

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

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants