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

--concurrent-marking and --parallel-marking don't work and disabled by default #41012

Closed
MGorkov opened this issue Nov 29, 2021 · 6 comments
Closed

Comments

@MGorkov
Copy link

MGorkov commented Nov 29, 2021

Version

v16.13.0

Platform

Linux dev 4.18.0-305.el8.x86_64 #1 SMP Thu Apr 29 08:54:30 EDT 2021 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

node --concurrent-marking
node --parallel-marking
node --concurrent-marking --parallel-marking

How often does it reproduce? Is there a required condition?

always

What is the expected behavior?

node runs without error

What do you see instead?

Fatal error in , line 0
Check failed: !FLAG_concurrent_marking && !FLAG_parallel_marking.

FailureMessage Object: 0x7ffeda8b29f0
1: 0xb6f151 [node]
2: 0x1bf56f4 V8_Fatal(char const*, ...) [node]
3: 0xe603ad [node]
4: 0xeb31ae v8::internal::Heap::SetUp() [node]
5: 0xe3cd6b v8::internal::Isolate::Init(v8::internal::SnapshotData*, v8::internal::SnapshotData*, bool) [node]
6: 0x124af66 [node]
7: 0xd1314e v8::Isolate::Initialize(v8::Isolate*, v8::Isolate::CreateParams const&) [node]
8: 0xb4451c node::NodeMainInstance::NodeMainInstance(v8::Isolate::CreateParams*, uv_loop_s*, node::MultiIsolatePlatform*, std::vector<std::string, std::allocatorstd::string > const&, std::vector<std::string, std::allocatorstd::string > const&, std::vector<unsigned long, std::allocator > const*) [node]
9: 0xac67d3 node::Start(int, char**) [node]
10: 0x7fe2a3721493 __libc_start_main [/lib64/libc.so.6]
11: 0xa3bfbc [node]

Additional information

these options are disabled by default:
node --v8-options | grep -A1 -E '(--parallel-marking|--concurrent-marking)'

--concurrent-marking (use concurrent marking)
type: bool default: false
--parallel-marking (use parallel marking in atomic pause)
type: bool default: false

@targos
Copy link
Member

targos commented Nov 29, 2021

Concurrent marking is not enabled in Node.js builds. Let's ask @nodejs/v8 if it's something stable enough that we should enable it. If yes, since which V8 version?

@victorgomes
Copy link

@mlippautz

@MGorkov
Copy link
Author

MGorkov commented Nov 29, 2021

Concurrent marking is not enabled in Node.js builds. Let's ask @nodejs/v8 if it's something stable enough that we should enable it. If yes, since which V8 version?

It is unstable only in new v9.* versions? It looks like everything was fine in the previous nodejs version, with old V8

@targos
Copy link
Member

targos commented Nov 29, 2021

Refs: v8/v8@acf5e1a

The issue is that the v8_enable_atomic_object_field_writes and v8_enable_atomic_marking_state compile options were not ported to Node.js, so they end up being both disabled. Should we enable them? @ulan

@ulan
Copy link
Contributor

ulan commented Nov 29, 2021

@targos: yes, please go ahead with enabling them in Node. Both v8_enable_atomic_object_field_writes and v8_enable_atomic_marking_state are safe and intended to be enabled by default. Sorry for breaking Node, I thought the build rules are generated automatically from V8's GN.

@targos targos self-assigned this Nov 29, 2021
@mlippautz
Copy link

Hey @ulan :)

Yes, it's safe to enable and the concurrent/parallel marking just moved behind those flags.

targos added a commit to targos/node that referenced this issue Nov 29, 2021
It was unintentionally disabled during a V8 update.

Fixes: nodejs#41012
targos added a commit to targos/node that referenced this issue Dec 3, 2021
It was unintentionally disabled during a V8 update.

Fixes: nodejs#41012
targos added a commit to targos/node that referenced this issue Dec 5, 2021
It was unintentionally disabled during a V8 update.

Fixes: nodejs#41012
@targos targos closed this as completed in ca353de Dec 6, 2021
danielleadams pushed a commit that referenced this issue Dec 13, 2021
It was unintentionally disabled during a V8 update.

Fixes: #41012

PR-URL: #41013
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Dec 14, 2021
It was unintentionally disabled during a V8 update.

Fixes: #41012

PR-URL: #41013
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
It was unintentionally disabled during a V8 update.

Fixes: #41012

PR-URL: #41013
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
It was unintentionally disabled during a V8 update.

Fixes: #41012

PR-URL: #41013
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
It was unintentionally disabled during a V8 update.

Fixes: nodejs#41012

PR-URL: nodejs#41013
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
It was unintentionally disabled during a V8 update.

Fixes: #41012

PR-URL: #41013
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos removed their assignment Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants