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

fix --prof-process --preprocess flag #14966

Closed
wants to merge 1 commit into from
Closed

fix --prof-process --preprocess flag #14966

wants to merge 1 commit into from

Conversation

davidmarkclements
Copy link
Member

Checklist
Affected core subsystem(s)

tools

This is a one-line fix to prevent the --preprocess
option (used with --prof-process to output JSON)
to cause an isolate log file profiling process to crash.

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Aug 21, 2017
@davidmarkclements
Copy link
Member Author

cc @mcollina @jasnell @MylesBorins

@mcollina
Copy link
Member

@mcollina
Copy link
Member

Linting is failing.

This is a one-line fix to prevent the --preprocess
option (used with --prof-process to output JSON)
to cause an isolate log file profiling process to crash.
@davidmarkclements
Copy link
Member Author

sorry :bowtie: - fixed

@mcollina
Copy link
Member

@mcollina mcollina self-requested a review August 22, 2017 14:04
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina
Copy link
Member

CI is green

jasnell pushed a commit that referenced this pull request Aug 23, 2017
This is a one-line fix to prevent the --preprocess
option (used with --prof-process to output JSON)
to cause an isolate log file profiling process to crash.

PR-URL: #14966
Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

Landed in 7c948ce

@jasnell jasnell closed this Aug 23, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Aug 25, 2017
This is a one-line fix to prevent the --preprocess
option (used with --prof-process to output JSON)
to cause an isolate log file profiling process to crash.

PR-URL: nodejs/node#14966
Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Aug 28, 2017
This is a one-line fix to prevent the --preprocess
option (used with --prof-process to output JSON)
to cause an isolate log file profiling process to crash.

PR-URL: nodejs/node#14966
Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
This is a one-line fix to prevent the --preprocess
option (used with --prof-process to output JSON)
to cause an isolate log file profiling process to crash.

PR-URL: #14966
Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
@mscdex
Copy link
Contributor

mscdex commented Sep 10, 2017

I just noticed this landed without a prefix in the commit message. You might consider using a git hook (such as core-validate-commit) to prevent such errors in the future.

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
This is a one-line fix to prevent the --preprocess
option (used with --prof-process to output JSON)
to cause an isolate log file profiling process to crash.

PR-URL: #14966
Reviewed-By: Luca Maraschi <luca.maraschi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

The test appears to time out on v6.x

Is this commit applicable to that branch?

@MylesBorins
Copy link
Contributor

ping

@davidmarkclements
Copy link
Member Author

The v8 version on the Node 6 branch does not appear to support the --preprocess flag

What's the appropriate way to ensure a test doesn't run against the v6.x branch?


On Node 8.7.0:

node --prof-process --help isolate-0x102801600-v8.log
Testing v8 version different from logging version
Cmdline args: [options] [log-file-name]
Default log file name is "v8.log".

Options:
  -j, --js             Show only ticks from JS VM state
  -g, --gc             Show only ticks from GC VM state
  -c, --compiler       Show only ticks from COMPILER VM state
  -o, --other          Show only ticks from OTHER VM state
  -e, --external       Show only ticks from EXTERNAL VM state
  --filter-runtime-timer Show only ticks matching the given runtime timer scope
  --call-graph-size    Set the call graph size
  --ignore-unknown     Exclude ticks of unknown code entries from processing
  --separate-ic        Separate IC entries
  --separate-bytecodes Separate Bytecode entries
  --separate-builtins  Separate Builtin entries
  --separate-stubs     Separate Stub entries
  --unix               Specify that we are running on *nix platform
  --windows            Specify that we are running on Windows platform
  --mac                Specify that we are running on Mac OS X platform
  --nm                 Specify the 'nm' executable to use (e.g. --nm=/my_dir/nm)
  --target             Specify the target root directory for cross environment
  --range              Specify the range limit as [start],[end]
  --distortion         Specify the logging overhead in picoseconds
  --source-map         Specify the source map that should be used for output
  --timed-range        Ignore ticks before first and after last Date.now() call
  --pairwise-timed-range, --ptr Ignore ticks outside pairs of Date.now() calls
  --only-summary       Print only tick summary, exclude other information
  --preprocess         Preprocess for consumption with web interface

On Node 6.11.4:

node --prof-process --help isolate-0x102801600-v8.log
Testing v8 version different from logging version
Cmdline args: [options] [log-file-name]
Default log file name is "v8.log".

Options:
  -j, --js             Show only ticks from JS VM state
  -g, --gc             Show only ticks from GC VM state
  -c, --compiler       Show only ticks from COMPILER VM state
  -o, --other          Show only ticks from OTHER VM state
  -e, --external       Show only ticks from EXTERNAL VM state
  --call-graph-size    Set the call graph size
  --ignore-unknown     Exclude ticks of unknown code entries from processing
  --separate-ic        Separate IC entries
  --unix               Specify that we are running on *nix platform
  --windows            Specify that we are running on Windows platform
  --mac                Specify that we are running on Mac OS X platform
  --nm                 Specify the 'nm' executable to use (e.g. --nm=/my_dir/nm)
  --target             Specify the target root directory for cross environment
  --range              Specify the range limit as [start],[end]
  --distortion         Specify the logging overhead in picoseconds
  --source-map         Specify the source map that should be used for output
  --timed-range        Ignore ticks before first and after last Date.now() call
  --pairwise-timed-range, --ptr Ignore ticks outside pairs of Date.now() calls
  --only-summary       Print only tick summary, exclude other information

@mcollina
Copy link
Member

We just don't backport the commit to that branch. I've added the relevant tags!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants