tools: change tick processor install path by matthewloring · Pull Request #4021 · nodejs/node · GitHub

This change cleans up outstanding comments on #3032. It improves error
handling when no isolate file is provided and changes the installation
location of the processor to $PREFIX/share/node-tick-processor.

@bnoordhuis Does $PREFIX/share seem like a more appropriate place? I'm not too familiar with the standard installation structure.

$PREFIX/bin would be the logical place.

I agree that putting it in $PREFIX/bin would be logical. Although, if we do that, it could cause conflicts with naming it node-tick-processor as there is an npm package called node-tick-processor...

$PREFIX/bin gets my vote as well. It's unfortunate there's an existing node-tick-processor package. Shortening it to ntp probably isn't a great idea either... Suggestions welcome.

any reason for it to not be just tick-processor?

+1 to node-tick-processor. Tab completion would make it pretty obvious that it "belongs to" node.

There didn't seem to be consensus on the name so I left it as node-tick-processor but I have moved the script to the bin directory.

LGTM

console.error?

LGTM with a nit / suggestion. I'm tagging this ctc-agenda to resolve the naming issue.

Sounds good!

Resolution from today's call is that it would be best to rename it to something that doesn't conflict with an existing package.

An alternative solution is to bake the scripts into the node binary and put them behind a command line flag.

The original name tick-processor does not have any collisions. If we want a node specific name, nodejs-tick-processor is also free. @bnoordhuis do you have a preference between these?

I don't really like either but I guess nodejs-tick-processor is the least objectionable.

I've changed the name to nodejs-tick-processor for now, I agree these name options aren't the best.

The most attractive option that came up during the call today was to bake it in and make it work something like node --prof-process or something along those lines.

IMO "tick processor" is a weird naming that doesn't really point to what it does and it'd be good to move from it if possible. It's kind of like the misnaming of GL "shaders".

I am happy to pursue this route but I'm not sure how to get started or where the change would live.

@matthewloring maybe the best entry-point would be in src/node.js, have a look at the section with // There is user code to be run and also how the repl is invoked. If the tick processor could be an internal module in lib/internal/ then it could be pulled up from there and invoked.

I've got a prototype exposing the functionality behind the --prof-process flag. The issue is the v8_prof_processor needs to load scripts from inside the bundled v8 and the lib/internal directory. Any guidance on how I could load these resources from inside the packaged node binary?

You can bake them into the binary with this patch:

diff --git a/node.gyp b/node.gyp
index a970c76..c67e369 100644
--- a/node.gyp
+++ b/node.gyp
@@ -79,6 +79,19 @@
       'lib/internal/socket_list.js',
       'lib/internal/util.js',
       'lib/internal/streams/lazy_transform.js',
+      'deps/v8/tools/SourceMap.js',
+      'deps/v8/tools/codemap.js',
+      'deps/v8/tools/consarray.js',
+      'deps/v8/tools/consarray.js',
+      'deps/v8/tools/csvparser.js',
+      'deps/v8/tools/csvparser.js',
+      'deps/v8/tools/logreader.js',
+      'deps/v8/tools/profile.js',
+      'deps/v8/tools/profile_view.js',
+      'deps/v8/tools/splaytree.js',
+      'deps/v8/tools/tickprocessor-driver.js',
+      'deps/v8/tools/tickprocessor.js',
+      'tools/v8-prof/polyfill.js',
     ],
   },

diff --git a/tools/js2c.py b/tools/js2c.py
index 0fc0ae0..ec9705e 100755
--- a/tools/js2c.py
+++ b/tools/js2c.py
@@ -310,7 +310,7 @@ def JS2C(source, target):
     else:
       ids.append((id, len(lines)))

-    escaped_id = id.replace('/', '_')
+    escaped_id = id.replace('-', '_').replace('/', '_')
     source_lines.append(SOURCE_DECLARATION % {
       'id': id,
       'escaped_id': escaped_id,

And then they're accessible through process.binding('natives'):

$ out/Release/node -p 'Object.keys(process.binding("natives")).filter(s => s.includes("v8"))'
[ 'v8',
  'v8/tools/SourceMap',
  'v8/tools/codemap',
  'v8/tools/consarray',
  'v8/tools/csvparser',
  'v8/tools/logreader',
  'v8/tools/profile',
  'v8/tools/profile_view',
  'v8/tools/splaytree',
  'v8/tools/tickprocessor-driver',
  'v8/tools/tickprocessor',
  'v8-prof/polyfill' ]

@bnoordhuis @rvagg I've got the flag working and tests passing with the latest revision.

@matthewloring I'll try to look at this tomorrow

@Fishrock123 Thanks!

This change cleans up outstanding comments on #3032. It improves error handling when no isolate file is provided and adds the --prof-process flag to the node binary which executes the tick processor on the provided isolate file.

Is it possible to do this without writing temp files and forking a new process? I imagine you can just concatenate the files and then eval() or vm.runInNewContext() them.

(EDIT: It's perhaps a bit of a pain to send the output through c++filt. Shouldn't be impossible, though.)

This would definitely be a big improvement. The current state was the easiest way to get things working but I'm hoping to get rid of the temp files/forking in a future PR.

No strong opinion, happy for this to be on lts-agenda though cause it's another interesting topic for us to cover in general for helping form policy.

I'm generally ok with this. There shouldn't be any real impact on LTS deployments.

@nodejs/lts can people chime in on this one? We have a good window right now with v4.4.0 to get this change in

LGTM for v4.4

@nodejs/lts can I please get another lgtm :D

lgtm for v4.4, I wouldn't +1 it if it was the only reason to bump semver-minor on LTS though

This change cleans up outstanding comments on nodejs#3032. It improves error handling when no isolate file is provided and adds the --prof-process flag to the node binary which executes the tick processor on the provided isolate file. PR-URL: nodejs#4021 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com>

This change cleans up outstanding comments on #3032. It improves error handling when no isolate file is provided and adds the --prof-process flag to the node binary which executes the tick processor on the provided isolate file. PR-URL: #4021 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com>

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * src: - Better support for Big-Endian systems (Bryon Leung) #3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841 * https: - A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) #3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) #5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) #5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) #4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) #5214 PR-URL: #5301

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) nodejs#3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) nodejs#4482 * src: - Better support for Big-Endian systems (Bryon Leung) nodejs#3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) nodejs#2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) nodejs#4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) nodejs#4841 * https: - A potential fix for nodejs#3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) nodejs#4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) nodejs#3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) nodejs#5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) nodejs#5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) nodejs#4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) nodejs#4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) nodejs#4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) nodejs#5214 PR-URL: nodejs#5301

This change cleans up outstanding comments on #3032. It improves error handling when no isolate file is provided and adds the --prof-process flag to the node binary which executes the tick processor on the provided isolate file. PR-URL: #4021 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com>

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * src: - Better support for Big-Endian systems (Bryon Leung) #3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841 * https: - A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) #3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) #5510 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) #4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) #5214 PR-URL: #5301

This change cleans up outstanding comments on nodejs#3032. It improves error handling when no isolate file is provided and adds the --prof-process flag to the node binary which executes the tick processor on the provided isolate file. PR-URL: nodejs#4021 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com>

Notable changes: * build: - Add support for Intel's VTune JIT profiling when compiled with --enable-vtune-profiling. For more information about VTune, see https://software.intel.com/en-us/node/544211. (Chunyang Dai) nodejs#3785. - Properly enable V8 snapshots by default. Due to a configuration error, snapshots have been kept off by default when the intention is for the feature to be enabled. (Fedor Indutny) nodejs#3962. * crypto: - Simplify use of ECDH (Elliptic Curve Diffie-Hellman) objects (created via crypto.createECDH(curve_name)) with private keys that are not dynamically generated via generateKeys(). The public key is now computed when explicitly setting a private key. Added validity checks to reduce the possibility of computing weak or invalid shared secrets. Also, deprecated the setPublicKey() method for ECDH objects as its usage is unnecessary and can lead to inconsistent state. (Michael Ruddy) nodejs#3511. - Update root certificates from the current list stored maintained by Mozilla NSS. (Ben Noordhuis) nodejs#3951. - Multiple CA certificates can now be passed with the ca option to TLS methods as an array of strings or in a single new-line separated string. (Ben Noordhuis) nodejs#4099 * tools: Include a tick processor in core, exposed via the --prof-process command-line argument which can be used to process V8 profiling output files generated when using the --prof command-line argument. (Matt Loring) nodejs#4021. PR-URL: nodejs#4181

Fishrock123 added the tools Issues and PRs related to the tools directory. label Nov 25, 2015

bnoordhuis reviewed Dec 1, 2015
View reviewed changes

bnoordhuis added the ctc-agenda label Dec 1, 2015

rvagg mentioned this pull request Dec 2, 2015

Node Foundation CTC Meeting 2015-12-02 #4115

Closed

mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Dec 2, 2015

bnoordhuis removed the ctc-agenda label Dec 2, 2015

tools: add --prof-process flag to node binary

f485bb9

This change cleans up outstanding comments on #3032. It improves error handling when no isolate file is provided and adds the --prof-process flag to the node binary which executes the tick processor on the provided isolate file.

bnoordhuis reviewed Dec 7, 2015
View reviewed changes

MylesBorins mentioned this pull request Jan 28, 2016

tools: run tick processor without forking #4224

Closed

matthewloring deleted the tick-improve branch February 25, 2016 19:55

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 1, 2016

MylesBorins mentioned this pull request Mar 1, 2016

V4.4.0 proposal #5301

Merged

MylesBorins added lts-landed-on-v4.x and removed lts-agenda labels Mar 21, 2016

tools: change tick processor install path by matthewloring · Pull Request #4021 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: change tick processor install path - #4021

tools: change tick processor install path#4021matthewloring wants to merge 1 commit intonodejs:masternodejs/node:masterfrom matthewloring:tick-improveCopy head branch name to clipboard

Conversation

matthewloring commented Nov 25, 2015

Uh oh!

matthewloring commented Nov 25, 2015

Uh oh!

Nibbler999 commented Nov 25, 2015

Uh oh!

evanlucas commented Nov 25, 2015

Uh oh!

bnoordhuis commented Nov 25, 2015

Uh oh!

evanlucas commented Nov 25, 2015

Uh oh!

jbergstroem commented Nov 26, 2015

Uh oh!

matthewloring commented Dec 1, 2015

Uh oh!

evanlucas commented Dec 1, 2015

Uh oh!

bnoordhuis Dec 1, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Dec 1, 2015

Uh oh!

matthewloring commented Dec 1, 2015

Uh oh!

bnoordhuis commented Dec 2, 2015

Uh oh!

matthewloring commented Dec 2, 2015

Uh oh!

bnoordhuis commented Dec 2, 2015

Uh oh!

matthewloring commented Dec 2, 2015

Uh oh!

rvagg commented Dec 2, 2015

Uh oh!

matthewloring commented Dec 2, 2015

Uh oh!

rvagg commented Dec 2, 2015

Uh oh!

matthewloring commented Dec 3, 2015

Uh oh!

bnoordhuis commented Dec 3, 2015

Uh oh!

matthewloring commented Dec 3, 2015

Uh oh!

Fishrock123 commented Dec 4, 2015

Uh oh!

matthewloring commented Dec 4, 2015

Uh oh!

bnoordhuis Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

matthewloring Dec 7, 2015

Choose a reason for hiding this comment

Uh oh!

rvagg commented Jan 12, 2016

Uh oh!

jasnell commented Jan 12, 2016

Uh oh!

MylesBorins commented Feb 17, 2016

Uh oh!

jasnell commented Feb 17, 2016

Uh oh!

MylesBorins commented Feb 22, 2016

Uh oh!

rvagg commented Feb 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

tools: change tick processor install path#4021
matthewloring wants to merge 1 commit into
nodejs:masterfrom
matthewloring:tick-improve