Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Aug 14, 2024

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

@joyeecheung joyeecheung changed the title Revert "v8: enable maglev on supported architectures" [v22.x] Revert "v8: enable maglev on supported architectures" Aug 14, 2024
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2024
@nodejs-github-bot 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 v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Aug 14, 2024
@richardlau richardlau added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 14, 2024
Copy link
Contributor

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.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

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.
@joyeecheung
Copy link
Member Author

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..

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 16, 2024
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS
Copy link
Member

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.

@joyeecheung
Copy link
Member Author

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

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2024
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

@joyeecheung 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 Sep 5, 2024
@joyeecheung
Copy link
Member Author

The CI is green, although actually it looks like the commit queue doesn't work for staging branches, so it'll need to be merged manually by a releaser? @RafaelGSS

@RafaelGSS
Copy link
Member

I'll land it asap

RafaelGSS pushed a commit that referenced this pull request Sep 6, 2024
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>
RafaelGSS pushed a commit that referenced this pull request Sep 6, 2024
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>
@RafaelGSS
Copy link
Member

Landed in 99e5abc...5243e32

@RafaelGSS RafaelGSS closed this Sep 6, 2024
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
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: TODO
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
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
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
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
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
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
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
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
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
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
RafaelGSS added a commit that referenced this pull request Sep 16, 2024
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
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
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
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
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
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
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
RafaelGSS added a commit that referenced this pull request Sep 17, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. 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. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. v22.x v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants