querystring: reduce memory usage

[sapics]

This PR will reduce the memory usage when program loads querystring or internal/querystring. (reduce about 8 Byte * (256 - 103) count = 1,224 Byte, for each)
It is also possible to remove isHexTable by replacing if(isHexTable[code]) to if(unhexTable[code]>=0), which reduce 8 * 103 Byte. Currently, I did not replace that for simplicity. If you feel better to replace it, please notice me.

To avoid number->boolean casting, I did not do following replacement as #34179 (comment)
I also change the conditional expression as

- if (isHexTable[code] === 1) {
+ if (isHexTable[code]) {

because I feel that the name of variable isHexTable looks boolean name.

Checklist

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

[benjamingr]

Hey, thanks for your contribution 🙏

What's the impact on performance?

(you can run the querystring benchmark :])

[sapics]

Hello, thanks for your suggestion 🙏

 querystring/querystring-parse.js n=1000000 type='altspaces'                                                                                   **     -5.77 %      ±4.13% ±5.55% ±7.36%
 querystring/querystring-parse.js n=1000000 type='encodefake'                                                                                 ***      7.28 %      ±1.14% ±1.52% ±1.98%
 querystring/querystring-parse.js n=1000000 type='encodelast'                                                                                          0.40 %      ±0.67% ±0.89% ±1.17%
 querystring/querystring-parse.js n=1000000 type='encodemany'                                                                                          0.56 %      ±0.57% ±0.76% ±0.99%
 querystring/querystring-parse.js n=1000000 type='manyblankpairs'                                                                                     -0.06 %      ±0.68% ±0.90% ±1.18%
 querystring/querystring-parse.js n=1000000 type='manypairs'                                                                                    *     -1.09 %      ±0.90% ±1.20% ±1.56%
 querystring/querystring-parse.js n=1000000 type='multicharsep'                                                                                        0.42 %      ±1.63% ±2.18% ±2.84%
 querystring/querystring-parse.js n=1000000 type='multivalue'                                                                                         -0.43 %      ±0.85% ±1.14% ±1.48%
 querystring/querystring-parse.js n=1000000 type='multivaluemany'                                                                                     -0.50 %      ±1.22% ±1.62% ±2.12%
 querystring/querystring-parse.js n=1000000 type='noencode'                                                                                            0.31 %      ±0.96% ±1.28% ±1.67%
 querystring/querystring-stringify.js n=1000000 type='array'                                                                                          -1.62 %      ±2.49% ±3.33% ±4.38%
 querystring/querystring-stringify.js n=1000000 type='encodelast'                                                                                     -0.17 %      ±0.68% ±0.90% ±1.17%
 querystring/querystring-stringify.js n=1000000 type='encodemany'                                                                                     -0.35 %      ±1.29% ±1.72% ±2.24%
 querystring/querystring-stringify.js n=1000000 type='multiprimitives'                                                                                -0.28 %      ±0.97% ±1.29% ±1.68%
 querystring/querystring-stringify.js n=1000000 type='noencode'                                                                                       -1.07 %      ±2.98% ±4.00% ±5.28%
 querystring/querystring-unescapebuffer.js n=10000000 input='%20%21%22%23%24%25%26%27%28%29%2A%2B%2C%2D%2E%2F%30%31%32%33%34%35%36%37'                 0.76 %      ±1.38% ±1.86% ±2.45%
 querystring/querystring-unescapebuffer.js n=10000000 input='there is nothing to unescape here'                                                        0.36 %      ±0.64% ±0.85% ±1.11%
 querystring/querystring-unescapebuffer.js n=10000000 input='there%20are%20several%20spaces%20that%20need%20to%20be%20unescaped'                       0.26 %      ±0.52% ±0.69% ±0.90%
 querystring/querystring-unescapebuffer.js n=10000000 input='there%2Qare%0-fake%escaped values in%%%%this%9Hstring'                           ***     -1.52 %      ±0.38% ±0.51% ±0.66%

From the benchmark in linux, it looks almost same performance 😄
This commit is intended to reduce some memory usage at initialization, thus, it might be small impact to performance ;)
Do you feel something that could have a negative impact on performance, in my commit?

[bnoordhuis]

Do you feel something that could have a negative impact on performance, in my commit?

If I read your changes right, it's now doing out-of-bounds array element reads and V8 used to penalize those. Maybe it no longer does or maybe it's insignificant here.

[benjamingr]

Maybe it no longer does or maybe it's insignificant here.

It looks pretty significant so I am -0 on landing this.

Interesting change + results :]

[sapics]

@bnoordhuis

If I read your changes right, it's now doing out-of-bounds array element reads

Yes, I did. And I used a lot in my personal coding :)

V8 used to penalize those.

Thanks for letting me know! I did not know that!

@benjamingr

It looks pretty significant so I am -0 on landing this.
Interesting change + results :]

I agree with you. I do not want to land unrecommended one, too :)
So, I close this pr.

Again, thank you both for advice!

[addaleax]

@sapics One idea that might be worth exploring here is using an Int8Array for these tables instead. I would assume that should reduce memory overhead by a factor of 4 (32-bit integer → 8-bit integer), without affecting performance negatively.

[sapics]

Thanks @addaleax for your suggestion!

The suggestion looks much better! I fixed my commit, force-push it and reopen this PR.

Now, total memory usage would decrease (64 - 8) * (256 * 2 + 128 * 3) / 8 = 6272 Bytes (about 6KB!).
(I thought Number is 64 bits in js, 32 bits by V8 optimization? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number)

Furthermore, we can get better benchmark ;)

                                                                                                                                        confidence improvement accuracy (*)    (**)   (***)
 querystring\\querystring-parse.js n=1000000 type='altspaces'                                                                                           0.22 %      ±1.64% ±2.19% ±2.86%
 querystring\\querystring-parse.js n=1000000 type='encodefake'                                                                                 ***      4.00 %      ±1.11% ±1.48% ±1.92%
 querystring\\querystring-parse.js n=1000000 type='encodelast'                                                                                 ***      2.22 %      ±1.14% ±1.52% ±1.99%
 querystring\\querystring-parse.js n=1000000 type='encodemany'                                                                                          0.27 %      ±0.94% ±1.25% ±1.63%
 querystring\\querystring-parse.js n=1000000 type='manyblankpairs'                                                                                     -0.08 %      ±0.30% ±0.40% ±0.52%
 querystring\\querystring-parse.js n=1000000 type='manypairs'                                                                                          -0.56 %      ±1.18% ±1.57% ±2.05%
 querystring\\querystring-parse.js n=1000000 type='multicharsep'                                                                                **     -2.18 %      ±1.33% ±1.78% ±2.33%
 querystring\\querystring-parse.js n=1000000 type='multivalue'                                                                                          0.22 %      ±1.17% ±1.55% ±2.03%
 querystring\\querystring-parse.js n=1000000 type='multivaluemany'                                                                              **      1.46 %      ±0.87% ±1.16% ±1.51%
 querystring\\querystring-parse.js n=1000000 type='noencode'                                                                                            0.39 %      ±1.03% ±1.37% ±1.78%
 querystring\\querystring-stringify.js n=1000000 type='array'                                                                                   **      3.65 %      ±2.69% ±3.59% ±4.70%
 querystring\\querystring-stringify.js n=1000000 type='encodelast'                                                                               *      1.51 %      ±1.47% ±1.97% ±2.59%
 querystring\\querystring-stringify.js n=1000000 type='encodemany'                                                                                      0.68 %      ±0.99% ±1.32% ±1.72%
 querystring\\querystring-stringify.js n=1000000 type='multiprimitives'                                                                                -2.49 %      ±4.81% ±6.47% ±8.57%
 querystring\\querystring-stringify.js n=1000000 type='noencode'                                                                                        0.69 %      ±1.79% ±2.38% ±3.11%
 querystring\\querystring-unescapebuffer.js n=10000000 input='%20%21%22%23%24%25%26%27%28%29%2A%2B%2C%2D%2E%2F%30%31%32%33%34%35%36%37'        ***      4.58 %      ±0.79% ±1.06% ±1.40%
 querystring\\querystring-unescapebuffer.js n=10000000 input='there is nothing to unescape here'                                               ***     -1.35 %      ±0.75% ±1.00% ±1.31%
 querystring\\querystring-unescapebuffer.js n=10000000 input='there%20are%20several%20spaces%20that%20need%20to%20be%20unescaped'              ***      5.65 %      ±0.65% ±0.86% ±1.12%
 querystring\\querystring-unescapebuffer.js n=10000000 input='there%2Qare%0-fake%escaped values in%%%%this%9Hstring'                             *      0.33 %      ±0.33% ±0.44% ±0.58%

[addaleax]

(I thought Number is 64 bits in js, 32 bits by V8 optimization? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number)

Yes, “small integers” (signed integers up to 31 bits in length, aka “smi”s) take up only a single 32-bit value, and V8 does have a special array representation for smi arrays, as far as I remember.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/32219/
Benchmarks: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/638/

[nodejs-github-bot]

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

[github-actions]

Commit Queue failed

- Loading data for nodejs/node/pull/34179
✔  Done loading data for nodejs/node/pull/34179
----------------------------------- PR info ------------------------------------
Title      querystring: reduce memory usage (#34179)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     sapics:querystring/reduce-memory-usage -> nodejs:master
Labels     author ready, querystring
Commits    1
 - querystring: reduce memory usage by Int8Array
Committers 1
 - sapics 
PR-URL: https://github.com/nodejs/node/pull/34179
Reviewed-By: Anna Henningsen 
Reviewed-By: Denys Otrishko 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34179
Reviewed-By: Anna Henningsen 
Reviewed-By: Denys Otrishko 
--------------------------------------------------------------------------------
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-11-08T12:24:00Z: https://ci.nodejs.org/job/node-test-pull-request/34198/
   ℹ  Last Benchmark CI on 2020-07-05T13:02:13Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/638/
- Querying data for job/node-test-pull-request/34198/
✔  Build data downloaded
- Querying failures of job/node-test-commit/42045/
✔  Data downloaded
   ✖  4 failure(s) on the last Jenkins CI run
   ℹ  This PR was created on Fri, 03 Jul 2020 06:36:29 GMT
   ✔  Approvals: 2
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/34179#pullrequestreview-442670690
   ✔  - Denys Otrishko (@lundibundi): https://github.com/nodejs/node/pull/34179#pullrequestreview-442675347
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/352514303

[nodejs-github-bot]

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

[aduh95]

@sapics Can you fix the linter issue please? You need to declare const { Int8Array } = primordials;.

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[sapics]

@aduh95 Thank you for noticing linter issue! I fix it and push the commit.

[nodejs-github-bot]

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

[github-actions]

Landed in 0dee498...00b5ee6