[v22.x] Revert "v8: enable maglev on supported architectures" by joyeecheung · Pull Request #54384 · nodejs/node · GitHub

src: create handle scope in FastInternalModuleStat

It needs a handle scope for the context handle. Since the
FastApiCallbackOptions struct doesn't have isolate on it
in V8 12.4 on Node.js 22, use Isolate::TryGetCurrent() to
get to the isolate needed for the handle scope creation and
fallback to the slow callback if no isolate is entered.

Revert "v8: enable maglev on supported architectures"

This reverts commit 1a5acd0.

Reason to revert: we have seen several crashes/unexpected JS behaviors with maglev on v22 (which ships V8 v12.4). The bugs lie in the codegen so it would be difficult for users to work around them or even figure out where the bugs are coming from. Some bugs are fixed in the upstream while some others probably remain. As v22 will get stuck with V8 v12.4 as LTS, it will be increasingly difficult to backport patches for them even if the bugs are fixed. So disable it by default on v22 to reduce the churn and troubles for users.

Refs: #52797

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @richardlau.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

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

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

lgtm

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

There appears to be a consistent crash in node-test-commit-arm-debug with this PR,
e.g. https://ci.nodejs.org/job/node-test-commit-arm-debug/14293/nodes=ubuntu2004_debug-arm64/consoleFull

16:46:34 not ok 2405 parallel/test-npm-install
16:46:34   ---
16:46:34   duration_ms: 1636.99700
16:46:34   severity: fail
16:46:34   exitcode: 1
16:46:34   stack: |-
16:46:34     npm warn cli npm v10.8.2 does not support Node.js v22.6.1-pre. This version of npm supports the following node versions: `^18.17.0 || >=20.5.0`. You can find the latest version at https://nodejs.org/.
16:46:34     FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope
16:46:34     ----- Native stack trace -----
16:46:34     
16:46:34      1: 0xaaaaca4e6cdc node::DumpNativeBacktrace(_IO_FILE*) [npm install]
16:46:34      2: 0xaaaaca62ff58 node::OnFatalError(char const*, char const*) [npm install]
16:46:34      3: 0xaaaacaab6190 v8::Utils::ReportApiFailure(char const*, char const*) [npm install]
16:46:34      4: 0xaaaacae61a4c v8::internal::HandleScope::Extend(v8::internal::Isolate*) [npm install]
16:46:34      5: 0xaaaacaaa67c0 v8::internal::HandleScope::CreateHandle(v8::internal::Isolate*, unsigned long) [npm install]
16:46:34      6: 0xaaaacaaf7180 v8::Object::GetCreationContext() [npm install]
16:46:34      7: 0xaaaacaaf7210 v8::Object::GetCreationContextChecked() [npm install]
16:46:34      8: 0xaaaaca63e0bc  [npm install]
16:46:34      9: 0xffffa80951d8 
16:46:34     Aborted (core dumped)
16:46:34     node:assert:126
16:46:34       throw new AssertionError(obj);
16:46:34       ^
16:46:34     
16:46:34     AssertionError [ERR_ASSERTION]: npm install got error code 134
16:46:34         at handleExit (/home/iojs/build/workspace/node-test-commit-arm-debug/test/parallel/test-npm-install.js:62:10)
16:46:34         at handleExit (/home/iojs/build/workspace/node-test-commit-arm-debug/test/common/index.js:488:15)
16:46:34         at ChildProcess.exithandler (node:child_process:429:5)
16:46:34         at ChildProcess.emit (node:events:520:28)
16:46:34         at maybeClose (node:internal/child_process:1105:16)
16:46:34         at Socket.<anonymous> (node:internal/child_process:457:11)
16:46:34         at Socket.emit (node:events:520:28)
16:46:34         at Pipe.<anonymous> (node:net:337:12) {
16:46:34       generatedMessage: false,
16:46:34       code: 'ERR_ASSERTION',
16:46:34       actual: 134,
16:46:34       expected: 0,
16:46:34       operator: 'strictEqual'
16:46:34     }
16:46:34     
16:46:34     Node.js v22.6.1-pre
16:46:34   ...

which isn't occurring on v22.x-staging: https://ci.nodejs.org/job/node-test-commit-arm-debug/14291/

It needs a handle scope for the context handle. Since the FastApiCallbackOptions struct doesn't have isolate on it in V8 12.4 on Node.js 22, use Isolate::TryGetCurrent() to get to the isolate needed for the handle scope creation and fallback to the slow callback if no isolate is entered.

This reverts commit 1a5acd0. Reason to revert: we have seen several crashes/unexpected JS behaviors with maglev on v22 (which ships V8 v12.4). The bugs lie in the codegen so it would be difficult for users to work around them or even figure out where the bugs are coming from. Some bugs are fixed in the upstream while some others probably remain. As v22 will get stuck with V8 v12.4 as LTS, it will be increasingly difficult to backport patches for them even if the bugs are fixed. So disable it by default on v22 to reduce the churn and troubles for users.

Added another commit to handle the missing handle scope assertion, this fixes the npm install crash locally for me - on the other hand does this mean that the fast path had been rarely called on v22 when maglev was still enabled by default? Yikes..

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

I was planning to land it on the next 22 proposal, but CI seems flaky. I will need to update v22.x-staging and then you'll need to rebase on top of it. If we get a green CI tomorrow (Monday 19) I can cherry-pick it.

@RafaelGSS I am not seeing any merge conflicts, has v22.x-staging been updated?

It needs a handle scope for the context handle. Since the FastApiCallbackOptions struct doesn't have isolate on it in V8 12.4 on Node.js 22, use Isolate::TryGetCurrent() to get to the isolate needed for the handle scope creation and fallback to the slow callback if no isolate is entered. PR-URL: #54384 Refs: #52797 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This reverts commit 1a5acd0. Reason to revert: we have seen several crashes/unexpected JS behaviors with maglev on v22 (which ships V8 v12.4). The bugs lie in the codegen so it would be difficult for users to work around them or even figure out where the bugs are coming from. Some bugs are fixed in the upstream while some others probably remain. As v22 will get stuck with V8 v12.4 as LTS, it will be increasingly difficult to backport patches for them even if the bugs are fixed. So disable it by default on v22 to reduce the churn and troubles for users. PR-URL: #54384 Refs: #52797 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 99e5abc...5243e32

Notable changes: lib: * (SEMVER-MINOR) add util.getCallSite() API (Rafael Gonzaga) #54380 repl: * doc-deprecate instantiating `node:repl` classes without `new` (Aviv Keller) #54842 src: * create handle scope in FastInternalModuleStat (Joyee Cheung) #54384 stream: * (SEMVER-MINOR) relocate the status checking code in the onwritecomplete (YoonSoo_Shin) #54032 tls: * (SEMVER-MINOR) add `allowPartialTrustChain` flag (Anna Henningsen) #54790 v8: * Revert "v8: enable maglev on supported architectures (Joyee Cheung) #54384 PR-URL: #54966

nodejs/node#54384

* chore: bump node in DEPS to v22.13.1 * chore: fixup GN build file * nodejs/node#55529 * nodejs/node#55798 * nodejs/node#55530 * module: simplify --inspect-brk handling nodejs/node#55679 * src: fix outdated js2c.cc references nodejs/node#56133 * crypto: include openssl/rand.h explicitly nodejs/node#55425 * build: use variable for crypto dep path nodejs/node#55928 * crypto: fix RSA_PKCS1_PADDING error message nodejs/node#55629 * build: use variable for simdutf path nodejs/node#56196 * test,crypto: make crypto tests work with BoringSSL nodejs/node#55491 * fix: suppress clang -Wdeprecated-declarations in libuv libuv/libuv#4486 * deps: update libuv to 1.49.1 nodejs/node#55114 * test: make test-node-output-v8-warning more flexible nodejs/node#55401 * [v22.x] Revert "v8: enable maglev on supported architectures" nodejs/node#54384 * fix: potential WIN32_LEAN_AND_MEAN redefinition c-ares/c-ares#869 * deps: update nghttp2 to 1.64.0 nodejs/node#55559 * src: provide workaround for container-overflow nodejs/node#55591 * build: use variable for simdutf path nodejs/node#56196 * chore: fixup patch indices * fixup! module: simplify --inspect-brk handling * lib: fix fs.readdir recursive async nodejs/node#56041 * lib: avoid excluding symlinks in recursive fs.readdir with filetypes nodejs/node#55714 This doesn't currently play well with ASAR - this should be fixed in a follow up * test: disable CJS permission test for config.main This has diverged as a result of our revert of src,lb: reducing C++ calls of esm legacy main resolve * fixup! lib: fix fs.readdir recursive async * deps: update libuv to 1.49.1 nodejs/node#55114 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

nodejs/node#54384

* chore: bump node in DEPS to v22.13.0 * chore: bump node in DEPS to v22.13.1 * src: move evp stuff to ncrypto nodejs/node#54911 * crypto: add Date fields for validTo and validFrom nodejs/node#54159 * module: fix discrepancy between .ts and .js nodejs/node#54461 * esm: do not interpret "main" as a URL nodejs/node#55003 * src: modernize likely/unlikely hints nodejs/node#55155 * chore: update patch indices * crypto: add validFromDate and validToDate fields to X509Certificate nodejs/node#54159 * chore: fixup perfetto patch * fix: clang warning in simdjson * src: add receiver to fast api callback methods nodejs/node#54408 * chore: fixup revert patch * fixup! esm: do not interpret "main" as a URL * fixup! crypto: add Date fields for validTo and validFrom * fix: move ArrayBuffer test patch * src: fixup Error.stackTraceLimit during snapshot building nodejs/node#55121 * fix: bad rebase * chore: fixup amaro * chore: address feedback from review * src: revert filesystem::path changes nodejs/node#55015 * chore: fixup GN build file * nodejs/node#55529 * nodejs/node#55798 * nodejs/node#55530 * module: simplify --inspect-brk handling nodejs/node#55679 * src: fix outdated js2c.cc references nodejs/node#56133 * crypto: include openssl/rand.h explicitly nodejs/node#55425 * build: use variable for crypto dep path nodejs/node#55928 * crypto: fix RSA_PKCS1_PADDING error message nodejs/node#55629 * build: use variable for simdutf path nodejs/node#56196 * test,crypto: make crypto tests work with BoringSSL nodejs/node#55491 * fix: suppress clang -Wdeprecated-declarations in libuv libuv/libuv#4486 * deps: update libuv to 1.49.1 nodejs/node#55114 * test: make test-node-output-v8-warning more flexible nodejs/node#55401 * [v22.x] Revert "v8: enable maglev on supported architectures" nodejs/node#54384 * fix: potential WIN32_LEAN_AND_MEAN redefinition c-ares/c-ares#869 * deps: update nghttp2 to 1.64.0 nodejs/node#55559 * src: provide workaround for container-overflow nodejs/node#55591 * build: use variable for simdutf path nodejs/node#56196 * chore: fixup patch indices * fixup! module: simplify --inspect-brk handling * lib: fix fs.readdir recursive async nodejs/node#56041 * lib: avoid excluding symlinks in recursive fs.readdir with filetypes nodejs/node#55714 This doesn't currently play well with ASAR - this should be fixed in a follow up * test: disable CJS permission test for config.main This has diverged as a result of our revert of src,lb: reducing C++ calls of esm legacy main resolve * fixup! lib: fix fs.readdir recursive async * deps: update libuv to 1.49.1 nodejs/node#55114 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

joyeecheung changed the title Revert "v8: enable maglev on supported architectures" [v22.x] Revert "v8: enable maglev on supported architectures" Aug 14, 2024

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2024

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Aug 14, 2024

richardlau approved these changes Aug 14, 2024

View reviewed changes

richardlau added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 14, 2024

RafaelGSS approved these changes Aug 14, 2024

View reviewed changes

legendecas approved these changes Aug 14, 2024

View reviewed changes

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

anonrig approved these changes Aug 14, 2024

View reviewed changes

targos approved these changes Aug 15, 2024

View reviewed changes

lpinca approved these changes Aug 15, 2024

View reviewed changes

mcollina approved these changes Aug 15, 2024

View reviewed changes

joyeecheung added 2 commits August 16, 2024 19:00

joyeecheung force-pushed the disable-maglev-22 branch from c38e136 to 1729d1e Compare August 16, 2024 17:01

joyeecheung mentioned this pull request Aug 16, 2024

Fix the bug caused by fast api changes in v22.5.0 and have a post-mortem #53934

Closed

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2024

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

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

RafaelGSS closed this Sep 6, 2024

RafaelGSS mentioned this pull request Sep 16, 2024

v22.9.0 proposal #54966

Merged

targos mentioned this pull request Sep 19, 2024

Maglev on x64 causes segmentation fault while running TypeScript #52797

Closed

targos added the dont-land-on-v20.x label Sep 22, 2024

panva removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2024

codebytere added a commit to electron/electron that referenced this pull request Jan 23, 2025

[v22.x] Revert "v8: enable maglev on supported architectures"

fcec7b9

nodejs/node#54384

codebytere added a commit to electron/electron that referenced this pull request Jan 28, 2025

[v22.x] Revert "v8: enable maglev on supported architectures"

edc4004

nodejs/node#54384

codebytere added a commit to electron/electron that referenced this pull request Jan 28, 2025

[v22.x] Revert "v8: enable maglev on supported architectures"

f1d8b14

nodejs/node#54384

codebytere added a commit to electron/electron that referenced this pull request Feb 3, 2025

[v22.x] Revert "v8: enable maglev on supported architectures"

3c6a40c

nodejs/node#54384

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025

[v22.x] Revert "v8: enable maglev on supported architectures"

df00fd4

nodejs/node#54384

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025

[v22.x] Revert "v8: enable maglev on supported architectures"

194236f

nodejs/node#54384

codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025

[v22.x] Revert "v8: enable maglev on supported architectures"

7deb613

nodejs/node#54384

[v22.x] Revert "v8: enable maglev on supported architectures" by joyeecheung · Pull Request #54384 · nodejs/node · GitHub

Uh oh!

Uh oh!

[v22.x] Revert "v8: enable maglev on supported architectures" - #54384

[v22.x] Revert "v8: enable maglev on supported architectures"#54384
joyeecheung wants to merge 2 commits into
nodejs:v22.x-stagingfrom
joyeecheung:disable-maglev-22

joyeecheung commented Aug 14, 2024 •
edited
Loading

Uh oh!

Uh oh!

github-actions Bot commented Aug 14, 2024

Uh oh!

nodejs-github-bot commented Aug 14, 2024

Uh oh!

nodejs-github-bot commented Aug 15, 2024

Uh oh!

mcollina left a comment

Uh oh!

nodejs-github-bot commented Aug 15, 2024

Uh oh!

richardlau commented Aug 15, 2024

Uh oh!

joyeecheung commented Aug 16, 2024

Uh oh!

nodejs-github-bot commented Aug 16, 2024

Uh oh!

RafaelGSS commented Aug 19, 2024

Uh oh!

joyeecheung commented Aug 19, 2024

Uh oh!

RafaelGSS commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

[v22.x] Revert "v8: enable maglev on supported architectures" by joyeecheung · Pull Request #54384 · nodejs/node · GitHub

Uh oh!

Uh oh!

[v22.x] Revert "v8: enable maglev on supported architectures" - #54384

[v22.x] Revert "v8: enable maglev on supported architectures"#54384joyeecheung wants to merge 2 commits intonodejs:v22.x-stagingnodejs/node:v22.x-stagingfrom joyeecheung:disable-maglev-22joyeecheung/node:disable-maglev-22Copy head branch name to clipboard

Conversation

joyeecheung commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

src: create handle scope in FastInternalModuleStat

Revert "v8: enable maglev on supported architectures"

Uh oh!

github-actions Bot commented Aug 14, 2024

Uh oh!

nodejs-github-bot commented Aug 14, 2024

Uh oh!

nodejs-github-bot commented Aug 15, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 15, 2024

Uh oh!

richardlau commented Aug 15, 2024

Uh oh!

joyeecheung commented Aug 16, 2024

Uh oh!

nodejs-github-bot commented Aug 16, 2024

Uh oh!

RafaelGSS commented Aug 19, 2024

Uh oh!

joyeecheung commented Aug 19, 2024

Uh oh!

RafaelGSS commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

[v22.x] Revert "v8: enable maglev on supported architectures"#54384
joyeecheung wants to merge 2 commits into
nodejs:v22.x-stagingfrom
joyeecheung:disable-maglev-22

joyeecheung commented Aug 14, 2024 •
edited
Loading