src: Add new perf flags in NODE_OPTIONS #25565

tomgco · 2019-01-18T13:04:31Z

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

sam-github · 2019-01-18T16:18:17Z

@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 · 2019-01-18T16:24:23Z

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 · 2019-01-18T16:26:35Z

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 · 2019-01-18T16:28:02Z

I think just src: for the subsystem is fine.

tomgco · 2019-01-18T16:32:11Z

I think just src: for the subsystem is fine.

Thanks for the feedback, updated.

cjihrig

Thanks!

tomgco · 2019-01-19T16:25:54Z

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 · 2019-01-25T10:47:53Z

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 · 2019-01-25T10:52:06Z

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

gireeshpunathil · 2019-01-25T10:54:53Z

the resume build was instantly aborted fully for some reason.

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

tomgco · 2019-01-25T12:40:59Z

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 · 2019-01-27T20:34:04Z

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

addaleax · 2019-02-04T19:20:53Z

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

addaleax · 2019-02-08T13:54:34Z

Landed in f5b9a78, f265225

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 · 2019-04-04T18:27:17Z

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

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 18, 2019

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 approved these changes Jan 18, 2019

View reviewed changes

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

cjihrig approved these changes Jan 18, 2019

View reviewed changes

addaleax approved these changes Jan 18, 2019

View reviewed changes

gireeshpunathil approved these changes Jan 25, 2019

View reviewed changes

tomgco added 2 commits January 25, 2019 10:54

src: allow --perf-basic-prof-only-functions in NODE_OPTIONS

1c49ced

src: allow --perf-prof-unwinding-info in NODE_OPTIONS

dd4518a

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

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

jasnell approved these changes Feb 5, 2019

View reviewed changes

addaleax closed this Feb 8, 2019

targos mentioned this pull request Feb 14, 2019

v11.10.0 proposal #26098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: Add new perf flags in NODE_OPTIONS #25565

src: Add new perf flags in NODE_OPTIONS #25565

tomgco commented Jan 18, 2019 •

edited

Loading

sam-github commented Jan 18, 2019

cjihrig commented Jan 18, 2019

tomgco commented Jan 18, 2019 •

edited

Loading

cjihrig commented Jan 18, 2019

tomgco commented Jan 18, 2019

cjihrig left a comment

tomgco commented Jan 19, 2019

gireeshpunathil commented Jan 25, 2019

gireeshpunathil commented Jan 25, 2019

gireeshpunathil commented Jan 25, 2019

tomgco commented Jan 25, 2019

addaleax commented Jan 27, 2019

addaleax commented Feb 4, 2019

addaleax commented Feb 8, 2019

hekike commented Apr 4, 2019 •

edited

Loading

src: Add new perf flags in NODE_OPTIONS #25565

src: Add new perf flags in NODE_OPTIONS #25565

Conversation

tomgco commented Jan 18, 2019 • edited Loading

Checklist

sam-github commented Jan 18, 2019

cjihrig commented Jan 18, 2019

tomgco commented Jan 18, 2019 • edited Loading

cjihrig commented Jan 18, 2019

tomgco commented Jan 18, 2019

cjihrig left a comment

Choose a reason for hiding this comment

tomgco commented Jan 19, 2019

gireeshpunathil commented Jan 25, 2019

gireeshpunathil commented Jan 25, 2019

gireeshpunathil commented Jan 25, 2019

tomgco commented Jan 25, 2019

addaleax commented Jan 27, 2019

addaleax commented Feb 4, 2019

addaleax commented Feb 8, 2019

hekike commented Apr 4, 2019 • edited Loading

tomgco commented Jan 18, 2019 •

edited

Loading

tomgco commented Jan 18, 2019 •

edited

Loading

hekike commented Apr 4, 2019 •

edited

Loading