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

[v12.x] cli: allow --jitless V8 flag in NODE_OPTIONS #32594

Closed

Conversation

MylesBorins
Copy link
Member

This work is modeled on #30094 which allowed
--disallow-code-generation-from-strings in NODE_OPTIONS.

The --jitless v8 option has been supported since 12.0.0. As a v8
option, node automatically picks it up, but there have been a few issues
that were resolved by simply telling users about the option: #26758,

This PR:

  • allows --jitless in NODE_OPTIONS
  • documents --jitless
  • moves --experimental-loader=module to locally restore alphabetical
    order in option documentation

Refs: #30094
Refs: #26758
Refs: #28800

PR-URL: #32100
Reviewed-By: Richard Lau riclau@uk.ibm.com
Reviewed-By: Colin Ihrig cjihrig@gmail.com
Reviewed-By: Sam Roberts vieuxtech@gmail.com
Reviewed-By: Gus Caplan me@gus.host
Reviewed-By: Shelley Vohr codebytere@gmail.com
Reviewed-By: David Carlier devnexen@gmail.com
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Michael Dawson michael_dawson@ca.ibm.com
Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

This work is modeled on nodejs#30094 which allowed
`--disallow-code-generation-from-strings` in `NODE_OPTIONS`.

The `--jitless` v8 option has been supported since 12.0.0. As a v8
option, node automatically picks it up, but there have been a few issues
that were resolved by simply telling users about the option: nodejs#26758,

This PR:
  - allows `--jitless` in `NODE_OPTIONS`
  - documents `--jitless`
  - moves `--experimental-loader=module` to locally restore alphabetical
    order in option documentation

Refs: nodejs#30094
Refs: nodejs#26758
Refs: nodejs#28800

PR-URL: nodejs#32100
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v12.x labels Apr 1, 2020
@nodejs-github-bot
Copy link
Collaborator

Co-Authored-By: Anna Henningsen <github@addaleax.net>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30326/ (:heavy_check_mark:)

MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
This work is modeled on #30094 which allowed
`--disallow-code-generation-from-strings` in `NODE_OPTIONS`.

The `--jitless` v8 option has been supported since 12.0.0. As a v8
option, node automatically picks it up, but there have been a few issues
that were resolved by simply telling users about the option: #26758,

This PR:
  - allows `--jitless` in `NODE_OPTIONS`
  - documents `--jitless`
  - moves `--experimental-loader=module` to locally restore alphabetical
    order in option documentation

Refs: #30094
Refs: #26758
Refs: #28800

Backport-PR-URL: #32594
PR-URL: #32100
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Member Author

landed in aec2226

@guybedford
Copy link
Contributor

It appears the --jitless flag disables WebAssembly support entirely currently. See the following v8 source comment from v8/src/init/v8.cc:

  // Do not expose wasm in jitless mode.
  //
  // Even in interpreter-only mode, wasm currently still creates executable
  // memory at runtime. Unexpose wasm until this changes.
  // The correctness fuzzers are a special case: many of their test cases are
  // built by fetching a random property from the the global object, and thus
  // the global object layout must not change between configs. That is why we
  // continue exposing wasm on correctness fuzzers even in jitless mode.
  // TODO(jgruber): Remove this once / if wasm can run without executable
  // memory.

@MylesBorins this unfortunately makes this flag a blocker for having any Wasm execution at all and hence #33416.

@nodejs/v8 is there a roadmap for --jitless to support Web Assembly at all?

Alternatively would we be able to consider disabling this flag in Node.js to land internal Wasm code until a solution for Web Assembly is found?

@verwaest
Copy link
Contributor

verwaest commented Jul 2, 2020

At this time there are no plans (that I'm aware of) to support wasm jitless. Interpreting wasm will very likely always be quite a bit slower than compiling and running. JS jitless made sense even where we can JIT js since a lot of code doesn't get hot and actually finishes faster if we just run it as bytecode. And it's definitely leaner.

What's the reason you want to run node jitless?

Just for my own understanding, not suggesting we're planning on building something like this: I presume you're running the wasm modules on an installation you control, and could AOT wasm to native code rather than JIT?

What's the main reason wasm is used instead of native node modules?

@guybedford
Copy link
Contributor

@verwaest the problem is if Node.js core tries to adopt Wasm internally then this flag will break Node.js core. So this may be a blocker for that. Yes running native or JS sounds preferable then - although worth noting this constraint on Node.js core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants