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

joyeecheung · 2024-08-09T01:11:51Z

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

Using the Intl API to get the default locale slows down the startup
significantly. This patch uses a new v8 API to get the default locale
directly.

Refs: #53765 (comment)

First commit comes from https://chromium-review.googlesource.com/c/v8/v8/+/5772938

From benchmark CI:

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1594/console

14:54:15                                                                                           confidence improvement accuracy (*)   (**)  (***)
14:54:15 misc/startup-core.js count=30 mode='process' script='benchmark/fixtures/require-builtins'        ***     18.60 %       ±0.30% ±0.41% ±0.54%
14:54:15 misc/startup-core.js count=30 mode='process' script='test/fixtures/semicolon'                    ***     25.02 %       ±2.92% ±3.93% ±5.19%
14:54:15 misc/startup-core.js count=30 mode='process' script='test/fixtures/snapshot/typescript'          ***      4.32 %       ±0.04% ±0.06% ±0.07%
14:54:15 misc/startup-core.js count=30 mode='worker' script='benchmark/fixtures/require-builtins'                 -0.05 %       ±0.18% ±0.24% ±0.31%
14:54:15 misc/startup-core.js count=30 mode='worker' script='test/fixtures/semicolon'                              0.02 %       ±0.36% ±0.47% ±0.62%
14:54:15 misc/startup-core.js count=30 mode='worker' script='test/fixtures/snapshot/typescript'            **     -1.48 %       ±0.87% ±1.16%

nodejs-github-bot · 2024-08-09T01:11:56Z

Review requested:

@nodejs/security-wg
@nodejs/v8-update
@nodejs/web-standards

ljharb

thanks, this was beyond my skills but definitely seems like an improvement :-)

joyeecheung · 2024-08-09T13:15:38Z

From benchmark CI:

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1594/console

14:54:15                                                                                           confidence improvement accuracy (*)   (**)  (***)
14:54:15 misc/startup-core.js count=30 mode='process' script='benchmark/fixtures/require-builtins'        ***     18.60 %       ±0.30% ±0.41% ±0.54%
14:54:15 misc/startup-core.js count=30 mode='process' script='test/fixtures/semicolon'                    ***     25.02 %       ±2.92% ±3.93% ±5.19%
14:54:15 misc/startup-core.js count=30 mode='process' script='test/fixtures/snapshot/typescript'          ***      4.32 %       ±0.04% ±0.06% ±0.07%
14:54:15 misc/startup-core.js count=30 mode='worker' script='benchmark/fixtures/require-builtins'                 -0.05 %       ±0.18% ±0.24% ±0.31%
14:54:15 misc/startup-core.js count=30 mode='worker' script='test/fixtures/semicolon'                              0.02 %       ±0.36% ±0.47% ±0.62%
14:54:15 misc/startup-core.js count=30 mode='worker' script='test/fixtures/snapshot/typescript'            **     -1.48 %       ±0.87% ±1.16%

This allows embedders to query the default locale used by Intl APIs. This information is already available to user land via Intl?.Collator().resolvedOptions().locale, the issue with this is that it's a lot slower and requires dynamic access to Intl API, which is subject to patching and prototype pollution so it's not as reliable for embedders than having a V8 API to query the default locale directly. Refs: nodejs/node#54279 Change-Id: I5a1823993c9ae79f8f61f54c6464daf882a09ba3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5772938 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#95678}

Original commit message: [api] add v8::Isolate::GetDefaultLocale() This allows embedders to query the default locale used by Intl APIs. This information is already available to user land via Intl?.Collator().resolvedOptions().locale, the issue with this is that it's a lot slower and requires dynamic access to Intl API, which is subject to patching and prototype pollution so it's not as reliable for embedders than having a V8 API to query the default locale directly. Refs: nodejs#54279 Change-Id: I5a1823993c9ae79f8f61f54c6464daf882a09ba3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5772938 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#95678} Refs: v8/v8@e74d0f4

Using the Intl API to get the default locale slows down the startup significantly. This patch uses a new v8 API to get the default locale directly.

joyeecheung · 2024-08-19T16:55:07Z

Upstream CL has landed, backported and this should be ready for review now.

cc @nodejs/startup

nodejs-github-bot · 2024-08-19T16:55:18Z

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

nodejs-github-bot · 2024-08-19T16:55:33Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6142/

nodejs-github-bot · 2024-08-19T16:58:38Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6142/

nodejs-github-bot · 2024-08-19T17:10:59Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6142/

nodejs-github-bot · 2024-08-19T17:48:57Z

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

nodejs-github-bot · 2024-08-19T17:49:20Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6143/

nodejs-github-bot · 2024-08-19T17:52:52Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6143/

nodejs-github-bot · 2024-08-19T18:04:19Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6143/

codecov · 2024-08-19T19:26:53Z

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.31%. Comparing base (2c14615) to head (c330418).
Report is 403 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_config.cc	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54279      +/-   ##
==========================================
+ Coverage   87.08%   87.31%   +0.23%     
==========================================
  Files         648      648              
  Lines      182341   182376      +35     
  Branches    34982    34982              
==========================================
+ Hits       158783   159240     +457     
+ Misses      16831    16401     -430     
- Partials     6727     6735       +8

Files with missing lines	Coverage Δ
lib/internal/navigator.js	`99.32% <100.00%> (+<0.01%)`	⬆️
lib/internal/process/pre_execution.js	`88.46% <ø> (-2.46%)`	⬇️
src/node_external_reference.h	`100.00% <ø> (ø)`
src/node_config.cc	`96.42% <93.75%> (-3.58%)`	⬇️

... and 62 files with indirect coverage changes

lib/internal/navigator.js

nodejs-github-bot · 2024-08-26T16:21:13Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6155/

nodejs-github-bot · 2024-08-26T16:24:44Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6155/

nodejs-github-bot · 2024-08-26T16:31:16Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6155/

nodejs-github-bot · 2024-08-27T08:37:24Z

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

nodejs-github-bot · 2024-08-27T16:02:52Z

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

nodejs-github-bot · 2024-08-28T13:36:57Z

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

nodejs-github-bot · 2024-08-28T19:54:59Z

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

nodejs-github-bot · 2024-08-29T03:25:53Z

Landed in c44d41b...75741a1

Original commit message: [api] add v8::Isolate::GetDefaultLocale() This allows embedders to query the default locale used by Intl APIs. This information is already available to user land via Intl?.Collator().resolvedOptions().locale, the issue with this is that it's a lot slower and requires dynamic access to Intl API, which is subject to patching and prototype pollution so it's not as reliable for embedders than having a V8 API to query the default locale directly. Refs: #54279 Change-Id: I5a1823993c9ae79f8f61f54c6464daf882a09ba3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5772938 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#95678} Refs: v8/v8@e74d0f4 PR-URL: #54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

Using the Intl API to get the default locale slows down the startup significantly. This patch uses a new v8 API to get the default locale directly. PR-URL: #54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

Original commit message: [api] add v8::Isolate::GetDefaultLocale() This allows embedders to query the default locale used by Intl APIs. This information is already available to user land via Intl?.Collator().resolvedOptions().locale, the issue with this is that it's a lot slower and requires dynamic access to Intl API, which is subject to patching and prototype pollution so it's not as reliable for embedders than having a V8 API to query the default locale directly. Refs: #54279 Change-Id: I5a1823993c9ae79f8f61f54c6464daf882a09ba3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5772938 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#95678} Refs: v8/v8@e74d0f4 PR-URL: #54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

Using the Intl API to get the default locale slows down the startup significantly. This patch uses a new v8 API to get the default locale directly. PR-URL: #54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

Original commit message: [api] add v8::Isolate::GetDefaultLocale() This allows embedders to query the default locale used by Intl APIs. This information is already available to user land via Intl?.Collator().resolvedOptions().locale, the issue with this is that it's a lot slower and requires dynamic access to Intl API, which is subject to patching and prototype pollution so it's not as reliable for embedders than having a V8 API to query the default locale directly. Refs: #54279 Change-Id: I5a1823993c9ae79f8f61f54c6464daf882a09ba3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5772938 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#95678} Refs: v8/v8@e74d0f4 PR-URL: #54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

Using the Intl API to get the default locale slows down the startup significantly. This patch uses a new v8 API to get the default locale directly. PR-URL: #54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

Original commit message: [api] add v8::Isolate::GetDefaultLocale() This allows embedders to query the default locale used by Intl APIs. This information is already available to user land via Intl?.Collator().resolvedOptions().locale, the issue with this is that it's a lot slower and requires dynamic access to Intl API, which is subject to patching and prototype pollution so it's not as reliable for embedders than having a V8 API to query the default locale directly. Refs: #54279 Change-Id: I5a1823993c9ae79f8f61f54c6464daf882a09ba3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5772938 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#95678} Refs: v8/v8@e74d0f4 PR-URL: #54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

Using the Intl API to get the default locale slows down the startup significantly. This patch uses a new v8 API to get the default locale directly. PR-URL: #54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

bnoordhuis · 2024-09-18T18:55:46Z

For future reference: this PR didn't bump v8_embedder_string. v22.7.0, v22.8.0 and v22.9.0 all report the same V8 version but v22.7.0 does not have the new API.

Note the V8 version didn't change between 22.7.0 and 22.9.0 although Node.js did add a new API in nodejs/node#54279 Remove unused and/or unnecessary patches. Upstream disabled Maglev so we no longer have to do that manually. The genv8constants.py patches were for a script that I removed upstream last year.

joyeecheung · 2024-09-18T19:51:58Z

hmm, the PR diff did bump the string (https://github.com/nodejs/node/pull/54279/files#diff-9513a0c08584f4800e1461ca8c3456d1aeb899968ea14b2f6859ea462c37b5bb), but I think it might have been lost during merging if another PR that bumped the string landed before it.

joyeecheung · 2024-09-18T19:53:53Z

(I guess the right way to prevent something like this from happening again is probably asking folks to only leave REPLACEME in the PR, and let the git node land bump the string properly when it gets merged).

Note the V8 version didn't change between 22.7.0 and 22.9.0 although Node.js did add a new API in nodejs/node#54279 Remove unused and/or unnecessary patches. Upstream disabled Maglev so we no longer have to do that manually. The genv8constants.py patches were for a script that I removed upstream last year.

Original commit message: [api] add v8::Isolate::GetDefaultLocale() This allows embedders to query the default locale used by Intl APIs. This information is already available to user land via Intl?.Collator().resolvedOptions().locale, the issue with this is that it's a lot slower and requires dynamic access to Intl API, which is subject to patching and prototype pollution so it's not as reliable for embedders than having a V8 API to query the default locale directly. Refs: nodejs#54279 Change-Id: I5a1823993c9ae79f8f61f54c6464daf882a09ba3 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5772938 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#95678} Refs: v8/v8@e74d0f4 PR-URL: nodejs#54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

Using the Intl API to get the default locale slows down the startup significantly. This patch uses a new v8 API to get the default locale directly. PR-URL: nodejs#54279 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 Aug 9, 2024

ljharb approved these changes Aug 9, 2024

View reviewed changes

OnlyAviv added wip Issues and PRs that are still a work in progress. v8 engine Issues and PRs related to the V8 dependency. dependencies Pull requests that update a dependency file. labels Aug 9, 2024

joyeecheung added 2 commits August 19, 2024 18:51

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

4857714

Using the Intl API to get the default locale slows down the startup significantly. This patch uses a new v8 API to get the default locale directly.

joyeecheung force-pushed the locale-native branch from 55ec980 to 4857714 Compare August 19, 2024 16:52

joyeecheung marked this pull request as ready for review August 19, 2024 16:52

joyeecheung changed the title ~~[wip] src: use v8::Isolate::GetDefaultLocale() to compute navigator.language~~ src: use v8::Isolate::GetDefaultLocale() to compute navigator.language Aug 19, 2024

joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed wip Issues and PRs that are still a work in progress. labels Aug 19, 2024

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

joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 19, 2024

bnoordhuis approved these changes Aug 19, 2024

View reviewed changes

lib/internal/navigator.js Show resolved Hide resolved

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Aug 29, 2024

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

nodejs-github-bot closed this Aug 29, 2024

RafaelGSS mentioned this pull request Aug 30, 2024

v22.8.0 proposal #54560

Merged

bnoordhuis mentioned this pull request Sep 18, 2024

Update to node 22.9.0 rubyjs/libv8-node#62

Merged

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 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joyeecheung commented Aug 9, 2024 •

edited

Loading

nodejs-github-bot commented Aug 9, 2024

ljharb left a comment

joyeecheung commented Aug 9, 2024

joyeecheung commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

codecov bot commented Aug 19, 2024 •

edited

Loading

nodejs-github-bot commented Aug 26, 2024

nodejs-github-bot commented Aug 26, 2024

nodejs-github-bot commented Aug 26, 2024

nodejs-github-bot commented Aug 27, 2024

nodejs-github-bot commented Aug 27, 2024

nodejs-github-bot commented Aug 28, 2024

nodejs-github-bot commented Aug 28, 2024

nodejs-github-bot commented Aug 29, 2024

bnoordhuis commented Sep 18, 2024

joyeecheung commented Sep 18, 2024

joyeecheung commented Sep 18, 2024 •

edited

Loading

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

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

Conversation

joyeecheung commented Aug 9, 2024 • edited Loading

nodejs-github-bot commented Aug 9, 2024

ljharb left a comment

Choose a reason for hiding this comment

joyeecheung commented Aug 9, 2024

joyeecheung commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

nodejs-github-bot commented Aug 19, 2024

codecov bot commented Aug 19, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Aug 26, 2024

nodejs-github-bot commented Aug 26, 2024

nodejs-github-bot commented Aug 26, 2024

nodejs-github-bot commented Aug 27, 2024

nodejs-github-bot commented Aug 27, 2024

nodejs-github-bot commented Aug 28, 2024

nodejs-github-bot commented Aug 28, 2024

nodejs-github-bot commented Aug 29, 2024

bnoordhuis commented Sep 18, 2024

joyeecheung commented Sep 18, 2024

joyeecheung commented Sep 18, 2024 • edited Loading

joyeecheung commented Aug 9, 2024 •

edited

Loading

codecov bot commented Aug 19, 2024 •

edited

Loading

joyeecheung commented Sep 18, 2024 •

edited

Loading