Add support for running the profiler output processing scripts with node.

Add support for running the profiler output processing scripts with node.

Currently, using the v8 profiler with node requires fetching v8 dependencies and building natively inside the node installation. This change allows the processing scripts to be fall back to node if a d8 installation cannot be found. It maintains compatibility with d8 if d8 can be found.

This is an experiment. It would be great to make these tools more accessible to node developers.

[mattloring, 2015-06-18 19:33:08]

mattloring@google.com changed reviewers:
+ ofrobots@google.com

[mattloring, 2015-06-18 19:33:08]

mattloring@google.com changed required reviewers:
+ ofrobots@google.com

[mattloring, 2015-06-18 19:44:47]

mattloring@google.com changed reviewers:
+ machenbach@chromium.org

[mattloring, 2015-06-18 19:44:47]

mattloring@google.com changed required reviewers:
+ machenbach@chromium.org

[Michael Achenbach, 2015-06-19 08:10:06]

machenbach@chromium.org changed reviewers:
+ jkummerow@chromium.org
- ofrobots@google.com

[Michael Achenbach, 2015-06-19 08:10:06]

machenbach@chromium.org changed required reviewers:
- machenbach@chromium.org, ofrobots@google.com

[Michael Achenbach, 2015-06-19 08:10:07]

I never touched any of these files. + jkummerow

[Jakob Kummerow, 2015-06-19 09:43:53]

jkummerow@chromium.org changed reviewers:
+ rossberg@chromium.org

[Jakob Kummerow, 2015-06-19 09:43:54]

Yeah, node.js developers will love this.

[+rossberg] Andreas, do the import/export statements (look at codemap.js for an
example) conflict with official ECMAScript module plans? Once we enable this
support, people will get mad if we break it again, even temporarily.

Style issues:
- Keep two empty new lines between top-level things (specifically, between the
import/export statements and the rest of the file).
- Comments should begin with an uppercase and end with appropriate punctuation.

A few more nits and one major comment below:

https://codereview.chromium.org/1179173009/diff/60001/tools/profile.js
File tools/profile.js (left):

https://codereview.chromium.org/1179173009/diff/60001/tools/profile.js#oldcod...
tools/profile.js:402: 
keep this line

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js
File tools/tickprocessor.js (left):

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js#...
tools/tickprocessor.js:33: 
keep this line

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js
File tools/tickprocessor.js (right):

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js#...
tools/tickprocessor.js:39: return require('child_process').execSync(name + ' ' +
args.join(' ')).toString();
80-col

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js#...
tools/tickprocessor.js:328: this.processLogChunk(contents);
Nope, this is not equivalent. processLogLine has extra logic in it.

[noordhuis, 2015-06-19 10:43:06]

info@bnoordhuis.nl changed reviewers:
+ info@bnoordhuis.nl

[noordhuis, 2015-06-19 10:43:07]

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js
File tools/tickprocessor.js (right):

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js#...
tools/tickprocessor.js:39: return require('child_process').execSync(name + ' ' +
args.join(' ')).toString();
I suggest using `execFileSync(name, args, { encoding: 'utf8'})` here.

[rossberg, 2015-06-19 11:38:25]

On 2015/06/19 09:43:54, Jakob wrote:
> Yeah, node.js developers will love this.
> 
> [+rossberg] Andreas, do the import/export statements (look at codemap.js for
an
> example) conflict with official ECMAScript module plans? Once we enable this
> support, people will get mad if we break it again, even temporarily.

I don't see any compatibility problems.

Yet, I'm not fond of putting embedder-specific hacks into the V8 implementation
itself. In particular, since it's not possible to have regression tests for that
on our side. Isn't there a way to address this on the node side? E.g., by having
a simple build script that wraps the respective files with the same statements
that this CL adds?

[mattloring, 2015-06-19 17:29:04]

https://codereview.chromium.org/1179173009/diff/60001/tools/profile.js
File tools/profile.js (left):

https://codereview.chromium.org/1179173009/diff/60001/tools/profile.js#oldcod...
tools/profile.js:402: 
On 2015/06/19 09:43:53, Jakob wrote:
> keep this line

Done.

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js
File tools/tickprocessor.js (left):

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js#...
tools/tickprocessor.js:33: 
On 2015/06/19 09:43:53, Jakob wrote:
> keep this line

Done.

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js
File tools/tickprocessor.js (right):

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js#...
tools/tickprocessor.js:39: return require('child_process').execSync(name + ' ' +
args.join(' ')).toString();
On 2015/06/19 09:43:53, Jakob wrote:
> 80-col

Done.

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js#...
tools/tickprocessor.js:39: return require('child_process').execSync(name + ' ' +
args.join(' ')).toString();
On 2015/06/19 10:43:07, noordhuis wrote:
> I suggest using `execFileSync(name, args, { encoding: 'utf8'})` here.

Done.

https://codereview.chromium.org/1179173009/diff/60001/tools/tickprocessor.js#...
tools/tickprocessor.js:328: this.processLogChunk(contents);
On 2015/06/19 09:43:53, Jakob wrote:
> Nope, this is not equivalent. processLogLine has extra logic in it.

Done.

[mattloring, 2015-06-19 17:45:07]

On 2015/06/19 11:38:25, rossberg wrote:
> On 2015/06/19 09:43:54, Jakob wrote:
> > Yeah, node.js developers will love this.
> > 
> > [+rossberg] Andreas, do the import/export statements (look at codemap.js for
> an
> > example) conflict with official ECMAScript module plans? Once we enable this
> > support, people will get mad if we break it again, even temporarily.
> 
> I don't see any compatibility problems.
> 
> Yet, I'm not fond of putting embedder-specific hacks into the V8
implementation
> itself. In particular, since it's not possible to have regression tests for
that
> on our side. Isn't there a way to address this on the node side? E.g., by
having
> a simple build script that wraps the respective files with the same statements
> that this CL adds?

It looks like modifying the node build process to wrap these files would be
difficult. Would it be preferable to use ES6 modules syntax instead of node
specific constructs to implement this change inside V8?

[mattloring, 2015-06-23 00:57:03]

As far as I know, there is not currently es6 module support in v8. As soon as
there is support, we can use this patch in its current form. It would still be
nice to put something in place so that node developers can more easily use the
profiler in the mean time.

[Jakob Kummerow, 2015-06-23 09:13:44]

Andreas (rossberg@) is on vacation this week, so it'll be a while before he can
chime in again.

I guess the alternative is to check in something like the following as
tools/node-tick-processor (modeled after linux-tick-processor and the polyfills
in your latest patch set):
---------------------
#!/bin/bash

TEMP_SCRIPT_FILE="/tmp/node-tick-processor-input-script"
tools_path=`cd $(dirname "$0");pwd`
log_file="v8.log"
for arg in "$@"
do
  if ! expr "X${arg}" : "^X-" > /dev/null; then
    log_file=${arg}
  fi
done

cat > $TEMP_SCRIPT_FILE << EOF
// Node polyfill
var os = os || {
  system: function(name, args) {
    return require('child_process').execFileSync(
        name, args, {encoding: 'utf8'}).toString();
  }
};
var print = print || console.log.bind(console);
var read = read || function(fileName) {
  return require('fs').readFileSync(fileName, 'utf8');
};
print("typeof process: " + typeof process);
print("process.argv: " + process.argv);
if (typeof process !== "undefined" && process.argv) {
  print("Polyfilling arguments");
  arguments = process.argv.slice(2);

  // Polyfill "readline()".
  var __readline_buffer__ = read(arguments[0]).split("\n");
  var __readline_index__ = 0;
  var readline = function() {
    if (__readline_index__ >= __readline_buffer__.length) return false;
    return __readline_buffer__[__readline_index__++];
  }
}
EOF

cat $tools_path/splaytree.js $tools_path/codemap.js \
  $tools_path/csvparser.js $tools_path/consarray.js \
  $tools_path/profile.js $tools_path/profile_view.js \
  $tools_path/logreader.js $tools_path/tickprocessor.js \
  $tools_path/SourceMap.js \
  $tools_path/tickprocessor-driver.js >> $TEMP_SCRIPT_FILE
  
node $TEMP_SCRIPT_FILE $log_file

rm -f $TEMP_SCRIPT_FILE
---------------------

For some reason with the nodejs-0.10.25 that happens to be on my machine, this
script only mostly works: ticks in generated code aren't attributed properly,
they all show up as "d8". I haven't debugged that. Maybe node 0.10 is just too
old, or maybe one of the polyfills isn't working right.

[mattloring, 2015-06-26 22:26:47]

I've implemented this polyfill as Jakob suggested. Would the shell scripts be
better placed here with their non-node analogues or in node?

[Jakob Kummerow, 2015-06-29 09:28:05]

On 2015/06/26 22:26:47, mattloring wrote:
> I've implemented this polyfill as Jakob suggested. Would the shell scripts be
> better placed here with their non-node analogues or in node?

Not sure. I wouldn't mind having them here, but OTOH V8 developers have no
reason to use/test them, so there's a good chance they'll break at some point.
Maybe hosting them in the node repository would make it easier to update them as
needed; in particular this would avoid the following sequence:
(1) some change breaks the polyfill (e.g., list of required .js files changes)
(2) V8 creates a branch
(3) node imports that branch
(4) node users realize that their tick processor is broken
(5) node developers propose V8 upstream patch
(6) patch needs V8 team review
(7) patch needs backporting to branch
(8) node must pull updated branch (with backport)
Steps 5-8 could be cut short to:
(5') node developers update their tick processor

[rossberg, 2015-06-30 14:02:03]

On 2015/06/29 09:28:05, Jakob wrote:
> On 2015/06/26 22:26:47, mattloring wrote:
> > I've implemented this polyfill as Jakob suggested. Would the shell scripts
be
> > better placed here with their non-node analogues or in node?
> 
> Not sure. I wouldn't mind having them here, but OTOH V8 developers have no
> reason to use/test them, so there's a good chance they'll break at some point.
> Maybe hosting them in the node repository would make it easier to update them
as
> needed; in particular this would avoid the following sequence:
> (1) some change breaks the polyfill (e.g., list of required .js files changes)
> (2) V8 creates a branch
> (3) node imports that branch
> (4) node users realize that their tick processor is broken
> (5) node developers propose V8 upstream patch
> (6) patch needs V8 team review
> (7) patch needs backporting to branch
> (8) node must pull updated branch (with backport)
> Steps 5-8 could be cut short to:
> (5') node developers update their tick processor

I agree. Checking in anything into the V8 tree that cannot reliably be
maintained and tested as part of the normal V8 flow and automation is suboptimal
for both sides.

[mattloring, 2015-06-30 14:56:51]

That makes sense. I'll work to get this into node directly. Thank you for your
suggestions during the review process!