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

option --prof documentation help added #34991

Closed
wants to merge 2 commits into from

Conversation

Krank2me
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 30, 2020
@mmarchini
Copy link
Contributor

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
Copy link
Contributor Author

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 😕

Comment on lines 404 to 405
+ "Generate V8 profiler output.",
+ &EnvironmentOptions::syntax_prof_only);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

mmarchini commented Sep 2, 2020

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

Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@gireeshpunathil gireeshpunathil added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 4, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

gireeshpunathil pushed a commit that referenced this pull request Nov 5, 2020
PR-URL: #34991
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@gireeshpunathil
Copy link
Member

landed in eb24573 , thanks for the contribution! 🎉

danielleadams pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #34991
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
PR-URL: #34991
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #34991
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #34991
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.

6 participants