src: Add new perf flags in NODE_OPTIONS by tomgco · Pull Request #25565 · nodejs/node · GitHub
Skip to content

src: Add new perf flags in NODE_OPTIONS#25565

Closed
tomgco wants to merge 2 commits intonodejs:masterfrom
tomgco:support-for-prof-unwinding-info
Closed

src: Add new perf flags in NODE_OPTIONS#25565
tomgco wants to merge 2 commits intonodejs:masterfrom
tomgco:support-for-prof-unwinding-info

Conversation

@tomgco
Copy link
Copy Markdown
Contributor

@tomgco tomgco commented Jan 18, 2019 •
edited
Loading

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
Copy Markdown
Contributor

sam-github commented Jan 18, 2019

@cjihrig
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

tomgco commented Jan 18, 2019 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Contributor Author

tomgco commented Jan 18, 2019

I think just src: for the subsystem is fine.

Thanks for the feedback, updated.

cjihrig
cjihrig approved these changes Jan 18, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

gireeshpunathil 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!

@gireeshpunathil
Copy link
Copy Markdown
Member

gireeshpunathil commented Jan 25, 2019

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

@gireeshpunathil
Copy link
Copy Markdown
Member

gireeshpunathil commented Jan 25, 2019

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
Copy Markdown
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
Copy Markdown
Member

addaleax commented Jan 27, 2019

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

@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
Copy Markdown
Member

addaleax commented Feb 4, 2019

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

@addaleax
Copy link
Copy Markdown
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
@tomgco @addaleax
src: allow --perf-basic-prof-only-functions in NODE_OPTIONS
f5b9a78 
PR-URL: #25565
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
@tomgco @addaleax
src: allow --perf-prof-unwinding-info in NODE_OPTIONS
f265225 
PR-URL: #25565
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
@tomgco @addaleax
src: allow --perf-basic-prof-only-functions in NODE_OPTIONS
e6a2548 
PR-URL: #25565
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 8, 2019
@tomgco @addaleax
src: allow --perf-prof-unwinding-info in NODE_OPTIONS
c72c4b0 
PR-URL: #25565
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@hekike
Copy link
Copy Markdown
Contributor

hekike commented Apr 4, 2019 •
edited
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+1 more reviewer

@sam-github sam-github sam-github approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tomgco @sam-github @cjihrig @gireeshpunathil @addaleax @hekike @jasnell @nodejs-github-bot