url: reduce deplicated codes in autoEscapeStr

[starkwang]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

url

[starkwang]

CI: https://ci.nodejs.org/job/node-test-pull-request/12990/

[BridgeAR]

@starkwang I guess it might have a minor performance penalty but without having a significant impact. Would you be so kind and provide some benchmarks for it nevertheless? :-)

[starkwang]

@BridgeAR Pushed commit to fix performance regression. Here is the benchmark:

                                             confidence improvement accuracy (*)    (**)   (***)
 url/url-parse-.js n=1000000 type="escaped"          *     -4.06 %      ±3.29% ±4.38% ±5.70%
 url/url-parse-.js n=1000000 type="normal"           *      2.04 %      ±1.64% ±2.18% ±2.83%

[starkwang]

New CI: https://ci.nodejs.org/job/node-test-pull-request/12993/

[joyeecheung]

Isn't the whole point of having these switch statements to avoid the overhead of accessing a Map/object? (although that looks quite Crankshaftscript-ish so I doubt if the comment here still holds)...if we are going to access an object anyway, then the switch statements can just be replaced with something like const code = escapedCharacterCodes[rest.charCodeAt(i)]; if (code)..., and this comment can be removed.

[starkwang]

That's exactly what I do in the first commit, but it causes ~10% performance regression in url.parse.

[joyeecheung]

@bmeurer Would you be interested in taking a look at this?

[bmeurer]

Intuitively I'd suggest to go with an array instead of an object literal. Did you try with an array?

[joyeecheung]

Benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/117/
Ah, right, the benchmark CI cannot compare new benchmarks..

[starkwang]

@bmeurer @joyeecheung I tried to use array and found it was faster. Here is the benchmark with array:

                                                confidence improvement accuracy (*)    (**)   (***)
 url/url-parse.js n=10000000 type="escaped"          *     -2.80 %      ±2.67% ±3.55% ±4.63%
 url/url-parse.js n=10000000 type="normal"         ***      1.94 %      ±0.77% ±1.03% ±1.36%

[BridgeAR]

Is a holey array as fast as a dense array? I expect it would be better to add empty entries instead of having holes?

[starkwang]

It seems that dense array is still faster than holey array: https://jsperf.com/packed-vs-holey-arrays

But the comparison above is between the array case and the original switch case.

[BridgeAR]

Would you be so kind and switch to the dense array in that case? :-)

I guess we do not need a new benchmark in that case even though it would of course still be nice to have the results with those.

[starkwang]

Sorry I don't fully understand. The code of chars in rest is not dense, so the array used to map them is not dense as well. Changing it to a dense array may cost a lot.

[BridgeAR]

What I meant is to use it like e.g. util.js.

[starkwang]

CI before land: https://ci.nodejs.org/job/node-test-pull-request/13327/

[starkwang]

Landed in 3cef3e6