inspector: implement --cpu-prof-interval #27535
Closed
Conversation
This file contains hidden or 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.
Collaborator
nodejs-github-bot added c++ Collaborator
Member Author
| cc @nodejs/v8-inspector @nodejs/process @nodejs/diagnostics |
hybrist approved these changes May 3, 2019
| })"); | ||
| DispatchMessage(enable); | ||
| DispatchMessage(start); | ||
| DispatchMessage("Profiler.enable"); |
Contributor
There was a problem hiding this comment.
Really liking the added clarity in these methods! 👍
addaleax approved these changes May 3, 2019
| } | ||
| if (env->options()->cpu_prof) { | ||
| const std::string& dir = env->options()->cpu_prof_dir; | ||
| env->set_cpu_prof_interval(env->options()->cpu_prof_interval); |
Member
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?
Member Author
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
Collaborator
Collaborator
Trott approved these changes May 5, 2019
Member
| 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
8 participants
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