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

--prof-process CLI args no longer work in 10.10.0 #22786

Closed
bengl opened this issue Sep 10, 2018 · 6 comments
Closed

--prof-process CLI args no longer work in 10.10.0 #22786

bengl opened this issue Sep 10, 2018 · 6 comments
Labels
cli Issues and PRs related to the Node.js command line interface.

Comments

@bengl
Copy link
Member

bengl commented Sep 10, 2018

Prior Node 10.10.0, the node --prof-process tool could be passed arguments altering its behavior. For example, --preprocess could be passed to deliver JSON data, and -j to only show JS VM ticks.

It seems that in Node 10.10.0, none of the --prof-process-specific arguments work anymore.

This currenly breaks 0x, clinic flame and probably some other perf-analysis tools.

e.g.

Node 10.9.0
$ node --prof-process --preprocess isolate-0x3b065a0-v8.log
# ... tons of useful JSON data
Node 10.10.0
$ node --prof-process --preprocess isolate-0x3b065a0-v8.log
node: bad option: --preprocess

I think this might be because of #22490. (/cc @addaleax)

@targos
Copy link
Member

targos commented Sep 10, 2018

Are those arguments documented somewhere? I didn't know about them before you opened this issue

@joyeecheung
Copy link
Member

joyeecheung commented Sep 10, 2018

This looks like an argument being passed down to the v8 scripts by v8_prof_processor.js, it's supposed to be treated as an exec argument but the file is run by us when --prof-process is on

tickArguments.push.apply(tickArguments, process.argv.slice(1));
script = `(function(module, require) {
arguments = ${JSON.stringify(tickArguments)};
function write (s) { process.stdout.write(s) }
function printErr(err) { console.error(err); }
${script}
})`;

@addaleax
Copy link
Member

This currenly breaks 0x, clinic flame and probably some other perf-analysis tools.

Would clinic be a candidate for CITGM?

I think this might be because of #22490. (/cc @addaleax)

#22392 could be the culprit too – either way, this is probably my fault.

@joyeecheung
Copy link
Member

Maybe we should just break out of the argument parse loop once we reach --prof-process? That's how the user would've imagined how it works?

@addaleax addaleax added the cli Issues and PRs related to the Node.js command line interface. label Sep 10, 2018
addaleax added a commit to addaleax/node that referenced this issue Sep 10, 2018
Make sure that options after `--prof-process` are not treated
as Node.js options.

Fixes: nodejs#22786
@addaleax
Copy link
Member

Proposed fix in #22790, would be cool if you could try that?

@jasnell
Copy link
Member

jasnell commented Sep 10, 2018

We can definitely explore getting clinic into citgm

targos pushed a commit that referenced this issue Sep 16, 2018
Make sure that options after `--prof-process` are not treated
as Node.js options.

Fixes: #22786

PR-URL: #22790
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos closed this as completed in 23f8b02 Sep 16, 2018
targos pushed a commit that referenced this issue Sep 19, 2018
Make sure that options after `--prof-process` are not treated
as Node.js options.

Fixes: #22786

PR-URL: #22790
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this issue Sep 20, 2018
Make sure that options after `--prof-process` are not treated
as Node.js options.

Fixes: #22786

PR-URL: #22790
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Issues and PRs related to the Node.js command line interface.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants