src: improve TextEncoder encodeInto performance

[anonrig]

Improves the performance of TextEncoder.encodeInto method

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

It seems none of the benchmarks are triggering the fast path

                                                                               confidence improvement accuracy (*)   (**)  (***)
util/text-encoder.js op='encode' type='ascii' n=1000000 len=1024                              -0.77 %       ±1.08% ±1.44% ±1.87%
util/text-encoder.js op='encode' type='ascii' n=1000000 len=256                                0.70 %       ±0.97% ±1.29% ±1.67%
util/text-encoder.js op='encode' type='ascii' n=1000000 len=32                                -0.34 %       ±1.12% ±1.50% ±1.95%
util/text-encoder.js op='encode' type='ascii' n=1000000 len=8192                               0.08 %       ±0.09% ±0.12% ±0.15%
util/text-encoder.js op='encode' type='one-byte-string' n=1000000 len=1024                     0.36 %       ±0.61% ±0.81% ±1.05%
util/text-encoder.js op='encode' type='one-byte-string' n=1000000 len=256                      0.29 %       ±0.64% ±0.85% ±1.10%
util/text-encoder.js op='encode' type='one-byte-string' n=1000000 len=32                       0.48 %       ±0.95% ±1.27% ±1.65%
util/text-encoder.js op='encode' type='one-byte-string' n=1000000 len=8192                    -0.02 %       ±0.05% ±0.06% ±0.08%
util/text-encoder.js op='encode' type='two-byte-string' n=1000000 len=1024             **      0.26 %       ±0.15% ±0.20% ±0.26%
util/text-encoder.js op='encode' type='two-byte-string' n=1000000 len=256              **     -0.73 %       ±0.45% ±0.60% ±0.79%
util/text-encoder.js op='encode' type='two-byte-string' n=1000000 len=32                       0.02 %       ±0.84% ±1.11% ±1.45%
util/text-encoder.js op='encode' type='two-byte-string' n=1000000 len=8192            ***     -0.15 %       ±0.02% ±0.03% ±0.04%
util/text-encoder.js op='encodeInto' type='ascii' n=1000000 len=1024                  ***     -0.64 %       ±0.21% ±0.29% ±0.37%
util/text-encoder.js op='encodeInto' type='ascii' n=1000000 len=256                   ***     -2.94 %       ±0.56% ±0.75% ±0.99%
util/text-encoder.js op='encodeInto' type='ascii' n=1000000 len=32                    ***     -8.50 %       ±2.68% ±3.57% ±4.64%
util/text-encoder.js op='encodeInto' type='ascii' n=1000000 len=8192                  ***     -0.05 %       ±0.02% ±0.02% ±0.03%
util/text-encoder.js op='encodeInto' type='one-byte-string' n=1000000 len=1024        ***     -0.99 %       ±0.22% ±0.29% ±0.38%
util/text-encoder.js op='encodeInto' type='one-byte-string' n=1000000 len=256         ***     -1.47 %       ±0.81% ±1.08% ±1.41%
util/text-encoder.js op='encodeInto' type='one-byte-string' n=1000000 len=32            *     -2.82 %       ±2.60% ±3.46% ±4.50%
util/text-encoder.js op='encodeInto' type='one-byte-string' n=1000000 len=8192          *     -0.07 %       ±0.06% ±0.08% ±0.10%
util/text-encoder.js op='encodeInto' type='two-byte-string' n=1000000 len=1024        ***     -0.67 %       ±0.13% ±0.18% ±0.23%
util/text-encoder.js op='encodeInto' type='two-byte-string' n=1000000 len=256         ***     -1.69 %       ±0.42% ±0.56% ±0.73%
util/text-encoder.js op='encodeInto' type='two-byte-string' n=1000000 len=32           **     -2.70 %       ±1.93% ±2.57% ±3.35%
util/text-encoder.js op='encodeInto' type='two-byte-string' n=1000000 len=8192        ***      0.11 %       ±0.05% ±0.06% ±0.08%

[codecov]

Codecov Report

Attention: Patch coverage is 17.39130% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.16%. Comparing base (723d7bb) to head (06de6fa).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/encoding_binding.cc	17.39%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58080      +/-   ##
==========================================
- Coverage   90.17%   90.16%   -0.02%     
==========================================
  Files         630      630              
  Lines      186473   186495      +22     
  Branches    36613    36615       +2     
==========================================
- Hits       168160   168157       -3     
- Misses      11128    11139      +11     
- Partials     7185     7199      +14     

Files with missing lines	Coverage Δ
src/encoding_binding.h	100.00% <ø> (ø)
src/node_external_reference.h	100.00% <ø> (ø)
src/encoding_binding.cc	72.36% <17.39%> (-7.30%)	⬇️

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

[BridgeAR]

@anonrig even if the benchmark is not hitting the fast path: the benchmarks currently show a regression. I am surprised that is the case. Is there a possibility to prevent that for non-optimized code?

[anonrig]

@anonrig even if the benchmark is not hitting the fast path: the benchmarks currently show a regression. I am surprised that is the case. Is there a possibility to prevent that for non-optimized code?

I think that's due to unreliable benchmarks, I'll re-run the benchmarks to see if they persist.

Edit 2: New benchmarks: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1712/

[anonrig]

It seems it still degrades performance. Opened an issue on v8-dev https://groups.google.com/g/v8-dev/c/x2Fs6do0jDA

[BridgeAR]

These changes seem to regress the performance (see both benchmark runs), not improve it. I could imagine that there is overhead involved with gathering the data during the runs.

[anonrig]

cc @erikcorry

[gahaas]

It makes sense to me that the "fast API" is slower in this benchmark than regular API calls. "fast API" is mostly a marketing name, a more telling name would be something like "unboxed API", because primitive values get passed unboxed, i.e. not boxed in a HeapNumber. If there are no parameters of primitive type, there is no reason why the "fast API" should be faster.

If I understood the benchmark correctly, then you even open a HandleScope in the API function. In the case of regular API calls, the HandleScope already gets opened automatically by the API call, which is faster.

Therefore, there is no reason why the fast API would be faster in this example, and there is a reason why regular API calls would be faster, so in total the result is not surprising.

[anonrig]

If there are no parameters of primitive type, there is no reason why the "fast API" should be faster.

I wonder: what is the benefit of adding Local as a parameter? Only for DX?

In the case of regular API calls, the HandleScope already gets opened automatically by the API call, which is faster.

@gahaas Where is it getting called automatically? Would you mind sharing the code?

[gahaas]

If there are no parameters of primitive type, there is no reason why the "fast API" should be faster.

I wonder: what is the benefit of adding Local as a parameter? Only for DX?

There are API functions that have parameters of primitive type and reference type. i.e. web gpu APIs that take a TypedArray and Number parameters. Without supporting Local as a parameter it would not be possible to use the fast API there.

In the case of regular API calls, the HandleScope already gets opened automatically by the API call, which is faster.

@gahaas Where is it getting called automatically? Would you mind sharing the code?

Opening the HandleScope starts around here in the macro assembler.