option --prof documentation help added

[Krank2me]

Hi, I trying to add prof documentation help on node_options.cc file, but when I test running show the following error:

../src/node_options.cc:405:35: error: no member named 'syntax_prof_only' in
'node::EnvironmentOptions'
&EnvironmentOptions::syntax_prof_only);

So I don't know if I have to add 'syntax_prof_only' somewhere before.

Thaks for you help

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[mmarchini]

I think you need to add it to EnvironmentOptions. I'm also not entirely sure if this would work as is since this is a V8 flag and adding it with AddOption might hijack the options passed to V8 (or maybe it won't, I'm not super familiar with that part of the code).

With that being said, I'm still opposed to adding any V8 options as Node.js options (which is what this PR is doing) for the reasons outlined here: #34281 (comment) and #34281 (comment). If the goal is to have --prof in --help we could have a "Commonly used V8 options" section in our --help instead.

[Krank2me]

I think you need to add it to EnvironmentOptions. I'm also not entirely sure if this would work as is since this is a V8 flag and adding it with AddOption might hijack the options passed to V8 (or maybe it won't, I'm not super familiar with that part of the code).

With that being said, I'm still opposed to adding any V8 options as Node.js options (which is what this PR is doing) for the reasons outlined here: #34281 (comment) and #34281 (comment). If the goal is to have --prof in --help we could have a "Commonly used V8 options" section in our --help instead.

I think you need to add it to EnvironmentOptions. I'm also not entirely sure if this would work as is since this is a V8 flag and adding it with AddOption might hijack the options passed to V8 (or maybe it won't, I'm not super familiar with that part of the code).

With that being said, I'm still opposed to adding any V8 options as Node.js options (which is what this PR is doing) for the reasons outlined here: #34281 (comment) and #34281 (comment). If the goal is to have --prof in --help we could have a "Commonly used V8 options" section in our --help instead.

Hi @mmalecki thank you for you answer, I thought something similar, but I just searched other option help into [EnvironmentOptions] but I didn't find it 😕

[addaleax]

Suggested change

	+ "Generate V8 profiler output.",
	+ &EnvironmentOptions::syntax_prof_only);
	"Generate V8 profiler output.",
	V8Option{});

[Krank2me]

hello @addaleax I added your suggested change but when I running test show the following error:

../src/node_options.cc:405:1: error: invalid argument type 'node::options_parser::OptionsParser<node::EnvironmentOptions>::V8Option' to unary expression V8Option{});

thanks for you help :)

[addaleax]

@Krank2me As you can see, the Github actions fail because this code currently doesn’t compile – if I apply the suggestion here, it passes locally for me.

If you are having trouble with this, it might be good if you could upload the relevant parts of the compile output somewhere

[Krank2me]

@addaleax you're right, thank you so much for you help :)

[addaleax]

@mmarchini We do already document a few V8 flags this way, in particular --abort-on-uncaught-exception, --disallow-code-generation-from-strings, --huge-max-old-generation-size and --jitless – I think that’s fair

[mmarchini]

--abort-on-uncaught-exception is documented because we have specific behavior on Node.js (it's more of a hybrid flag today instead of a purely V8 flag), and the other flags seem safer/more stable than --prof (V8 has explicitly said to us many times that this flag is not supported and could break at any point). I don't feel strong enough about it to block, but I still think we should at the very least differentiate V8 flags from Node.js flags in our documentation (doesn't need to be implemented in this PR though)

[juanarbol]

Nice, thank you!

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[gireeshpunathil]

landed in eb24573 , thanks for the contribution! 🎉