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

process: introduce process.availableV8flags #40791

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 12, 2021

I'm opening this so at least two things can be discussed:

  1. Does this feature meet the criteria for inclusion in Node.js? (See #issuecomment-966772902.)
  2. Is there a more elegant way to implement this because I don't think anyone is going to be comfortable with that big hard-coded array?

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Nov 12, 2021
@Trott Trott added the v8 engine Issues and PRs related to the V8 dependency. label Nov 12, 2021
@Trott Trott force-pushed the availablev8flags branch 2 times, most recently from d7b3392 to 0368161 Compare November 12, 2021 01:41
@Trott Trott changed the title process: introduce process.availableV8flags() process: introduce process.availableV8flags Nov 12, 2021
@mscdex
Copy link
Contributor

mscdex commented Nov 12, 2021

It would be good to get input from someone on the V8 team to see if they would be interested in making the list of flags available programmatically in some way.

@Trott
Copy link
Member Author

Trott commented Nov 12, 2021

The reason for wanting to include this in core is to make https://www.npmjs.com/package/v8flags obsolete which has over 3 million weekly downloads and has comments in the source code like this

// this entire module is depressing. i should have spent my time learning
// how to patch v8 so that these options would just be available on the
// process object.

...and this:

// i can't wait for the day this whole module is obsolete because these
// options are available on the process object. this executes node with
// `--v8-options` and parses the result, returning an array of command
// line flags.

/cc @phated

This seems like a decent test case for our recently-added guide on whether to include things in core (which is about modules but there is an open PR to make it about functionality in general).

@cjihrig
Copy link
Contributor

cjihrig commented Nov 12, 2021

Would the v8 module be a better home for this than process?

@Trott
Copy link
Member Author

Trott commented Nov 12, 2021

Copying the criteria from #40601 and putting checkmarks next to the ones that apply or might apply. 👇

Strong arguments for including a component in core

  • The component provides functionality that is standardized (such as a
    Web API) and overlaps with existing functionality.
  • The component can only be implemented in core.
  • The component can only be implemented in a performant way in core.
  • Developer experience is significantly improved if the component is in core.
  • The component provides functionality that can be expected to solve at
    least one common use case Node.js users face.
  • The component requires native bindings. Inclusion in core enables
    utility across operating systems and architectures without requiring
    users to have a native compilation toolchain.
  • Part or all of the component will also be re-used or duplicated in core.

Strong arguments against including a component in core

  • None of the arguments list in the previous section apply.
  • The component has a license that prohibits Node.js from including it in core
    without also changing its own license.
  • There is already similar functionality in core and adding the component will
    provide a second API to do the same thing.
  • A component (or/and the standard it is based on) is deprecated and there is
    a non-deprecated alternative.
  • The component is evolving quickly and inclusion in core will require frequent
    API changes.

@Trott
Copy link
Member Author

Trott commented Nov 12, 2021

Would the v8 module be a better home for this than process?

Perhaps. I chose process because this property shares an API and a lot of underlying code with process.allowedNodeEnvironmentFlags. So using process allowed me to easily put around 75 lines of code into a shared class. I suppose it can still be in a shared internal module if it is determined that v8 is the right place for it.

@phated
Copy link
Contributor

phated commented Nov 12, 2021

Thanks @Trott! I yearn for a day when all our hacks upon hacks are obsolete ❤️

@Trott
Copy link
Member Author

Trott commented Nov 12, 2021

It would be good to get input from someone on the V8 team to see if they would be interested in making the list of flags available programmatically in some way.

@nodejs/v8

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 12, 2021
@Trott Trott marked this pull request as ready for review November 12, 2021 14:57
@Trott Trott added the v8 module Issues and PRs related to the "v8" subsystem. label Nov 12, 2021
@Trott
Copy link
Member Author

Trott commented Nov 12, 2021

@nodejs/process

doc/api/process.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member Author

Trott commented Nov 12, 2021

An additional complication to hard-coding the flags (aside from the code smell) is that some flags don't exist on all platforms (such as multi-mapped-mock-allocator).

@devsnek
Copy link
Member

devsnek commented Nov 12, 2021

this wouldn't be hard to expose from v8, they're in a big c array: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/flags/flags.cc;drc=861cbcc13edb6281c5d1c7976553840136b1f897;l=379

@targos
Copy link
Member

targos commented Nov 14, 2021

I really don't like the hard-coded approach, because

  1. We have to maintain the list
  2. The list depends on V8 compile options (for example, there are more flags in debug mode)

@Trott
Copy link
Member Author

Trott commented Nov 14, 2021

I really don't like the hard-coded approach, because

  1. We have to maintain the list
  2. The list depends on V8 compile options (for example, there are more flags in debug mode)

Yeah, I expect that literally no one likes the hard-coded list approach, including me.

@benjamingr
Copy link
Member

Can't we add a simple script that extracts all the flags from the file Gus sent as an alternative approach?

@camillobruni
Copy link
Contributor

I would prefer not exposing all flags on the V8-side as we open up an additional API surface.
It would probably make sense to have an API for an important subset – for instance we're adding a check for --jitless.

To understand the problem a bit better:

  • Are there very common flags folks are mostly interested in?
  • What's the most common use-case of the v8flags module?

Many V8-flags are just for debugging and internal development and should likely never be exposed in userland (and we obviously make our lives on the V8-side easy by not thinking too hard about which ones should be exposed)

@@ -255,6 +256,82 @@ const replaceUnderscoresRegex = /_/g;
const leadingDashesRegex = /^--?/;
const trailingValuesRegex = /=.*$/;

class FlagsSet extends Set {
Copy link
Member

Choose a reason for hiding this comment

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

If the the reason for putting the feature under process is code-sharing, this could be just split into a different file. I also think that v8 is a more appropriate place for it.

function buildAvailableV8Flags() {

const v8Flags = [
'--abort-on-contradictory-flags',
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look very reliable to me - V8 flags come and go and there are no guarantees about them. If possible we should do this programmatically, e.g. having a script that put them into a json file during every update and loading the json during build time, or just request v8 to expose a list of available flags through embedder API (that might be difficult to get accepted though).

Copy link
Member Author

@Trott Trott Nov 15, 2021

Choose a reason for hiding this comment

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

Yup, since the flags can come or go in any release, I added a test to check these flags against the output of node --v8-options. Unfortunately, as if there wasn't already enough reason to be skeptical about a hardcoded list of flags, the test fails because some flags aren't available on some platforms. And that's not even getting into problems when Node.js is built with different configuration flags. So this won't work at all without a bunch of extra logic. So yeah, the hardcoded array is a total nonstarter which is not surprising.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 15, 2021

I wonder if an alternative approach would be to provide an API that shows whether certain commonly used v8 flags that are not available (and if we don't know, or if it's available for now which also means they could go away soon, return undefined). IIUC the use case for such API is usually to check if a feature isn't supported in a particular build. We could have a set that records flags that are no longer available instead, and record the status of more flags in it as users request. Although in that case this still seems more like what a user land module should offer instead of something in core.

@Trott
Copy link
Member Author

Trott commented Nov 15, 2021

Can't we add a simple script that extracts all the flags from the file Gus sent as an alternative approach?

No because they are not static. Some, for example, are platform-dependent.

@Trott
Copy link
Member Author

Trott commented Nov 15, 2021

  • What's the most common use-case of the v8flags module?

@phated Can you answer that question?

I wonder if an API that accepts a string or an array of strings and returns a boolean indicating whether or not it is a valid V8 flag would meet the need. This avoids exposing an explicit list of V8 flags but allows tools to determine if something they've been handed is or isn't a V8 flag, assuming there's a compelling use case for that.

@camillobruni
Copy link
Contributor

Having something like bool v8::V8::Flags::IsSet(const char*) is roughly equivalent to providing a full list of all flags.
I prefer having a small explicit set of getters like bool v8::V8::Flags::IsFlagXXX().
We should probably allow simpler V8 setup via v8::V8::InitParams::Flags that are passed explicitly to v8::V8::Initialize to make it explicit that those flags are official (and only set once during V8 startup)

@phated
Copy link
Contributor

phated commented Nov 15, 2021

Can you answer that question?

I don't have any metrics on the flags people would use. We purposely expose them all so any flags that node or v8 accepts can be passed through a CLI tool, such that gulp --experimental-wasm-bigint will be translated to node --experimental-wasm-bigint bin/gulp.js

@camillobruni
Copy link
Contributor

FYI, I've been slowly working towards making V8 flags read-only.
Many (if not all) of the V8 flags should not be changed after calling v8::V8::Initialize

@Trott Trott added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Jun 17, 2022
@Trott Trott closed this Mar 7, 2024
@phated
Copy link
Contributor

phated commented Mar 13, 2024

Forever alone 😭

@Trott
Copy link
Member Author

Trott commented Mar 14, 2024

Forever alone 😭

Yeah, sorry. I still support this, but I'm not nearly as involved in Node.js as I once was and this will require someone willing to put in the work, both politically and technically. (Er....unless someone has exposed something within V8 already that we can leverage?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that need assistance from volunteers or PRs that need help to proceed. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency. v8 module Issues and PRs related to the "v8" subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.