inspector: implement --cpu-prof-interval #27535
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Auto-generate the message id and return it for future use (we can always parse the response to find the message containing the profile instead of relying on the inspector connection being synchornous). - Generate the message from method and parameter strings and create a `StringView` directly to avoid the unnecessary copy in `ToProtocolString()`.
This patch implements --cpu-prof-interval to specify the sampling interval of the CPU profiler started by --cpu-prof from the command line. Also adjust the interval to 100 in test-cpu-prof.js to make the test less flaky - it would fail if the time taken to finish the workload is smaller than the sampling interval, which was more likely on powerful machines when the interval was 1000.
nodejs-github-bot
added
c++
|
cc @nodejs/v8-inspector @nodejs/process @nodejs/diagnostics |
jkrems
approved these changes
May 3, 2019
| })"); | ||
| DispatchMessage(enable); | ||
| DispatchMessage(start); | ||
| DispatchMessage("Profiler.enable"); |
There was a problem hiding this comment.
Really liking the added clarity in these methods! 👍
addaleax
approved these changes
May 3, 2019
| @@ -320,6 +307,7 @@ void StartProfilers(Environment* env) { | |||
| } | |||
| if (env->options()->cpu_prof) { | |||
| const std::string& dir = env->options()->cpu_prof_dir; | |||
| env->set_cpu_prof_interval(env->options()->cpu_prof_interval); | |||
There was a problem hiding this comment.
We could just use env->options()->cpu_prof_interval everywhere instead of an extra setter/getter pair, right?
There was a problem hiding this comment.
I was thinking about making this profiling capability available on-demand during runtime in the future, as requested here, in that case I think it makes more sense to put the options on the Environment itself (or in a separate struct when we have too many of them), because these may not come from CLI at that point.
Co-Authored-By: joyeecheung <joyeec9h3@gmail.com>
joyeecheung
added
the
author ready
fhinkel
approved these changes
May 5, 2019
Trott
approved these changes
May 5, 2019
|
Landed in 3d98051...7cfcf80 |
Trott
pushed a commit
to Trott/io.js
that referenced
this pull request
May 5, 2019
- Auto-generate the message id and return it for future use (we can always parse the response to find the message containing the profile instead of relying on the inspector connection being synchornous). - Generate the message from method and parameter strings and create a `StringView` directly to avoid the unnecessary copy in `ToProtocolString()`. PR-URL: nodejs#27535 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Trott
pushed a commit
to Trott/io.js
that referenced
this pull request
May 5, 2019
This patch implements --cpu-prof-interval to specify the sampling interval of the CPU profiler started by --cpu-prof from the command line. Also adjust the interval to 100 in test-cpu-prof.js to make the test less flaky - it would fail if the time taken to finish the workload is smaller than the sampling interval, which was more likely on powerful machines when the interval was 1000. PR-URL: nodejs#27535 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
that referenced
this pull request
May 6, 2019
- Auto-generate the message id and return it for future use (we can always parse the response to find the message containing the profile instead of relying on the inspector connection being synchornous). - Generate the message from method and parameter strings and create a `StringView` directly to avoid the unnecessary copy in `ToProtocolString()`. PR-URL: #27535 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
that referenced
this pull request
May 6, 2019
This patch implements --cpu-prof-interval to specify the sampling interval of the CPU profiler started by --cpu-prof from the command line. Also adjust the interval to 100 in test-cpu-prof.js to make the test less flaky - it would fail if the time taken to finish the workload is smaller than the sampling interval, which was more likely on powerful machines when the interval was 1000. PR-URL: #27535 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
added
the
semver-minor
Merged
targos
added a commit
that referenced
this pull request
May 6, 2019
Notable changes:
* tls:
* Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
option to `tls.createServer()`. When enabled, TSL packet trace
information is written to `stderr`. This can be used to debug TLS
connection problems. #27497
* cli:
* Added a `--trace-tls` command-line flag that enables tracing of TLS
connections without the need to modify existing application code.
#27497
* Added a `--cpu-prof-interval` command-line flag. It can be used to
specify the sampling interval for the CPU profiles generated by
`--cpu-prof`. #27535
* module:
* Added the `createRequire()` method. It allows to create a require
function a file URL object, a file URL string or an absolute path
string. The existing `createRequireFromPath()` method is now
deprecated #27405.
* Throw on `require('./path.mjs')`. This is technically a breaking
change that should have landed with Node.js 12.0.0. It is necessary
to have this to keep the possibility for a future minor version to
load ES Modules with the require function.
#27417
* meta:
* Added Christian Clauss (https://github.com/cclauss) to
collaborators. #27554
PR-URL: #27578
targos
added a commit
that referenced
this pull request
May 7, 2019
Notable changes:
* deps:
* Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
parser refuse any request URL that contained the "|" (vertical bar)
character. #27595
* tls:
* Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
option to `tls.createServer()`. When enabled, TSL packet trace
information is written to `stderr`. This can be used to debug TLS
connection problems. #27497
#27376
* cli:
* Added a `--trace-tls` command-line flag that enables tracing of TLS
connections without the need to modify existing application code.
#27497
* Added a `--cpu-prof-interval` command-line flag. It can be used to
specify the sampling interval for the CPU profiles generated by
`--cpu-prof`. #27535
* module:
* Added the `createRequire()` method. It allows to create a require
function from a file URL object, a file URL string or an absolute
path string. The existing `createRequireFromPath()` method is now
deprecated #27405.
* Throw on `require('./path.mjs')`. This is technically a breaking
change that should have landed with Node.js 12.0.0. It is necessary
to have this to keep the possibility for a future minor version to
load ES Modules with the require function.
#27417
* repl:
* The REPL now supports multi-line statements using `BigInt` literals
as well as public and private class fields and methods.
#27400
* The REPL now supports tab autocompletion of file paths with `fs`
methods. #26648
* meta:
* Added Christian Clauss (https://github.com/cclauss) to
collaborators. #27554
PR-URL: #27578
targos
added a commit
that referenced
this pull request
May 7, 2019
Notable changes:
* deps:
* Updated llhttp to 1.1.3. This fixes a bug that made Node.js' HTTP
parser refuse any request URL that contained the "|" (vertical bar)
character. #27595
* tls:
* Added an `enableTrace()` method to `TLSSocket` and an `enableTrace`
option to `tls.createServer()`. When enabled, TSL packet trace
information is written to `stderr`. This can be used to debug TLS
connection problems. #27497
#27376
* cli:
* Added a `--trace-tls` command-line flag that enables tracing of TLS
connections without the need to modify existing application code.
#27497
* Added a `--cpu-prof-interval` command-line flag. It can be used to
specify the sampling interval for the CPU profiles generated by
`--cpu-prof`. #27535
* module:
* Added the `createRequire()` method. It allows to create a require
function from a file URL object, a file URL string or an absolute
path string. The existing `createRequireFromPath()` method is now
deprecated #27405.
* Throw on `require('./path.mjs')`. This is technically a breaking
change that should have landed with Node.js 12.0.0. It is necessary
to have this to keep the possibility for a future minor version to
load ES Modules with the require function.
#27417
* repl:
* The REPL now supports multi-line statements using `BigInt` literals
as well as public and private class fields and methods.
#27400
* The REPL now supports tab autocompletion of file paths with `fs`
methods. #26648
* meta:
* Added Christian Clauss (https://github.com/cclauss) to
collaborators. #27554
PR-URL: #27578
This was referenced May 7, 2019
This was referenced May 7, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
src: refactor V8ProfilerConnection::DispatchMessage()
can always parse the response to find the message containing
the profile instead of relying on the inspector connection being
synchornous).
a
StringViewdirectly to avoid the unnecessary copy inToProtocolString().inspector: implement --cpu-prof-interval
This patch implements --cpu-prof-interval to specify the sampling
interval of the CPU profiler started by --cpu-prof from the command
line. Also adjust the interval to 100 in test-cpu-prof.js to make
the test less flaky - it would fail if the time taken to finish
the workload is smaller than the sampling interval, which was
more likely on powerful machines when the interval was 1000.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes