src: add fast path to TextEncoder.encodeInto#45701
src: add fast path to TextEncoder.encodeInto#45701anonrig wants to merge 3 commits intonodejs:mainfrom
Conversation
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 <verwaest@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84552}
Refs: v8/v8@bc831f8
|
Review requested:
|
nodejs-github-bot
added
build
github-actions
bot
removed
the
request-ci
targos
left a comment
targos
left a comment
There was a problem hiding this comment.
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 |
|
That would be fine, yes. |
|
These changes look fine to me. I'm approving, but I agree on what @targos said. |
anonrig
changed the title
anonrig
changed the title
| 'src/node_contextify.cc', | ||
| 'src/node_credentials.cc', | ||
| 'src/node_dir.cc', | ||
| 'src/node_encoding.cc', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm still not sure why this is a new file when it only covers a tiny part of all encoding-related routines.
|
@tniessen Can you review this, since in your last review, you |
anonrig
added
the
author ready
Co-authored-by: Anna Henningsen <anna@addaleax.net>
|
@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. |
| // For loop is required to trigger the fast path for encodeInto | ||
| // Since v8 fast path is only triggered when v8 optimization starts. |
There was a problem hiding this comment.
There is no guarantee that V8 would take the fast path.
| // 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. |
| 'src/node_contextify.cc', | ||
| 'src/node_credentials.cc', | ||
| 'src/node_dir.cc', | ||
| 'src/node_encoding.cc', |
There was a problem hiding this comment.
I'm still not sure why this is a new file when it only covers a tiny part of all encoding-related routines.
aduh95
removed
the
author ready
|
There are git conflicts, no Jenkins CI run on the last commit, and two collaborators blocking the PR, removing
author ready
|
|
Thanks, @aduh95. Since other pull requests are merged, I’ll update this pull request only focusing on encodeInto. |
10 participants
Original commit message:
Refs: v8/v8@bc831f8
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1259/