lib: make navigator not runtime-lookup process.version, arch, or platform #53765

ljharb · 2024-07-08T19:24:29Z

Preserves #53649.

nodejs-github-bot · 2024-07-08T19:24:34Z

Review requested:

@nodejs/startup
@nodejs/web-standards

mcollina · 2024-07-08T20:45:01Z

lib/internal/bootstrap/node.js

@@ -85,6 +85,10 @@ const {
  },
 } = internalBinding('util');

+require('internal/process/arch'); // Ensure the process arch is cached
+require('internal/process/platform'); // Ensure the process platform is cached
+require('internal/process/version'); // Etnsure the process version is cached


I have a feeling these will slow down startup.

They very well might, but since it's just three values i'm hoping not.

@joyeecheung can you take a look?

@ljharb I would prefer not to worsen this.

The problem is not “three small modules would not themselves regress startup”, but that if we continue with this pattern, we end up with hundreds of modules that inevitably regress startup. If this is not something universally used by all applications, then it should be lazy loaded, or put into an existing file already loaded on startup. See #45659 (comment) and #45849

navigator is lazy loaded already. For the process pieces, I’d love suggestions of a better way to ensure the original values are used here - all i was able to come up with were these tiny modules. (an alternative is making a single module that’s a shallow copy of a subset of the process object)

test/parallel/test-navigator.js

benjamingr

This LGTM though I think it's fine to remove the lib/internal/bootstrap code (they're required in lib/internal/navigator which is hopefully/presumably lazily loaded). That should also resolve @mcollina's concern

aduh95 · 2024-07-08T21:37:52Z

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

Results

                                                                                          confidence improvement accuracy (*)   (**)  (***)
misc/startup-cli-version.js count=30 cli='deps/corepack/dist/corepack.js'                                 0.03 %       ±0.07% ±0.09% ±0.11%
misc/startup-cli-version.js count=30 cli='deps/npm/bin/npm-cli.js'                                        0.03 %       ±0.15% ±0.19% ±0.25%
misc/startup-cli-version.js count=30 cli='deps/npm/bin/npx-cli.js'                                        0.04 %       ±0.18% ±0.24% ±0.31%
misc/startup-cli-version.js count=30 cli='tools/eslint/node_modules/eslint/bin/eslint.js'        ***      0.19 %       ±0.06% ±0.08% ±0.11%
misc/startup-core.js count=30 mode='process' script='benchmark/fixtures/require-builtins'                 0.25 %       ±0.37% ±0.49% ±0.64%
misc/startup-core.js count=30 mode='process' script='test/fixtures/semicolon'                             0.29 %       ±3.43% ±4.57% ±5.94%
misc/startup-core.js count=30 mode='process' script='test/fixtures/snapshot/typescript'          ***      1.05 %       ±0.05% ±0.07% ±0.09%
misc/startup-core.js count=30 mode='worker' script='benchmark/fixtures/require-builtins'                 -0.05 %       ±0.21% ±0.28% ±0.36%
misc/startup-core.js count=30 mode='worker' script='test/fixtures/semicolon'                              0.09 %       ±0.35% ±0.47% ±0.61%
misc/startup-core.js count=30 mode='worker' script='test/fixtures/snapshot/typescript'                   -0.71 %       ±1.03% ±1.37% ±1.78%

ljharb · 2024-07-08T21:57:22Z

@benjamingr i couldn't get it to work without the bootstrap, i think precisely because navigator is lazy loaded.

lib/internal/navigator.js

lib/internal/bootstrap/node.js

test/parallel/test-bootstrap-modules.js

lib/internal/bootstrap/node.js

ljharb · 2024-07-09T16:56:27Z

Updated; I went with just adding properties to the existing per-thread object to avoid creating an extra object, but I can certainly nest them in their own object (and try to come up with a name for it) if that's preferred.

nodejs-github-bot · 2024-07-11T12:26:15Z

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

ljharb · 2024-07-15T19:00:20Z

(rebased, and included updating https://github.com/nodejs/node/pull/53817/files#diff-26e6b1e2a90040b1ce20e7f305b2ff3140b483f863ee2573b4c6af9b696ad31eR1472, per request)

nodejs-github-bot · 2024-07-15T19:05:08Z

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

lib/internal/process/per_thread.js

nodejs-github-bot · 2024-07-15T21:00:54Z

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

nodejs-github-bot · 2024-07-15T23:09:56Z

Landed in 04e08ad

Preserves #53649. PR-URL: #53765 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Preserves nodejs#53649. PR-URL: nodejs#53765 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung · 2024-08-08T22:43:13Z

I just noticed that this actually led to a performance regression and defeated #53649 - the eagerly computed locale via Intl is taking up a whopping 30% of the startup time.

joyeecheung · 2024-08-08T22:46:35Z

Also, technically locale is runtime-dependent and should not be computed at startup - we don't update the locale dynamically in core currently but addons can use LocaleConfigurationChangeNotification() to update it dynamically, and the right behavior in that case is to reflect this in navigator.language (I think it'll need V8 to expose the computed default locale to implement it properly, however).

lib: make navigator not runtime-lookup process.version/arch/platform

e0437ed

Preserves nodejs#53649.

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 8, 2024

ljharb force-pushed the fix-navigator-bug branch 3 times, most recently from 1aa58ab to 4002f74 Compare July 8, 2024 20:35

mcollina reviewed Jul 8, 2024

View reviewed changes

aduh95 reviewed Jul 8, 2024

View reviewed changes

test/parallel/test-navigator.js Outdated Show resolved Hide resolved

benjamingr approved these changes Jul 8, 2024

View reviewed changes

ljharb force-pushed the fix-navigator-bug branch 5 times, most recently from 77899c0 to 0f7da61 Compare July 8, 2024 22:11

anonrig requested a review from jasnell July 8, 2024 22:26

anonrig reviewed Jul 8, 2024

View reviewed changes

lib/internal/navigator.js Outdated Show resolved Hide resolved

lib/internal/navigator.js Outdated Show resolved Hide resolved

ljharb force-pushed the fix-navigator-bug branch from 0f7da61 to 0acb361 Compare July 8, 2024 23:06

ljharb requested a review from anonrig July 8, 2024 23:10

ljharb force-pushed the fix-navigator-bug branch from 0acb361 to 263e61d Compare July 8, 2024 23:11

anonrig approved these changes Jul 8, 2024

View reviewed changes

daeyeon reviewed Jul 9, 2024

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

test/parallel/test-bootstrap-modules.js Outdated Show resolved Hide resolved

ljharb force-pushed the fix-navigator-bug branch from 263e61d to d5ccfd4 Compare July 9, 2024 04:04

joyeecheung requested changes Jul 9, 2024

View reviewed changes

lib/internal/bootstrap/node.js Outdated Show resolved Hide resolved

ljharb force-pushed the fix-navigator-bug branch from d5ccfd4 to 2b020c8 Compare July 9, 2024 16:55

ljharb requested review from joyeecheung and daeyeon July 9, 2024 16:56

aduh95 approved these changes Jul 11, 2024

View reviewed changes

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 11, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 11, 2024

daeyeon approved these changes Jul 11, 2024

View reviewed changes

ljharb force-pushed the fix-navigator-bug branch from abf95d3 to 8a25835 Compare July 15, 2024 18:59

aduh95 approved these changes Jul 15, 2024

View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 15, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 15, 2024

jasnell approved these changes Jul 15, 2024

View reviewed changes

ljharb force-pushed the fix-navigator-bug branch from 8a25835 to 42edb23 Compare July 15, 2024 20:46

aduh95 reviewed Jul 15, 2024

View reviewed changes

lib/internal/process/per_thread.js Outdated Show resolved Hide resolved

ljharb force-pushed the fix-navigator-bug branch from 42edb23 to e0437ed Compare July 15, 2024 20:56

aduh95 approved these changes Jul 15, 2024

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 15, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 15, 2024

nodejs-github-bot merged commit 04e08ad into nodejs:main Jul 15, 2024
53 checks passed

ljharb deleted the fix-navigator-bug branch July 15, 2024 23:16

aduh95 mentioned this pull request Jul 16, 2024

V22.5.0 proposal #53826

Merged

joyeecheung mentioned this pull request Aug 9, 2024

src: use v8::Isolate::GetDefaultLocale() to compute navigator.language #54279

Closed

targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: make navigator not runtime-lookup process.version, arch, or platform #53765

lib: make navigator not runtime-lookup process.version, arch, or platform #53765

ljharb commented Jul 8, 2024 •

edited

Loading

nodejs-github-bot commented Jul 8, 2024

mcollina Jul 8, 2024

ljharb Jul 8, 2024

This comment was marked as resolved.

mcollina Jul 9, 2024

mcollina Jul 9, 2024

joyeecheung Jul 9, 2024 •

edited

Loading

ljharb Jul 9, 2024

benjamingr left a comment

aduh95 commented Jul 8, 2024 •

edited

Loading

ljharb commented Jul 8, 2024

ljharb commented Jul 9, 2024 •

edited

Loading

nodejs-github-bot commented Jul 11, 2024

ljharb commented Jul 15, 2024

nodejs-github-bot commented Jul 15, 2024

nodejs-github-bot commented Jul 15, 2024

nodejs-github-bot commented Jul 15, 2024

joyeecheung commented Aug 8, 2024

joyeecheung commented Aug 8, 2024

lib: make navigator not runtime-lookup process.version, arch, or platform #53765

lib: make navigator not runtime-lookup process.version, arch, or platform #53765

Conversation

ljharb commented Jul 8, 2024 • edited Loading

nodejs-github-bot commented Jul 8, 2024

mcollina Jul 8, 2024

Choose a reason for hiding this comment

ljharb Jul 8, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

mcollina Jul 9, 2024

Choose a reason for hiding this comment

mcollina Jul 9, 2024

Choose a reason for hiding this comment

joyeecheung Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

ljharb Jul 9, 2024

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

aduh95 commented Jul 8, 2024 • edited Loading

ljharb commented Jul 8, 2024

ljharb commented Jul 9, 2024 • edited Loading

nodejs-github-bot commented Jul 11, 2024

ljharb commented Jul 15, 2024

nodejs-github-bot commented Jul 15, 2024

nodejs-github-bot commented Jul 15, 2024

nodejs-github-bot commented Jul 15, 2024

joyeecheung commented Aug 8, 2024

joyeecheung commented Aug 8, 2024

ljharb commented Jul 8, 2024 •

edited

Loading

joyeecheung Jul 9, 2024 •

edited

Loading

aduh95 commented Jul 8, 2024 •

edited

Loading

ljharb commented Jul 9, 2024 •

edited

Loading