lib: make navigator not runtime-lookup process.version, arch, or platform - #53765
lib: make navigator not runtime-lookup process.version, arch, or platform#53765nodejs-github-bot merged 1 commit into
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
There was a problem hiding this comment.
I have a feeling these will slow down startup.
Sorry, something went wrong.
There was a problem hiding this comment.
They very well might, but since it's just three values i'm hoping not.
Sorry, something went wrong.
This comment was marked as resolved.
There was a problem hiding this comment.
@joyeecheung can you take a look?
Sorry, something went wrong.
There was a problem hiding this comment.
@ljharb I would prefer not to worsen this.
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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)
Sorry, something went wrong.
benjamingr
left a comment
There was a problem hiding this comment.
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
Sorry, something went wrong.
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1577/ Resultsconfidence 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% |
Sorry, something went wrong.
|
@benjamingr i couldn't get it to work without the bootstrap, i think precisely because navigator is lazy loaded. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
|
(rebased, and included updating https://github.com/nodejs/node/pull/53817/files#diff-26e6b1e2a90040b1ce20e7f305b2ff3140b483f863ee2573b4c6af9b696ad31eR1472, per request) |
Sorry, something went wrong.
Sorry, something went wrong.
Sorry, something went wrong.
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>
|
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. |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
Preserves #53649.