tools: change tick processor install path #4021

matthewloring · 2015-11-25T14:15:27Z

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.

matthewloring · 2015-11-25T14:16:36Z

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

Nibbler999 · 2015-11-25T14:44:01Z

$PREFIX/bin would be the logical place.

evanlucas · 2015-11-25T15:26:10Z

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...

bnoordhuis · 2015-11-25T22:00:14Z

$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.

evanlucas · 2015-11-25T23:00:53Z

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

jbergstroem · 2015-11-26T04:15:25Z

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

matthewloring · 2015-12-01T01:50:39Z

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.

evanlucas · 2015-12-01T02:30:11Z

LGTM

bnoordhuis · 2015-12-01T23:33:29Z

tools/v8-prof/polyfill.js

+try {
+  fs.accessSync(logFile);
+} catch(e) {
+  console.log('Please provide a valid isolate file as the final argument.');


console.error?

bnoordhuis · 2015-12-01T23:34:22Z

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

matthewloring · 2015-12-01T23:47:32Z

Sounds good!

bnoordhuis · 2015-12-02T20:45:11Z

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.

matthewloring · 2015-12-02T21:02:47Z

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?

bnoordhuis · 2015-12-02T21:31:07Z

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

matthewloring · 2015-12-02T21:42:40Z

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

rvagg · 2015-12-02T21:48:49Z

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".

matthewloring · 2015-12-02T22:02:03Z

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

rvagg · 2015-12-02T22:32:10Z

@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.

matthewloring · 2015-12-03T00:05:56Z

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?

bnoordhuis · 2015-12-03T00:28:23Z

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' ]

matthewloring · 2015-12-03T18:42:15Z

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

Fishrock123 · 2015-12-04T01:16:14Z

@matthewloring I'll try to look at this tomorrow

matthewloring · 2015-12-04T01:50:13Z

@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.

bnoordhuis · 2015-12-07T17:39:17Z

lib/internal/v8_prof_processor.js

+  tickArguments.push('--windows');
+}
+tickArguments.push.apply(tickArguments, process.argv.slice(1));
+cp.spawn(process.execPath, tickArguments, { stdio: 'inherit' });


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.

rvagg · 2016-01-12T00:51:02Z

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.

jasnell · 2016-01-12T20:04:11Z

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

MylesBorins · 2016-02-17T19:05:17Z

@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

jasnell · 2016-02-17T19:19:35Z

LGTM for v4.4

MylesBorins · 2016-02-22T19:40:45Z

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

rvagg · 2016-02-23T01:34:53Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: change tick processor install path #4021

tools: change tick processor install path #4021

matthewloring commented Nov 25, 2015

matthewloring commented Nov 25, 2015

Nibbler999 commented Nov 25, 2015

evanlucas commented Nov 25, 2015

bnoordhuis commented Nov 25, 2015

evanlucas commented Nov 25, 2015

jbergstroem commented Nov 26, 2015

matthewloring commented Dec 1, 2015

evanlucas commented Dec 1, 2015

bnoordhuis Dec 1, 2015

bnoordhuis commented Dec 1, 2015

matthewloring commented Dec 1, 2015

bnoordhuis commented Dec 2, 2015

matthewloring commented Dec 2, 2015

bnoordhuis commented Dec 2, 2015

matthewloring commented Dec 2, 2015

rvagg commented Dec 2, 2015

matthewloring commented Dec 2, 2015

rvagg commented Dec 2, 2015

matthewloring commented Dec 3, 2015

bnoordhuis commented Dec 3, 2015

matthewloring commented Dec 3, 2015

Fishrock123 commented Dec 4, 2015

matthewloring commented Dec 4, 2015

bnoordhuis Dec 7, 2015

matthewloring Dec 7, 2015

rvagg commented Jan 12, 2016

jasnell commented Jan 12, 2016

MylesBorins commented Feb 17, 2016

jasnell commented Feb 17, 2016

MylesBorins commented Feb 22, 2016

rvagg commented Feb 23, 2016

tools: change tick processor install path #4021

tools: change tick processor install path #4021

Conversation

matthewloring commented Nov 25, 2015

matthewloring commented Nov 25, 2015

Nibbler999 commented Nov 25, 2015

evanlucas commented Nov 25, 2015

bnoordhuis commented Nov 25, 2015

evanlucas commented Nov 25, 2015

jbergstroem commented Nov 26, 2015

matthewloring commented Dec 1, 2015

evanlucas commented Dec 1, 2015

bnoordhuis Dec 1, 2015

Choose a reason for hiding this comment

bnoordhuis commented Dec 1, 2015

matthewloring commented Dec 1, 2015

bnoordhuis commented Dec 2, 2015

matthewloring commented Dec 2, 2015

bnoordhuis commented Dec 2, 2015

matthewloring commented Dec 2, 2015

rvagg commented Dec 2, 2015

matthewloring commented Dec 2, 2015

rvagg commented Dec 2, 2015

matthewloring commented Dec 3, 2015

bnoordhuis commented Dec 3, 2015

matthewloring commented Dec 3, 2015

Fishrock123 commented Dec 4, 2015

matthewloring commented Dec 4, 2015

bnoordhuis Dec 7, 2015

Choose a reason for hiding this comment

matthewloring Dec 7, 2015

Choose a reason for hiding this comment

rvagg commented Jan 12, 2016

jasnell commented Jan 12, 2016

MylesBorins commented Feb 17, 2016

jasnell commented Feb 17, 2016

MylesBorins commented Feb 22, 2016

rvagg commented Feb 23, 2016