src: Add new perf flags in NODE_OPTIONS

[tomgco]

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]

@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]

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]

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]

I think just src: for the subsystem is fine.

[tomgco]

I think just src: for the subsystem is fine.

Thanks for the feedback, updated.

[cjihrig]

Thanks!

[tomgco]

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]

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]

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

[gireeshpunathil]

the resume build was instantly aborted fully for some reason.

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

[tomgco]

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]

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

[addaleax]

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

[addaleax]

Landed in f5b9a78, f265225

[hekike]

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