Port to nan for compatibility with newer V8/Node versions.

metamatt commented 9 years ago

This compiles with node 0.10.33 and seems to work in some cursory tests.

This comes near to compiling with node 0.12.0, except for a bunch of errors like

error: ‘DeleteAllSnapshots’ is not a member of ‘v8::HeapProfiler’
error: ‘class v8::CpuProfileNode’ has no member named ‘GetSelfTime’

These are changes to the V8 profiler API that are not abstracted by nan.

metamatt commented 9 years ago

FWIW, I see this as a step in the direction of enabling #66/#77/#78, but it doesn't do the whole job, because the V8 profiler APIs also changed in ways not abstracted by nan.

Here are the remaining errors when compiling with node 0.12.0.

metamatt commented 9 years ago

New version of PR (amends first commit, adds second commit).

I amended the first commit because it turns out my first stab at that was incomplete. There were places I'd left raw V8-isms and neglected to convert to nan, and it still compiles and works fine in node 0.10, but didn't compile in node 0.12, but I didn't notice because the profile-api-specific breakage (see 2nd commit) prevented the compiler from getting far enough to complain about it. Also, there were some places I confused NanScope and NanEscapableScope that again, didn't matter for node 0.10, but I discovered once I wrote the 2nd commit and started testing with node 0.12.

The 2nd commit handles the places that the V8 profiler API changed. It's pretty clumsy, and doesn't entirely work -- in particular, the top-down cpu profile is available but all the sample values are 0. I haven't looked into that yet.

metamatt commented 9 years ago

After a bit more work on this, I'm pessimistic about getting the CPU profiling working. @c4milo, I'm wondering if you can chime in and save me some time figuring stuff out if you already know it, specifically about the history of the Chrome inspector profile view, and the frontend/backend split. Here's what I observe, with some assumptions:

The docs for node-webkit-agent imply that at some earlier point, it was possible to use the profiler tab in a live version of Chrome as the frontend and connect to an arbitrary backend, but that's no longer possible and you host the frontend at http://c4milo.github.io/node-webkit-agent/26.0.1410.65/inspector.html?host=localhost:9999&page=0 (note the version 26 visible in the URL).
Using that version 26 frontend against the current node-webkit-agent 0.3.x and Node 0.10 (V8 3.14) works fine; I assume the backend in node-webkit-agent is from roughly that vintage of Chrome
Using that version 26 frontend against my patches here and Node 0.12.0 (V8 3.28) does connect, and the heap profiler is actually useful, but the CPU profiler is not.
If I run current Chrome stable (Chrome 41, V8 4.1) and open the web inspector, the profiler tab looks pretty much like the one from Chrome 26, but there's no option to connect to an external backend, and the related stuff seems to be missing from the source code.
NodeJS/V8 version correlations: Node 0.10 is V8 3.14. Node 0.12 is V8 3.28.
Chrome/V8 version correlations: Chrome 26 is V8 3.16. V8 3.28 is something like Chrome 38?

From this, I'm guessing that getting the V26 frontend to work against the ~V38 backend isn't going to work (the internal v8::profiler API is too different), and the split frontend/backend feature was removed (soon after V26 and that's why you've pinned the frontend you serve up at V26?) so we can't just use the V38 frontend against that V38 backend.

Is this roughly true? Where do we go from here? As things stand, this branch compiles for node 0.12, the heap profiler seems useful, the cpu profiler is not, and I don't see how to do better.

c4milo commented 9 years ago

The docs for node-webkit-agent imply that at some earlier point, it was possible to use the profiler tab in a live version of Chrome as the frontend and connect to an arbitrary backend, but that's no longer possible and you host the frontend at http://c4milo.github.io/node-webkit-agent/26.0.1410.65/inspector.html?host=localhost:9999&page=0 (note the version 26 visible in the URL).

It was possible but I never recommended it because Chromium development pace is insanely fast for myself alone to keep up. I wouldn't worry too much about this. I think providing a newer Devtools frontend under gh-pages branch should be reasonably enough for most people, they can run it locally too.

Using that version 26 frontend against the current node-webkit-agent 0.3.x and Node 0.10 (V8 3.14) works fine; I assume the backend in node-webkit-agent is from roughly that vintage of Chrome

Your guess is correct.

Using that version 26 frontend against my patches here and Node 0.12.0 (V8 3.28) does connect, and the heap profiler is actually useful, but the CPU profiler is not.

Either the CPU profiling data serialization changed in V8 or devtools frontend protocol changed. If the former, we will have to extract a newer version of Devtools Frontend from the Blink project and host it in our gh-pages branch. If the latter, we will have to enable debugging mode in devtools frontend to inspect websockets frames while doing CPU profiling of any website, this is to find out what exactly changed in the JSON protocol so that we can make the correspondent changes in our backend.

If I run current Chrome stable (Chrome 41, V8 4.1) and open the web inspector, the profiler tab looks pretty much like the one from Chrome 26, but there's no option to connect to an external backend, and the related stuff seems to be missing from the source code.

I would suggest not to worry about supporting connections from devtools shipped with stable Chrome as I mentioned before.

Chrome/V8 version correlations: Chrome 26 is V8 3.16. V8 3.28 is something like Chrome 38?

no idea :/, maybe we can use https://omahaproxy.appspot.com to find out.

From this, I'm guessing that getting the V26 frontend to work against the ~V38 backend isn't going to work (the internal v8::profiler API is too different), and the split frontend/backend feature was removed (soon after V26 and that's why you've pinned the frontend you serve up at V26?) so we can't just use the V38 frontend against that V38 backend.

If we can find a Chromium version shipped with V8 3.28.73 we can extract the devtools frontend shipped with it and that should work.

Is this roughly true? Where do we go from here? As things stand, this branch compiles for node 0.12, the heap profiler seems useful, the cpu profiler is not, and I don't see how to do better.

You have done great work, thanks a lot. We were also lucky the heap profiler kept working without modifications. Also, one good side effect of upgrading our devtools frontend copy could be that flamegraphs might work right out of the box.

c4milo commented 9 years ago

I'm going to merge since the port to NAN is done. We can tackle the CPU profiler issue in another PR. Do you agree?

metamatt commented 9 years ago

I did grab the blink source and extract the devtools frontend and couldn't convince it to connect to an external backend. I didn't try super hard -- I just tried the URL query string that worked before and nothing happened, and I grepped around the source for stuff to do with the remote backend that I saw the V26 frontend including, and didn't see them. I also had to do some mucking with a protocol.json file that is in the newer frontend and not the V26 frontend. Sorry, this was all 2 weeks ago and I don't remember the exact details of what I tried.

I didn't try mapping V8 3.28.x to a Chromium version and then a blink version and extracting the devtools from that; that might indeed work better.

That V8/Chrome version lookup tool at https://omahaproxy.appspot.com/ is a lot easier than the way I was doing it with http://src.chromium.org/viewvc/chrome/releases/!

metamatt commented 9 years ago

going to merge since the port to NAN is done

Works for me, thanks.

And that makes it easier for someone else (you or anybody) to take up the next steps, since I don't have a lot of time to work on this in the near term.

c4milo commented 9 years ago

Thank you again! I know this was a pain to work on.

metamatt commented 9 years ago

Re the matching chrome/v8 versions, the V8 lookup part of that nicer tool seems broken at least for the range of chrome versions we're interested in, it just throws exceptions.

I did it the old way and here's the closest I can find:

http://src.chromium.org/viewvc/chrome/releases/38.0.2122.0/DEPS (says v8 change 23085) https://chromium.googlesource.com/v8/v8/+/3.28.73 (says 3.28.73 is change 23084; but also has git-svn-id: https://v8.googlecode.com/svn/trunk@23094)

metamatt commented 9 years ago

Then the blink we're looking for is

376    'src/third_party/WebKit':
377       Var("webkit_trunk")[:-6] + '/trunk@180090',

which is http://src.chromium.org/viewvc/blink?view=revision&revision=180090,

or in the git repo

commit 32a9fc540b435fe02e0c651583540ecd1ef72ecf
Author: adamk@chromium.org <adamk@chromium.org@bbb929c8-8fbe-4397-9dbb-9b2b20218538>
Date:   Tue Aug 12 18:16:25 2014 +0000

    ... <snip> ...

    git-svn-id: svn://svn.chromium.org/blink/trunk@180090 bbb929c8-8fbe-4397-9dbb-9b2b20218538

if I'm interpreting all the mappings between SVN commits correctly. The dates seem roughly right at least.

c4milo commented 9 years ago

That gives us as target Chromium 38.0.2122.0. It looks about right to me @metamatt. :+1:

c4milo commented 9 years ago

Now we just need to go and rip Chromium 38.0.2122.0 apart to get devtools out of it :dancer:

c4milo commented 9 years ago

But first we need to find out what Blink version was shipped with that Chromium version as devtools frontend is part of Blink. I got this result by hitting: https://omahaproxy.appspot.com/webkit.json?version=38.0.2122.0

{
  "blink_branch_position": "180090", 
  "chromium_version": "38.0.2122.0", 
  "webkit_branch_position": "180090", 
  "blink_position": "180090", 
  "webkit_version": "unknown", 
  "webkit_position": "180090"
}

c4milo commented 9 years ago

ah, I just saw your post @metamatt. Yes, you are right, our Blink revision is http://src.chromium.org/viewvc/blink?view=revision&revision=180090

c4milo / node-webkit-agent

Port to nan for compatibility with newer V8/Node versions. #79