loader: speed up line length calculation used by moduleProvider #50969

zeusdeux · 2023-11-29T22:02:05Z

When using a loader, for e.g., say TypeScript, the esm loader invokes the lineLengths function via maybeCacheSourceMap when sourcemaps are enabled. Therefore, lineLengths ends up getting called quite often when running large servers written in TypeScript for example. Making lineLengths faster should therefore speed up server startup times for anyone using a loader with node with sourcemaps enabled.

The change itself is fairly simple and is all about removing creation of unnecessary memory and iterating the whole source content only once with the hope of making the function cache friendly.

make -j4 test passes

Alongside that, below are the cpuprofiles of node 20.9.0 vs node main with this patch taken during the startup of large server (from work) written in TypeScript loaded using esno (a proxy to esbuild-register) showing a ~98% reduction in time taken by moduleProvider.

node 20.9.0

node `main` + patch from this PR

zeusdeux · 2023-11-29T22:42:07Z

Pinging the modules team since the loaders WG readme doesn't mention who is in it (apologies if you aren't working on loaders but got pinged here 🙏🏽 ).
@bmeck, @DanielRosenwasser, @devsnek, @GeoffreyBooth, @giltayar, @guybedford, @JakobJingleheimer, @jkrems, @ljharb, @MylesBorins, @qballer, @robpalme, @targos, @VoltrexKeyva, @WebReflection, @weswigham, @zackschuster

GeoffreyBooth · 2023-11-29T22:43:53Z

Next time please just ping @nodejs/loaders. cc @nodejs/performance

zeusdeux · 2023-11-29T22:48:24Z

Ah so those teams do exist! I assumed they didn't as the autocomplete here didn't show them. 🤦🏽‍♂️
Thanks for the pointer @GeoffreyBooth 🙏🏽

lib/internal/source_map/source_map_cache.js

ljharb · 2023-11-29T22:52:14Z

lib/internal/source_map/source_map_cache.js

+    // account in coverage calculations.
+    // codepoints for \n, \u2028 and \u2029
+    if (codePoint === 10 || codePoint === 0x2028 || codePoint === 0x2029) {
+      output.push(lineLength);


Suggested change

output.push(lineLength);

ArrayPrototypePush(output, lineLength);

ljharb · 2023-11-29T22:52:22Z

lib/internal/source_map/source_map_cache.js

+      lineLength = -1; // To not count the matched codePoint such as \n character
+    }
+  }
+  output.push(lineLength);


Suggested change

output.push(lineLength);

ArrayPrototypePush(output, lineLength);

ArrayPrototypePush has performance implications according to primordials.md. Since adding primordials is optional, I'm strongly against adding it for array.prototype.push in this PR.

I think for source map support, we don't want user code to be able to mess with internal implementation – i.e. we should take the potential performance penalty and generate reliably correct stack traces. Also, we should measure the perf difference, as it might be negligable.

alternatively, output[output.length] = lineLength?

nodejs-github-bot · 2023-11-29T22:54:10Z

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

lib/internal/source_map/source_map_cache.js

GeoffreyBooth · 2023-11-29T22:52:19Z

lib/internal/source_map/source_map_cache.js

+      lineLength = -1; // To not count the matched codePoint such as \n character
+    }
+  }
+  output.push(lineLength);


Suggested change

output.push(lineLength);

ArrayPrototypePush(output, lineLength);

lib/internal/source_map/source_map_cache.js

aduh95 · 2023-11-29T23:05:33Z

lib/internal/source_map/source_map_cache.js

+    // account in coverage calculations.
+    // codepoints for \n (new line), \u2028 (line separator) and \u2029 (paragraph separator)
+    if (codePoint === 10 || codePoint === 0x2028 || codePoint === 0x2029) {
+      output.push(lineLength)


Suggested change

output.push(lineLength)

ArrayPrototypePush(output, lineLength);

zeusdeux · 2023-11-29T23:05:56Z

lib/internal/source_map/source_map_cache.js

+    // account in coverage calculations.
+    // codepoints for \n (new line), \u2028 (line separator) and \u2029 (paragraph separator)
+    if (codePoint === 10 || codePoint === 0x2028 || codePoint === 0x2029) {
+      output.push(lineLength)


Using the non-primordial version due to ArrayPrototypePush being listed as having perf issues — https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md#primordials-with-known-performance-issues

aduh95 · 2023-11-29T23:06:00Z

lib/internal/source_map/source_map_cache.js

+      lineLength = -1; // To not count the matched codePoint such as \n character
+    }
+  }
+  output.push(lineLength)


Suggested change

output.push(lineLength)

ArrayPrototypePush(output, lineLength);

ArrayPrototypePush has known perf issues — #50969 (comment)

Using .push has reliability issues, as user code could affect internals.

zeusdeux · 2023-11-29T23:45:15Z

@aduh95 @GeoffreyBooth @anonrig What's the call on the usage of .push vs ArrayPrototypePush from the perspective of correctness as brought up by @aduh95? Should I invest time on measuring the perf impact or is .push ok as is?

anonrig · 2023-11-29T23:56:44Z

@zeusdeux Primordials are optional and not mandatory for new pull requests. It's your decision.

Documentation says that:

Usage of primordials should be preferred for any new code...

Ref: https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md

GeoffreyBooth · 2023-11-30T00:10:30Z

@aduh95 @GeoffreyBooth @anonrig What’s the call on the usage of .push vs ArrayPrototypePush from the perspective of correctness as brought up by @aduh95?

I don’t have a preference. I thought we were still debating whether they should be required, but I don’t really see the need.

When using a loader, for say TypeScript, the esm loader invokes the `lineLengths` function via `maybeCacheSourceMap` when sourcemaps are enabled. Therefore, `lineLengths` ends up getting called quite often when running large servers written in TypeScript for example. Making `lineLengths` faster should therefore speed up server startup times for anyone using a loader with node with sourcemaps enabled. The change itself is fairly simple and is all about removing creation of unnecessary memory and iterating the whole source content only once with the hope of making the function cache friendly.

zeusdeux · 2023-11-30T01:26:06Z

I went ahead and did 4 runs of the same test as in the perf profile screenshots in the original PR description. Using ArrayPrototypePush seems to shave off another ~8ms for an average of ~72ms across the four runs. I have therefore updated the PR to use it as we get both safety and speed in my preliminary tests.

Process for testing:

update the code to use ArrayPrototypePush
recompile node
link to the release binary
run server under test
take a cpuprofile x 4

Example run

GeoffreyBooth · 2023-11-30T04:06:01Z

Using ArrayPrototypePush seems to shave off

If you're going to use that one you might as well use the others too?

ljharb

looks great, robustness > performance, altho even better if we can get both.

you may want to benchmark arr[arr.length] = x over ArrayPrototypePush(arr, x) just in case tho

aduh95

Great to hear we can reconcile robustness and perf. Ideally we would run benchmarks after each V8 upgrades to see if the claims in primordials.md are still correct, but it's very time consuming, and easy to get false positive.

If you want to check the performance of arr[arr.length] = value and setOwnProperty(arr, arr.length, value) for completeness, that'd be useful data.

zeusdeux · 2023-11-30T10:21:41Z

@GeoffreyBooth which other ones? This PR uses StringPrototypeCodePointAt and ArrayPrototypePush already.
As for the completeness aspect @ljharb @aduh95, I can do that post landing perhaps as the time spent per run of data collection is quite high.

aduh95 · 2023-11-30T10:29:56Z

✘ This PR needs to wait 35 more hours to land

Sure, whatever works for you. But FYI this PR needs to wait at least 48 hours after it was opened before we can merge it to give time for folks to review, so it may be quicker to get the data before the PR is merged depending on what are your availabilities.

nodejs-github-bot · 2023-12-01T22:08:49Z

Commit Queue failed

- Loading data for nodejs/node/pull/50969
✔  Done loading data for nodejs/node/pull/50969
----------------------------------- PR info ------------------------------------
Title      loader: speed up line length calculation used by moduleProvider (#50969)
Author     Mudit  (@zeusdeux)
Branch     zeusdeux:speed-up-source-map-processing -> nodejs:main
Labels     module, performance, needs-ci, source maps
Commits    1
 - loader: speed up line length calc used by moduleProvider
Committers 1
 - Mudit Ameta 
PR-URL: https://github.com/nodejs/node/pull/50969
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Geoffrey Booth 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50969
Reviewed-By: Yagiz Nizipli 
Reviewed-By: Geoffrey Booth 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 29 Nov 2023 22:02:05 GMT
   ✔  Approvals: 3
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/50969#pullrequestreview-1758724448
   ✔  - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/50969#pullrequestreview-1756426036
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/50969#pullrequestreview-1757049826
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-11-29T22:54:10Z: https://ci.nodejs.org/job/node-test-pull-request/56011/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - loader: speed up line length calc used by moduleProvider
- Querying data for job/node-test-pull-request/56011/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

nodejs-github-bot · 2023-12-02T00:23:56Z

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

JakobJingleheimer

Huzzah 🙌 Thanks for this!

Out of curiosity, where did the huge perf gain come from? arr.map → for, or RegExp → equality assertion, or both significantly contributed? (I would guess both)

nodejs-github-bot · 2023-12-02T13:53:07Z

Landed in d5a7acf

zeusdeux · 2023-12-03T00:43:43Z

@JakobJingleheimer Thanks! The speed up was from the combination of the following —

Not allocating new memory for the regex and not trampolining to the regex constructor
Not allocating new memory for the function given to map and thus its whole closure
Not trampolining between map and the fn it calls per item
Only traversing the string content once linearly with no side effects to it thus making it cache line friendly
Doing simple math in a for loop that all compilers are extremely good at optimizing (for loops/while/do while will always beat perf of for of, map and friends)

Basically doing only what's needed and doing it using an known optimizable construct. Finally, no extra memory overhead than absolutely necessary. Thinking in C rather than JS in a way.

Functional programming is good but it's usually horrible for perf so going old school for parts that are sensitive to perf is always a win!

When using a loader, for say TypeScript, the esm loader invokes the `lineLengths` function via `maybeCacheSourceMap` when sourcemaps are enabled. Therefore, `lineLengths` ends up getting called quite often when running large servers written in TypeScript for example. Making `lineLengths` faster should therefore speed up server startup times for anyone using a loader with node with sourcemaps enabled. The change itself is fairly simple and is all about removing creation of unnecessary memory and iterating the whole source content only once with the hope of making the function cache friendly. PR-URL: #50969 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Jacob Smith <jacob@frende.me>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 29, 2023

zeusdeux changed the title ~~loader: Speed up line length calculation used by moduleProvider~~ loader: speed up line length calculation used by moduleProvider Nov 29, 2023

zeusdeux force-pushed the speed-up-source-map-processing branch 2 times, most recently from 172138f to 686ce4e Compare November 29, 2023 22:10

GeoffreyBooth added module Issues and PRs related to the module subsystem. source maps Issues and PRs related to source map support. performance Issues and PRs related to the performance of Node.js. labels Nov 29, 2023

anonrig approved these changes Nov 29, 2023

View reviewed changes

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 29, 2023

ljharb requested changes Nov 29, 2023

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2023

GeoffreyBooth reviewed Nov 29, 2023

View reviewed changes

GeoffreyBooth removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 29, 2023

zeusdeux force-pushed the speed-up-source-map-processing branch 2 times, most recently from 4e8a83f to 813a604 Compare November 29, 2023 23:01

aduh95 reviewed Nov 29, 2023

View reviewed changes

zeusdeux commented Nov 29, 2023

View reviewed changes

aduh95 reviewed Nov 29, 2023

View reviewed changes

zeusdeux force-pushed the speed-up-source-map-processing branch from 813a604 to 133f887 Compare November 29, 2023 23:08

zeusdeux requested review from GeoffreyBooth, aduh95 and ljharb November 29, 2023 23:14

zeusdeux force-pushed the speed-up-source-map-processing branch from 364939f to 51f44ab Compare November 30, 2023 01:26

ljharb approved these changes Nov 30, 2023

View reviewed changes

aduh95 approved these changes Nov 30, 2023

View reviewed changes

anonrig approved these changes Nov 30, 2023

View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 30, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 1, 2023

H4ad approved these changes Dec 2, 2023

View reviewed changes

H4ad added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 2, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 2, 2023

JakobJingleheimer approved these changes Dec 2, 2023

View reviewed changes

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 2, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 2, 2023

nodejs-github-bot merged commit d5a7acf into nodejs:main Dec 2, 2023

zeusdeux deleted the speed-up-source-map-processing branch December 3, 2023 00:44

targos mentioned this pull request Dec 4, 2023

v21.4.0 release proposal #51043

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

	output.push(lineLength);
	ArrayPrototypePush(output, lineLength);

	output.push(lineLength)
	ArrayPrototypePush(output, lineLength);

Uh oh!

loader: speed up line length calculation used by moduleProvider #50969

loader: speed up line length calculation used by moduleProvider #50969

Uh oh!

Conversation

zeusdeux commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

node 20.9.0

node main + patch from this PR

Uh oh!

zeusdeux commented Nov 29, 2023

Uh oh!

GeoffreyBooth commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeusdeux commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Nov 29, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusdeux commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anonrig commented Nov 29, 2023

Uh oh!

GeoffreyBooth commented Nov 30, 2023

Uh oh!

zeusdeux commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example run

Uh oh!

GeoffreyBooth commented Nov 30, 2023

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

zeusdeux commented Nov 30, 2023

Uh oh!

aduh95 commented Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 1, 2023

Uh oh!

nodejs-github-bot commented Dec 2, 2023

Uh oh!

JakobJingleheimer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 2, 2023

Uh oh!

zeusdeux commented Nov 29, 2023 •

edited

Loading

node `main` + patch from this PR

GeoffreyBooth commented Nov 29, 2023 •

edited

Loading

zeusdeux commented Nov 29, 2023 •

edited

Loading

zeusdeux commented Nov 29, 2023 •

edited

Loading

zeusdeux commented Nov 30, 2023 •

edited

Loading

aduh95 commented Nov 30, 2023 •

edited

Loading

JakobJingleheimer left a comment •

edited

Loading