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

node --prof-process is broken #19044

Closed
mourner opened this issue Feb 27, 2018 · 28 comments
Closed

node --prof-process is broken #19044

mourner opened this issue Feb 27, 2018 · 28 comments

Comments

@mourner
Copy link

mourner commented Feb 27, 2018

  • Version: v9.6.1
  • Platform: macOS

node --prof-process doesn't work in the current Node version.

→ node -v
v9.6.1
→ node --prof-process
evalmachine.<anonymous>:31
module.exports = { versionCheck };
^

ReferenceError: module is not defined
    at evalmachine.<anonymous>:31:1
    at internal/v8_prof_processor.js:41:28
    at NativeModule.compile (bootstrap_node.js:653:7)
    at Function.NativeModule.require (bootstrap_node.js:598:18)
    at startup (bootstrap_node.js:139:20)
    at bootstrap_node.js:665:3

Same result when pointing to an isolate file.

@addaleax
Copy link
Member

You’re right, python tools/test.py tick-processor is also failing for me on master.

I think this might be coming from 99d693d, /cc @MylesBorins

Do you think you could try this fix?

--- a/lib/internal/v8_prof_processor.js
+++ b/lib/internal/v8_prof_processor.js
@@ -34,9 +34,9 @@ if (process.platform === 'darwin') {
   tickArguments.push('--windows');
 }
 tickArguments.push.apply(tickArguments, process.argv.slice(1));
-script = `(function(require) {
+script = `(function(module, require) {
   arguments = ${JSON.stringify(tickArguments)};
   function write (s) { process.stdout.write(s) }
   ${script}
 })`;
-vm.runInThisContext(script)(require);
+vm.runInThisContext(script)(module, require);

@fhinkel
Copy link
Member

fhinkel commented Feb 28, 2018

🤔 I thought we had just fixed --prof-process. Does it work on master?

@fhinkel
Copy link
Member

fhinkel commented Feb 28, 2018

#18451, @cjihrig fixed it.

@mourner
Copy link
Author

mourner commented Feb 28, 2018

@fhinkel as far as I can see, it was broken again a week later — it's a different issue. I guess some new tests for flag would help :)

addaleax added a commit to addaleax/node that referenced this issue Feb 28, 2018
Make the script not error out immediately because of a missing
pseudo-global.
(Note that it seems like tests are still broken on `master`.)

Fixes: nodejs#19044
Refs: nodejs#18623
@addaleax
Copy link
Member

Does it work on master?

When we were testing for #18623, it didn’t work on master to begin with. :/

I guess some new tests for flag would help :)

We do have tests, but they’re resource-consuming and aren’t run as part of the standard test suite – I guess they just weren’t run the last time when changes to this were landed.

@mourner I’ve opened #19059 with the above patch, could you try to check that out?

@mourner
Copy link
Author

mourner commented Feb 28, 2018

@addaleax thanks a lot for the fix! I don't have Node source code set up locally but can try checking if no one else beats me to it in the nearest days.

@aabuelenin
Copy link

@addaleax thanks for the solution, it does work in the sense that I stopped getting the error, but --prof-process won't produce any output, it just keeps running without giving any feedback. I ran it against a 4MB isolation file and left it for an hour. Guess we need a profiler to profile node profiler :)

addaleax added a commit to addaleax/node that referenced this issue Mar 5, 2018
Make the script not error out immediately because of a missing
pseudo-global.
(Note that it seems like tests are still broken on `master`.)

PR-URL: nodejs#19059
Fixes: nodejs#19044
Refs: nodejs#18623
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@mourner
Copy link
Author

mourner commented Mar 5, 2018

@addaleax thanks a lot for the merge! Given the comment above by @aabuelenin, can we reopen the ticket until --prof-process is confirmed to work?

@gibfahn gibfahn reopened this Mar 5, 2018
@mourner
Copy link
Author

mourner commented Mar 11, 2018

Hey, any updates on this? Did anyone get --prof-process to work with Node v9?

@timdp
Copy link
Contributor

timdp commented Mar 11, 2018

@mourner For me at least, it's still broken because of #19260. It looks like the last working version was 9.5.0.

@guymguym
Copy link
Contributor

guymguym commented Mar 15, 2018

btw it seems to be broken on v8.10.0 as well.
node --prof-process is stuck consuming 100% cpu...
works fine on v8.9.4...

actually, this might be a different issue as it gets stuck and not failing on ReferenceError as in this case...

UPDATE: apparently the issue happens when I use node --prof with v8.10.0 to record the profiling information and then --prof-process gets stuck with either v8.10.0 or v8.9.4. However, when I use v8.9.4 to create the profiler file, both versions succeed with --prof-process...

OK nevermind, this is issue #19199.

@gibfahn
Copy link
Member

gibfahn commented Mar 15, 2018

So is this the same issue as #19199 or is it likely to be a different problem?

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Make the script not error out immediately because of a missing
pseudo-global.
(Note that it seems like tests are still broken on `master`.)

PR-URL: nodejs#19059
Fixes: nodejs#19044
Refs: nodejs#18623
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit that referenced this issue Jun 30, 2018
Make the script not error out immediately because of a missing
pseudo-global.
(Note that it seems like tests are still broken on `master`.)

PR-URL: #19059
Fixes: #19044
Refs: #18623
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
rvagg pushed a commit that referenced this issue Aug 16, 2018
Make the script not error out immediately because of a missing
pseudo-global.
(Note that it seems like tests are still broken on `master`.)

PR-URL: #19059
Fixes: #19044
Refs: #18623
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@joseph-jja
Copy link

joseph-jja commented Sep 6, 2018

node --prof-process is not working on node 8.11.x :( it just hangs, since this is LTS is there any fix in the pipeline coming?

@MylesBorins
Copy link
Contributor

is it fixed in the 8.12.0 rc?

https://nodejs.org/download/rc/v8.12.0-rc.6/

@joseph-jja
Copy link

I get this message when I used that version on a mac

ReferenceError: printErr is not defined
at TickProcessor.printError (evalmachine.:2770:3)
at TickProcessor.LogReader.processLog_ (evalmachine.:2522:12)
at TickProcessor.LogReader.processLogLine (evalmachine.:2405:10)
at TickProcessor.processLogFile (evalmachine.:2798:10)
at evalmachine.:3980:15
at internal/v8_prof_processor.js:41:28
at NativeModule.compile (bootstrap_node.js:613:7)
at Function.NativeModule.require (bootstrap_node.js:558:18)
at startup (bootstrap_node.js:150:20)
at bootstrap_node.js:625:3

@addaleax
Copy link
Member

@joseph-jja That looks like a different error – can you open a new issue, ideally with a full reproduction (should be doable)?

@joseph-jja
Copy link

Hi @addaleax I got this error when running node --prof-process on the output from --prof. Bottom line is --prof-process is broken in node 8.11+ AFAICS and 8.12 didn't fix it for me

@addaleax
Copy link
Member

@joseph-jja Can you open a new issue, please? I get that the bottom line is the same, but it would really be nice to not piggy-back on existing issues. That also gives the problem you are experiencing more visibility.

@mourner
Copy link
Author

mourner commented Sep 12, 2018

As far as I'm concerned, the original issue is resolved — --node-prof works as expected in Node v10.10. So let's close this and create new issues for any related problems.

@mourner mourner closed this as completed Sep 12, 2018
@joseph-jja
Copy link

So does this mean prof process won't be fixed in node 8.x?

@MylesBorins
Copy link
Contributor

@joseph-jja we are interested in fixing it. Can you please open a new issue explaining how to reproduce the error on 8.x

@hubiierik
Copy link

isolate-0x56385f9c7470-v8.log.gz

Hangs on v8.10.0

@MylesBorins
Copy link
Contributor

I just attempted to run the below on v8.14.0 and it worked fine. Am I missing something

node --pro -e "for(var i=0;i<1E7;i++);"
node --prof-process isolate*.log

@joseph-jja
Copy link

v8.14.0 generates isolate logs and when I prof process doing the below:

node --prof-process isolatexxxxx.log > logs/prof-1.txt

it spits out a bunch of errors to the console. What does make it into the file does look like it did some profiling.

@joseph-jja
Copy link

v8.14.0 also generates messages about Code move event for unknown code: with an address which do not end up in the isolate logs, which used to. Not sure they belong there. Also for each profile log it spits out this message:

(node:15644) [BROKEN_PROFILE_FILE] Warning: Profile file isolate-0x103000000-v8.log is broken
"tick,0x7fff91dedf49,117042369,1,0x10003965a,3,0x1001f27b0,0x24708cf3be16,0x24708cf3b8cd,0x24704dad98b7,0x2" at the file end is broken

(node:15735) [BROKEN_PROFILE_FILE] Warning: Profile file isolate-0x103800000-v8.log is broken
"tick,0x10062eaa7,117041854,0,0x7fff" at the file end is broken

I suppose somewhere after the tick there is a tock?

@mmarchini
Copy link
Contributor

@joseph-jja I believe the issue you might be looking for is #19199.

By the way, I tried --prof and --prof-process on several v8.x versions (including the ones where issues were reported) and didn't get any errors. We need reproducibles, if you could share a minimal example (with code) where this issue is happening it would be awesome :)

@MylesBorins
Copy link
Contributor

I'm going to reopen this issue until we have a fix for 8.x

@MylesBorins MylesBorins reopened this Nov 29, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

Node.js v8.x has reached the end-of-life and won't receive any fixes anymore. I am closing this since this issue only applies to Node.js v8.x. No matter if you run into this issue or not, please update to a newer Node.js version in case you still use v8.x.

@BridgeAR BridgeAR closed this as completed Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.