-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port to nan for compatibility with newer V8/Node versions. #79
Conversation
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. |
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.
The V8 profiler API changed greatly between V8 3.14 (in node 0.10) and 3.28 (in node 0.12.0): - a few functions got renamed - quite a few functions were removed entirely - the remaining CpuProfile and HeapProfile methods were mostly static and now require an object instance, to deal with the V8 Isolate (which is also where you get the object instance) I just #ifdef'd out references to the functions that don't exist any more, in the hopes that the frontend wouldn't call them, and I just use the current Isolate anywhere an instance is needed. This seems to mostly work. Current status: - node 0.10: this compiles, seems to work - node 0.12: this compiles, seems to mostly work. - heap snapshot seems to work - missing bottom-up cpu profile is annoying because UI starts with that one by default - even in top-down cpu profile all the samples are 0
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. |
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:
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. |
Invoke StartProfiling with record_samples=true. Expose the new CpuProfileNode::GetHitCount accessor, even though I'm not sure whether we can get this exposed in the UI. Add a few more comments on the differences in properties/accessors exposed. The V26 frontend still doesn't really know what to do with the CPU profiles we feed it, and show a call graph with 0 time spent.
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
Your guess is correct.
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
I would suggest not to worry about supporting connections from devtools shipped with stable Chrome as I mentioned before.
no idea :/, maybe we can use https://omahaproxy.appspot.com to find out.
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.
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. |
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? |
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/! |
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. |
Thank you again! I know this was a pain to work on. |
Port to nan for compatibility with newer V8/Node versions.
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) |
Then the blink we're looking for is
which is http://src.chromium.org/viewvc/blink?view=revision&revision=180090, or in the git repo
if I'm interpreting all the mappings between SVN commits correctly. The dates seem roughly right at least. |
That gives us as target Chromium 38.0.2122.0. It looks about right to me @metamatt. 👍 |
Now we just need to go and rip Chromium 38.0.2122.0 apart to get devtools out of it 💃 |
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"
} |
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 |
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
These are changes to the V8 profiler API that are not abstracted by
nan.