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 — src: add fast path to TextEncoder.encodeInto by anonrig · Pull Request #45701 · nodejs/node

Conversation

@anonrig

@anonrig anonrig commented Dec 1, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Original commit message:

[fastcall] Implement support for onebyte string arguments

Refs: v8/v8@bc831f8

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

                                                                                 confidence improvement accuracy (*)    (**)   (***)
util/text-encoder.js op='encodeInto' type='v8-one-byte-string' n=10000 len=0                     1.99 %       ±4.94%  ±6.58%  ±8.57%
util/text-encoder.js op='encodeInto' type='v8-one-byte-string' n=10000 len=1024         ***     13.53 %       ±6.84%  ±9.10% ±11.86%
util/text-encoder.js op='encodeInto' type='v8-one-byte-string' n=10000 len=256                   5.36 %       ±6.97%  ±9.29% ±12.12%
util/text-encoder.js op='encodeInto' type='v8-one-byte-string' n=10000 len=32768        ***     15.07 %       ±7.99% ±10.63% ±13.84%
util/text-encoder.js op='encodeInto' type='v8-two-byte-string' n=10000 len=0                    -5.38 %       ±6.23%  ±8.31% ±10.85%
util/text-encoder.js op='encodeInto' type='v8-two-byte-string' n=10000 len=1024                  1.45 %       ±6.77%  ±9.01% ±11.74%
util/text-encoder.js op='encodeInto' type='v8-two-byte-string' n=10000 len=256                  -2.36 %       ±7.55% ±10.05% ±13.08%
util/text-encoder.js op='encodeInto' type='v8-two-byte-string' n=10000 len=32768                -0.49 %       ±2.34%  ±3.11%  ±4.06%
util/text-encoder.js op='encode' type='v8-one-byte-string' n=10000 len=0                         1.62 %       ±6.95%  ±9.25% ±12.07%
util/text-encoder.js op='encode' type='v8-one-byte-string' n=10000 len=1024                     -2.32 %       ±5.28%  ±7.03%  ±9.17%
util/text-encoder.js op='encode' type='v8-one-byte-string' n=10000 len=256                      -4.58 %       ±6.07%  ±8.08% ±10.54%
util/text-encoder.js op='encode' type='v8-one-byte-string' n=10000 len=32768                     4.25 %      ±11.64% ±15.49% ±20.16%
util/text-encoder.js op='encode' type='v8-two-byte-string' n=10000 len=0                        -6.16 %       ±6.84%  ±9.11% ±11.86%
util/text-encoder.js op='encode' type='v8-two-byte-string' n=10000 len=1024                     -6.52 %       ±7.40%  ±9.85% ±12.83%
util/text-encoder.js op='encode' type='v8-two-byte-string' n=10000 len=256                      -6.35 %       ±6.74%  ±8.97% ±11.68%
util/text-encoder.js op='encode' type='v8-two-byte-string' n=10000 len=32768                    -5.08 %       ±7.21%  ±9.60% ±12.50%

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss, fannheyward, styfle, dritter, daeyeon, zyhou, sunderee, and jimmywarting reacted with thumbs up emoji daeyeon reacted with eyes emoji
All reactions
  • 👍 8 reactions
  • 👀 1 reaction
@anonrig
deps: V8: cherry-pick bc831f8ba33b
50fb246 
Original commit message:

    [fastcall] Implement support for onebyte string arguments

    This CL adds one byte string specialization support for fast API call arguments.
    It introduces a kOneByteString variant to CTypeInfo.

    We see a ~6x improvement in Deno's TextEncoder#encode microbenchmark.
    Rendered results: https://divy-v8-patches.deno.dev/

    Bug: chromium:1052746
    Change-Id: I47c3a9e101cd18ddc6ad58f627db3a34231b60f7
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4036884
    Reviewed-by: Toon Verwaest <[email protected]>
    Reviewed-by: Maya Lekova <[email protected]>
    Commit-Queue: Maya Lekova <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#84552}

Refs: v8/v8@bc831f8
@nodejs-github-bot

nodejs-github-bot commented Dec 1, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Dec 1, 2022
@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 Dec 1, 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 Dec 1, 2022
@nodejs-github-bot

nodejs-github-bot commented Dec 1, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot commented Dec 1, 2022

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5027/
All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot commented Dec 1, 2022

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5027/
All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot commented Dec 1, 2022

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5027/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos
targos previously requested changes Dec 1, 2022
View reviewed changes

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

I would be more inclined to accept this if the new capabilities were used by a subsequent commit. Otherwise the change doesn't bring anything to Node.js.

Sorry, something went wrong.

Uh oh!

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

anonrig commented Dec 1, 2022

Copy link
Copy Markdown
Member Author

I would be more inclined to accept this if the new capabilities were used by a subsequent commit. Otherwise the change doesn't bring anything to Node.js.

@targos Should I include my changes depending on this to this particular pull request, and add commit-queue-rebase?
All reactions

Sorry, something went wrong.

Uh oh!

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

targos commented Dec 1, 2022

Copy link
Copy Markdown
Member

That would be fine, yes.
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

ShogunPanda commented Dec 1, 2022

Copy link
Copy Markdown
Contributor

These changes look fine to me. I'm approving, but I agree on what @targos said.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ShogunPanda
ShogunPanda approved these changes Dec 1, 2022
View reviewed changes
@github-actions github-actions Bot mentioned this pull request Dec 2, 2022
CI Reliability 2022-12-02 nodejs/reliability#446
Open
10 tasks
@nodejs-github-bot

nodejs-github-bot commented Dec 2, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig changed the title deps: V8: cherry-pick bc831f8ba33b src: add fast path to TextEncoder Dec 2, 2022
@targos
targos dismissed their stale review December 2, 2022 16:10

resolved

This was referenced Dec 3, 2022
CI Reliability 2022-12-03 nodejs/reliability#447
Open
CI Reliability 2022-12-04 nodejs/reliability#448
Open
@anonrig anonrig added the review wanted PRs that need review. label Dec 4, 2022
anonrig
anonrig commented Dec 4, 2022
View reviewed changes
Comment thread src/node_encoding.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig commented Dec 4, 2022
View reviewed changes
Comment thread lib/internal/encoding.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@devsnek
devsnek self-requested a review December 4, 2022 01:25
@anonrig
anonrig requested a review from mscdex December 4, 2022 01:27
@anonrig anonrig changed the title src: add fast path to TextEncoder [WIP] src: add fast path to TextEncoder Dec 4, 2022
@anonrig
anonrig force-pushed the v8/fast-call branch from af9270a to 5c34041 Compare December 4, 2022 01:40
tniessen
tniessen requested changes Dec 4, 2022
View reviewed changes
Comment thread node.gyp Outdated

@tniessen tniessen Dec 4, 2022

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.

Nit: we already have so many files dealing with encodings, does this really need to be a new one (instead of, e.g., staying in node_buffer or maybe string_bytes)? And if so, is this the encoding file?

Sorry, something went wrong.

Uh oh!

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

@anonrig anonrig Dec 4, 2022

Copy link
Copy Markdown
Member Author

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.

I don’t have any specific thoughts on this. I’ll try to move the encode utf8 to the node_encoding once this is implementation improves the existing benchmark.

Sorry, something went wrong.

Uh oh!

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

@tniessen tniessen Dec 9, 2022

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.

I'm still not sure why this is a new file when it only covers a tiny part of all encoding-related routines.

Sorry, something went wrong.

Uh oh!

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

Uh oh!

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

nodejs-github-bot commented Dec 6, 2022

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5040/
All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot commented Dec 6, 2022

Copy link
Copy Markdown
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5040/
All reactions

Sorry, something went wrong.

Uh oh!

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

anonrig commented Dec 6, 2022

Copy link
Copy Markdown
Member Author

@tniessen Can you review this, since in your last review, you requested changes?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 6, 2022
@github-actions github-actions Bot mentioned this pull request Dec 7, 2022
CI Reliability 2022-12-07 nodejs/reliability#450
Open
10 tasks
targos
targos reviewed Dec 7, 2022
View reviewed changes
Comment thread lib/internal/encoding.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax requested changes Dec 7, 2022
View reviewed changes
Comment thread lib/internal/encoding.js Resolved

Uh oh!

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

Uh oh!

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

Uh oh!

There was an error while loading. Please reload this page.
@anonrig
anonrig force-pushed the v8/fast-call branch from 3642049 to bd013be Compare December 7, 2022 18:13
anonrig and others added 2 commits December 7, 2022 13:29
@anonrig @addaleax
src: add encoding_methods with fast api
7601758 
Co-authored-by: Anna Henningsen <[email protected]>
@anonrig
src: move decodeUtf8 to node_encoding
815fbee
@anonrig
anonrig force-pushed the v8/fast-call branch from bd013be to 815fbee Compare December 7, 2022 18:29
@anonrig
anonrig requested review from addaleax, jasnell and targos December 7, 2022 18:30
@github-actions github-actions Bot mentioned this pull request Dec 8, 2022
CI Reliability 2022-12-08 nodejs/reliability#451
Open
8 tasks
@anonrig anonrig mentioned this pull request Dec 8, 2022
benchmark: add variety of inputs to text-encoder #45787
Merged
@anonrig

anonrig commented Dec 8, 2022

Copy link
Copy Markdown
Member Author

@targos If it's ok, I recommend creating a pull request for merging the v8 cherry-pick, and continue this performance experiment in this pull request. There are several new areas (wasm for example) that can leverage this cherry-pick.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig mentioned this pull request Dec 8, 2022
deps: V8: cherry-pick bc831f8ba33b #45788
Merged
@github-actions github-actions Bot mentioned this pull request Dec 9, 2022
CI Reliability 2022-12-09 nodejs/reliability#452
Open
5 tasks
tniessen
tniessen reviewed Dec 9, 2022
View reviewed changes
Comment thread test/parallel/test-util-text-encoder.js
Comment on lines +14 to +15
// For loop is required to trigger the fast path for encodeInto
// Since v8 fast path is only triggered when v8 optimization starts.

@tniessen tniessen Dec 9, 2022

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.

There is no guarantee that V8 would take the fast path.

Suggested change
// For loop is required to trigger the fast path for encodeInto
// Since v8 fast path is only triggered when v8 optimization starts.
// Using a loop increases the chances of triggering the fast path for encodeInto
// because V8 heuristically optimizes based on information gathered at runtime.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread node.gyp Outdated

@tniessen tniessen Dec 9, 2022

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.

I'm still not sure why this is a new file when it only covers a tiny part of all encoding-related routines.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@github-actions github-actions Bot mentioned this pull request Dec 10, 2022
CI Reliability 2022-12-10 nodejs/reliability#453
Open
9 tasks
@aduh95 aduh95 removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 10, 2022
@aduh95

aduh95 commented Dec 10, 2022

Copy link
Copy Markdown
Contributor

There are git conflicts, no Jenkins CI run on the last commit, and two collaborators blocking the PR, removing author ready PRs with CI started, the required approvals, and no outstanding review comments. .
anonrig reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

anonrig commented Dec 10, 2022

Copy link
Copy Markdown
Member Author

Thanks, @aduh95. Since other pull requests are merged, I’ll update this pull request only focusing on encodeInto.
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 Dec 11, 2022
CI Reliability 2022-12-11 nodejs/reliability#454
Open
11 tasks
@anonrig anonrig removed the review wanted PRs that need review. label Dec 14, 2022
@anonrig anonrig mentioned this pull request Dec 21, 2022
deps: add simdutf dependency #45803
Closed
@jimmywarting jimmywarting mentioned this pull request Jan 27, 2023
ignore buffer in browsers kriszyp/cbor-x#67
Merged
@targos
targos removed their request for review March 10, 2023 15:44
@anonrig anonrig closed this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen requested changes
@mcollina mcollina mcollina approved these changes
@ShogunPanda ShogunPanda ShogunPanda approved these changes
@mscdex mscdex Awaiting requested review from mscdex
@devsnek devsnek Awaiting requested review from devsnek
@jasnell jasnell Awaiting requested review from jasnell
@addaleax addaleax Awaiting requested review from addaleax
+1 more reviewer
@targos targos targos left review comments
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

build Issues and PRs related to Node.js builds or CI infrastructure. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.

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.

10 participants

@anonrig @nodejs-github-bot @targos @ShogunPanda @tniessen @devsnek @aduh95 @mcollina @jasnell @addaleax
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.