GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — util: move validations for text decoder to c++ by anonrig · Pull Request #45388 · nodejs/node

Conversation

@anonrig

@anonrig anonrig commented Nov 9, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This is the 3rd pull request for my text-decoder performance patches. It removes any unnecessary check and moves them to the C++ side. I'll update the description when benchmark CI is complete.

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

                                                                                                 confidence improvement accuracy (*)    (**)   (***)
util/text-decoder.js type='ArrayBuffer' n=100 len=16384 ignoreBOM=0 encoding='iso-8859-3'                       -7.02 %       ±9.20% ±12.25% ±15.98%
util/text-decoder.js type='ArrayBuffer' n=100 len=16384 ignoreBOM=0 encoding='latin1'                           -8.56 %      ±10.35% ±13.81% ±18.03%
util/text-decoder.js type='ArrayBuffer' n=100 len=16384 ignoreBOM=0 encoding='utf-8'                            -3.94 %       ±7.71% ±10.27% ±13.38%
util/text-decoder.js type='ArrayBuffer' n=100 len=16384 ignoreBOM=1 encoding='iso-8859-3'                        2.52 %       ±9.46% ±12.59% ±16.40%
util/text-decoder.js type='ArrayBuffer' n=100 len=16384 ignoreBOM=1 encoding='latin1'                           -1.23 %       ±9.25% ±12.33% ±16.09%
util/text-decoder.js type='ArrayBuffer' n=100 len=16384 ignoreBOM=1 encoding='utf-8'                            -1.40 %       ±7.54% ±10.04% ±13.07%
util/text-decoder.js type='ArrayBuffer' n=100 len=256 ignoreBOM=0 encoding='iso-8859-3'                 ***     22.19 %      ±11.86% ±15.78% ±20.55%
util/text-decoder.js type='ArrayBuffer' n=100 len=256 ignoreBOM=0 encoding='latin1'                     ***     32.85 %      ±12.41% ±16.53% ±21.54%
util/text-decoder.js type='ArrayBuffer' n=100 len=256 ignoreBOM=0 encoding='utf-8'                       **     14.43 %       ±9.69% ±12.89% ±16.78%
util/text-decoder.js type='ArrayBuffer' n=100 len=256 ignoreBOM=1 encoding='iso-8859-3'                 ***     28.77 %      ±12.55% ±16.70% ±21.73%
util/text-decoder.js type='ArrayBuffer' n=100 len=256 ignoreBOM=1 encoding='latin1'                     ***     28.74 %      ±13.39% ±17.83% ±23.22%
util/text-decoder.js type='ArrayBuffer' n=100 len=256 ignoreBOM=1 encoding='utf-8'                      ***     16.03 %       ±8.30% ±11.05% ±14.42%
util/text-decoder.js type='ArrayBuffer' n=100 len=524288 ignoreBOM=0 encoding='iso-8859-3'                       7.19 %       ±7.88% ±10.48% ±13.64%
util/text-decoder.js type='ArrayBuffer' n=100 len=524288 ignoreBOM=0 encoding='latin1'                          -2.51 %       ±6.77%  ±9.01% ±11.72%
util/text-decoder.js type='ArrayBuffer' n=100 len=524288 ignoreBOM=0 encoding='utf-8'                            0.14 %       ±6.88%  ±9.16% ±11.92%
util/text-decoder.js type='ArrayBuffer' n=100 len=524288 ignoreBOM=1 encoding='iso-8859-3'                      -0.41 %       ±7.27%  ±9.67% ±12.59%
util/text-decoder.js type='ArrayBuffer' n=100 len=524288 ignoreBOM=1 encoding='latin1'                           1.48 %       ±7.13%  ±9.49% ±12.36%
util/text-decoder.js type='ArrayBuffer' n=100 len=524288 ignoreBOM=1 encoding='utf-8'                            0.62 %       ±6.23%  ±8.29% ±10.79%
util/text-decoder.js type='Buffer' n=100 len=16384 ignoreBOM=0 encoding='iso-8859-3'                            -2.13 %      ±11.79% ±15.68% ±20.41%
util/text-decoder.js type='Buffer' n=100 len=16384 ignoreBOM=0 encoding='latin1'                                -3.48 %      ±11.47% ±15.28% ±19.92%
util/text-decoder.js type='Buffer' n=100 len=16384 ignoreBOM=0 encoding='utf-8'                                 -7.71 %      ±13.21% ±17.59% ±22.93%
util/text-decoder.js type='Buffer' n=100 len=16384 ignoreBOM=1 encoding='iso-8859-3'                            -2.91 %      ±11.03% ±14.68% ±19.11%
util/text-decoder.js type='Buffer' n=100 len=16384 ignoreBOM=1 encoding='latin1'                                 1.95 %       ±9.36% ±12.46% ±16.24%
util/text-decoder.js type='Buffer' n=100 len=16384 ignoreBOM=1 encoding='utf-8'                                 -6.77 %      ±12.67% ±16.86% ±21.95%
util/text-decoder.js type='Buffer' n=100 len=256 ignoreBOM=0 encoding='iso-8859-3'                      ***     37.90 %      ±12.51% ±16.66% ±21.72%
util/text-decoder.js type='Buffer' n=100 len=256 ignoreBOM=0 encoding='latin1'                          ***     28.82 %      ±11.43% ±15.23% ±19.87%
util/text-decoder.js type='Buffer' n=100 len=256 ignoreBOM=0 encoding='utf-8'                           ***     24.74 %      ±13.10% ±17.54% ±23.06%
util/text-decoder.js type='Buffer' n=100 len=256 ignoreBOM=1 encoding='iso-8859-3'                      ***     31.08 %      ±11.10% ±14.81% ±19.34%
util/text-decoder.js type='Buffer' n=100 len=256 ignoreBOM=1 encoding='latin1'                          ***     18.27 %       ±9.55% ±12.71% ±16.54%
util/text-decoder.js type='Buffer' n=100 len=256 ignoreBOM=1 encoding='utf-8'                           ***     22.99 %      ±10.36% ±13.79% ±17.97%
util/text-decoder.js type='Buffer' n=100 len=524288 ignoreBOM=0 encoding='iso-8859-3'                           -0.68 %       ±5.90%  ±7.86% ±10.27%
util/text-decoder.js type='Buffer' n=100 len=524288 ignoreBOM=0 encoding='latin1'                                0.15 %       ±6.61%  ±8.79% ±11.44%
util/text-decoder.js type='Buffer' n=100 len=524288 ignoreBOM=0 encoding='utf-8'                                 1.44 %       ±5.09%  ±6.77%  ±8.82%
util/text-decoder.js type='Buffer' n=100 len=524288 ignoreBOM=1 encoding='iso-8859-3'                           -2.64 %       ±6.48%  ±8.62% ±11.23%
util/text-decoder.js type='Buffer' n=100 len=524288 ignoreBOM=1 encoding='latin1'                                3.62 %       ±6.72%  ±8.94% ±11.64%
util/text-decoder.js type='Buffer' n=100 len=524288 ignoreBOM=1 encoding='utf-8'                                 3.34 %       ±5.21%  ±6.93%  ±9.02%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=16384 ignoreBOM=0 encoding='iso-8859-3'                  3.80 %       ±9.18% ±12.21% ±15.89%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=16384 ignoreBOM=0 encoding='latin1'                     -1.58 %      ±10.41% ±13.87% ±18.07%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=16384 ignoreBOM=0 encoding='utf-8'                      -4.04 %       ±9.79% ±13.06% ±17.06%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=16384 ignoreBOM=1 encoding='iso-8859-3'                  5.56 %      ±10.50% ±14.01% ±18.31%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=16384 ignoreBOM=1 encoding='latin1'                      1.38 %       ±9.58% ±12.77% ±16.65%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=16384 ignoreBOM=1 encoding='utf-8'                       0.72 %       ±6.72%  ±8.95% ±11.68%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=256 ignoreBOM=0 encoding='iso-8859-3'           ***     16.70 %       ±9.00% ±11.97% ±15.59%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=256 ignoreBOM=0 encoding='latin1'               ***     22.84 %      ±10.16% ±13.53% ±17.65%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=256 ignoreBOM=0 encoding='utf-8'                 **     17.96 %      ±11.81% ±15.73% ±20.51%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=256 ignoreBOM=1 encoding='iso-8859-3'           ***     33.84 %      ±10.52% ±14.05% ±18.39%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=256 ignoreBOM=1 encoding='latin1'               ***     28.41 %      ±10.75% ±14.34% ±18.73%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=256 ignoreBOM=1 encoding='utf-8'                ***     24.81 %       ±6.80%  ±9.05% ±11.78%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=524288 ignoreBOM=0 encoding='iso-8859-3'                 1.99 %       ±6.46%  ±8.59% ±11.19%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=524288 ignoreBOM=0 encoding='latin1'                     0.89 %       ±6.93%  ±9.23% ±12.02%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=524288 ignoreBOM=0 encoding='utf-8'                     -0.38 %       ±7.79% ±10.36% ±13.49%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=524288 ignoreBOM=1 encoding='iso-8859-3'                 2.89 %       ±6.31%  ±8.40% ±10.94%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=524288 ignoreBOM=1 encoding='latin1'                     4.58 %       ±6.05%  ±8.05% ±10.47%
util/text-decoder.js type='SharedArrayBuffer' n=100 len=524288 ignoreBOM=1 encoding='utf-8'                     -2.54 %       ±7.23%  ±9.63% ±12.55%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 54 comparisons, you can thus
expect the following amount of false-positive results:
  2.70 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.54 false positives, when considering a   1% risk acceptance (**, ***),
  0.05 false positives, when considering a 0.1% risk acceptance (***)

CC @nodejs/performance @nodejs/util

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. i18n-api Issues and PRs related to Node.js internationalization support. needs-ci PRs that need a full CI run. labels Nov 9, 2022
@anonrig anonrig added the performance Issues and PRs related to the performance of Node.js. label Nov 9, 2022
@anonrig
anonrig force-pushed the perf/text-decoder-3 branch 2 times, most recently from 0437868 to 1bf201e Compare November 9, 2022 14:41
addaleax
addaleax approved these changes Nov 9, 2022
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just a heads up, while I personally really like this change and think it’s a good idea, this isn’t really in line with what the project has been doing in the past years in terms of input validation (which is to validate in JS and add CHECKs in C++).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/node_i18n.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
util: improve text decoder performance
c74850f
@anonrig
anonrig force-pushed the perf/text-decoder-3 branch from 1bf201e to c74850f Compare November 9, 2022 14:48
santigimeno
santigimeno approved these changes Nov 9, 2022
View reviewed changes
@anonrig anonrig 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 Nov 9, 2022
@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 Nov 9, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47809/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tony-go
tony-go approved these changes Nov 9, 2022
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Nov 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47812/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Nov 9, 2022
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Nov 9, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47816/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Nov 10, 2022
CI Reliability 2022-11-10 nodejs/reliability#424
Open
20 tasks
@nodejs-github-bot

nodejs-github-bot commented Nov 10, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47846/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Nov 11, 2022
CI Reliability 2022-11-11 nodejs/reliability#425
Open
19 tasks
@nodejs-github-bot

nodejs-github-bot commented Nov 11, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47866/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Nov 11, 2022

Copy link
Copy Markdown
Member Author

Landed in d6c10e1
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig closed this Nov 11, 2022
anonrig added a commit that referenced this pull request Nov 11, 2022
@anonrig
util: improve text decoder performance
2d76238 
PR-URL: #45388
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@github-actions github-actions Bot mentioned this pull request Nov 12, 2022
CI Reliability 2022-11-12 nodejs/reliability#426
Open
19 tasks
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 12, 2022
@anonrig @Trott
util: improve text decoder performance
ca3ed36 
PR-URL: nodejs#45388
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott

Trott commented Nov 12, 2022

Copy link
Copy Markdown
Member

Re-landed in ca3ed36
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Nov 13, 2022
CI Reliability 2022-11-13 nodejs/reliability#427
Open
CI Reliability 2022-11-14 nodejs/reliability#428
Open
This was referenced Nov 15, 2022
CI Reliability 2022-11-15 nodejs/reliability#429
Open
CI Reliability 2022-11-16 nodejs/reliability#430
Open
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
@anonrig @ruyadorno
util: improve text decoder performance
f86f90f 
PR-URL: #45388
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
v19.2.0 proposal #45615
Merged
@anonrig anonrig mentioned this pull request Nov 27, 2022
[v18.x backport] backport utf8 performance improvements #45650
Closed
anonrig added a commit to anonrig/node that referenced this pull request Nov 28, 2022
@anonrig
util: improve text decoder performance
8dea4eb 
PR-URL: nodejs#45388
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@anonrig anonrig added lts-watch-v14.x labels Nov 28, 2022
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@anonrig @danielleadams
util: improve text decoder performance
a771d77 
PR-URL: #45388
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@anonrig @danielleadams
util: improve text decoder performance
cd740d0 
PR-URL: #45388
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@richardlau richardlau removed the lts-watch-v14.x label Jan 27, 2023
@richardlau richardlau removed the lts-watch-v16.x label Aug 24, 2023
@targos targos added backported-to-v18.x and removed lts-watch-v18.x labels Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@addaleax addaleax addaleax approved these changes
@santigimeno santigimeno santigimeno approved these changes
+1 more reviewer
@tony-go tony-go tony-go approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. i18n-api Issues and PRs related to Node.js internationalization support. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@anonrig @nodejs-github-bot @Trott @mcollina @addaleax @santigimeno @tony-go @targos @richardlau
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.