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

src: Add new perf flags in NODE_OPTIONS #25565

Closed
wants to merge 2 commits into from

Conversation

tomgco
Copy link
Contributor

@tomgco tomgco commented Jan 18, 2019

I have added the following flags to be whitelisted for inclusion in NODE_OPTIONS:

  • --perf-basic-prof-only-functions
  • --perf-prof-unwinding-info

docs, src and tests having been updated.

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

Tests are failing on my machine, but they don't seem relevant to the changes I have made, also the tests on CI have passed.

=== release test-exception ===                                               
Path: node-report/test-exception                      
undefined:223                      
    "LESS_TERMCAP_me": "",
                        ^                                      
                                                                         
SyntaxError: Unexpected token n JSON at position 6966
    at JSON.parse (<anonymous>)                                             
    at Object.validateContent (/home/tomgco/Projects/node/test/common/report.js:36:23)            
    at fs.readFile (/home/tomgco/Projects/node/test/common/report.js:30:10)
    at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:54:3)
Command: out/Release/node /home/tomgco/Projects/node/test/node-report/test-exception.js  

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 18, 2019
@tomgco tomgco changed the title Allow --perf-basic-prof-only-functions and --perf-prof-unwinding-info in NODE_OPTIONS src: Add new perf flags in NODE_OPTIONS Jan 18, 2019
@sam-github
Copy link
Contributor

@gireeshpunathil would be interested in this, it looks like it is from the node-report tests. Can you look at the output file and see what is invalid about the JSON, and perhaps comment on #22712?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2019

I think the commits themselves might need to be reworked a bit. It seems like related changes are not grouped together logically. For example, the first commit adds docs for things that aren't in the code yet. I'd suggest doing one commit for the doc/test/src changes for --perf-basic-prof-only-functions, and another commit for --perf-prof-unwinding-info.

@tomgco
Copy link
Contributor Author

tomgco commented Jan 18, 2019

I think the commits themselves might need to be reworked a bit. It seems like related changes are not grouped together logically. For example, the first commit adds docs for things that aren't in the code yet. I'd suggest doing one commit for the doc/test/src changes for --perf-basic-prof-only-functions, and another commit for --perf-prof-unwinding-info.

Sure I can do that, what would the commit message be for the grouped changes? just src? or src/test/docs?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2019

I think just src: for the subsystem is fine.

@tomgco tomgco force-pushed the support-for-prof-unwinding-info branch from ae16eb0 to 4d24591 Compare January 18, 2019 16:31
@tomgco
Copy link
Contributor Author

tomgco commented Jan 18, 2019

I think just src: for the subsystem is fine.

Thanks for the feedback, updated.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Thanks!

@tomgco
Copy link
Contributor Author

tomgco commented Jan 19, 2019

As far as I can see the failure is a flakey test, if i am mistaken let me know what I need to fix:
https://ci.nodejs.org/job/node-test-binary-windows/23168/COMPILED_BY=vs2017-x86,RUNNER=win2012r2,RUN_SUBSET=3/consoleText

not ok 542 parallel/test-worker-uncaught-exception-async
  ---
  duration_ms: 120.94
  severity: fail
  exitcode: 1
  stack: |-
    timeout
    foo

@gireeshpunathil
Copy link
Member

the escape characters covered in the report values were not comprehensive, so probably we missed a character or two that was present in the user's LESS_TERMCAP_me environment variable that has special meaning in JSON. #25626 landed just now, so hopefully we should not see this anymore. @tomgco - could you please perform a rebase and retest to see if it settles failure on your system? If it persists, would you mind providing the value of the env variable LESS_TERMCAP_me for me to have a look? thanks!

@gireeshpunathil
Copy link
Member

Resume Build CI: https://ci.nodejs.org/job/node-test-commit/25296/

@gireeshpunathil
Copy link
Member

the resume build was instantly aborted fully for some reason.

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

@tomgco tomgco force-pushed the support-for-prof-unwinding-info branch from 4d24591 to dd4518a Compare January 25, 2019 10:55
@tomgco
Copy link
Contributor Author

tomgco commented Jan 25, 2019

the escape characters covered in the report values were not comprehensive, so probably we missed a character or two that was present in the user's LESS_TERMCAP_me environment variable that has special meaning in JSON. #25626 landed just now, so hopefully we should not see this anymore. @tomgco - could you please perform a rebase and retest to see if it settles failure on your system? If it persists, would you mind providing the value of the env variable LESS_TERMCAP_me for me to have a look? thanks!

That fix seems to have worked for me 👏

@addaleax
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 27, 2019
@addaleax
Copy link
Member

addaleax commented Feb 4, 2019

@addaleax
Copy link
Member

addaleax commented Feb 8, 2019

Landed in f5b9a78, f265225

@addaleax addaleax closed this Feb 8, 2019
addaleax pushed a commit that referenced this pull request Feb 8, 2019
PR-URL: #25565
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
PR-URL: #25565
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
PR-URL: #25565
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
PR-URL: #25565
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@hekike
Copy link
Contributor

hekike commented Apr 4, 2019

Any concern around interpreted frames and Linux Perf and the lack of V8 support?
Personally, I'm not sure it's a good idea adding more related flags without forcing --interpreted-frames-native-stack or showing a warning about it.

https://nodejs.org/en/docs/guides/diagnostics-flamegraph/#node-js-10

You can find more context here:
nodejs/diagnostics#148

In nutshell Linux Perf was never supported by V8 and it doesn't work with interpreted frames by default, although the new CodeEventListenerAPI it is and works well.
It's highly recommended to use Linux Perf via the new API through https://www.npmjs.com/package/linux-perf

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.

8 participants