tools: refactor js2c.cc to use c++20

[anonrig]

Refactors js2c.cc to use modern syntax.

[lemire]

@anonrig Let me check.

[lemire]

@anonrig Please see anonrig#14

[anonrig]

@anonrig Please see anonrig#14

amazing work as always @lemire

[jasnell]

Generally LGTM with nits.

[lemire]

@anonrig I did not lint my changes. :-)

[anonrig]

@lemire it seems windows build is failing. any idea?

[jasnell]

D:\a\node\node\out\Release\obj\global_intermediate\node_javascript.cc(12,1): error C1064: compiler limit: token overflowed internal buffer [D:\a\node\node\libnode.vcxproj]
  (compiling source file '/out/Release//obj/global_intermediate/node_javascript.cc')

"An identifier exceeds the length of the internal buffer used for identifiers. Shorten the name."

That's a new one for me. I don't know if this is actually the failure but spotted it in the build log.

[lemire]

@jasnell @anonrig

"An identifier exceeds the length of the internal buffer used for identifiers. Shorten the name."

I am not sure.

[lemire]

@anonrig @jasnell

I did not try with Visual Studio (yet), but here is the node_javascript.cc file generated on my mac with this PR:

https://gist.github.com/lemire/a8544683cb6fba911a86843cff048e0d

By the look of it, we have a string... a gigantic string spanning thousands of lines. I think that's what Visual Studio does not like, but how (or whether) it relates to this PR, I don't know.

[lemire]

@anonrig @jasnell

If one of you has quick access to Windows + Node, building this PR and checking what the node_javascript.cc file looks like might help.

It could possibly be an encoding problem... or anything really. Hard to tell. :-/

(I have my Windows laptop, but I have to leave just now.)

[lemire]

Ok. I think I figured it out.

[lemire]

Please don't merge this yet, I think we can make the table faster.

[lemire]

Nevermind, it is fine.

[anonrig]

@joyeecheung any concerns with landing this?

[joyeecheung]

The starts_with/ends_with changes LGTM. For the code table changes, I think it's a bit over-engineering to make the otherwise straigt-forward code this convoluted to save ~40MB for a build that already takes more than 4GB of memory to work (some of the digital oceans machines with 4GB memory and no extra swap space failed a few weeks ago because of this - which happened during building V8 and would not be in parallel to js2c), or to shave off a few ms from a build that takes tens of minutes/hours. But it's not that convoluted currently for me so I am fine with it (#54849 (comment) on the other hand, is definitely over-engineering this IMO).

I would not block it but I don't feel like approving the code-table part either though because I don't see it as an improvement - If it was another way around, say the code was written like this originally and someone sent a PR to simplify it to the current GetCodeTable(), then I would've approved that because I think that would be a readability improvement with a negligible cost for what the build already takes.

[lemire]

@joyeecheung @anonrig

Just to clarify... the current change with respect to the table is optimized for speed, not memory usage.

If we want to reduce memory usage, we can drop the table and call std::to_chars.

I expect that compared with @joyeecheung's port of this code from JavaScript to C++, this PR won't move the needle performance wise. But it should be a tiny bit faster!!!

💨

[joyeecheung]

FWIW the primary motivation of the original port (from Python, not JavaScript) was to experiment with compression options other than zlib (zlib had been shown to be too slow in decompression that we reverted snapshot compression to reverse a runtime performance regression). The performance aspect was only a nice-to-have (also here C++ is at least easier to get reviews from compared to Python).

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/54849
✔  Done loading data for nodejs/node/pull/54849
----------------------------------- PR info ------------------------------------
Title      tools: refactor js2c.cc to use c++20 (#54849)
Author     Yagiz Nizipli <[email protected]> (@anonrig)
Branch     anonrig:tools-simplify -> nodejs:main
Labels     c++, tools, commit-queue-squash, dont-land-on-v18.x, dont-land-on-v20.x
Commits    12
 - tools: refactor js2c.cc to use c++20
 - reducing memory usage and making GetCode's buffer potentially evaluat…
 - Update js2c.cc
 - Update js2c.cc
 - lint
 - minor fix
 - Update tools/js2c.cc
 - removing constexpr
 - lint
 - Update tools/js2c.cc
 - Update tools/js2c.cc
 - Update tools/js2c.cc
Committers 3
 - Yagiz Nizipli <[email protected]>
 - GitHub <[email protected]>
 - Daniel Lemire <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54849
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Lemire <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54849
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Lemire <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - Update tools/js2c.cc
   ⚠  - Update tools/js2c.cc
   ⚠  - Update tools/js2c.cc
   ℹ  This PR was created on Sun, 08 Sep 2024 16:08:34 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54849#pullrequestreview-2288589793
   ✔  - Daniel Lemire (@lemire): https://github.com/nodejs/node/pull/54849#pullrequestreview-2297353318
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10894139953

[nodejs-github-bot]

Landed in cde6dcc