Add support for next node.js stable version: 0.12

[c4milo]

[vvo]

Yes! :-)

[petehunt]

+1

[c4milo]

@vvo, @petehunt which version are you guys using?

[vvo]

Still 0.10.35 :-)

[pauliusuza]

+1, and io.js please

[metamatt]

I just sent #79 which ports this code to use nan. I see that as a necessary but not sufficient step to getting this working with current V8/node.js/io.js versions. (Not sufficient because the profiler API itself, which I don't know anything about and haven't looked into yet, has also changed and nan doesn't abstract that. So my PR uses nan where possible, but there are still a bunch of compile errors under 0.12.0 for missing profiler API methods.)

Here are the remaining errors when compiling #79 with node 0.12.0. (It compiles fine under node 0.10.x.)

[c4milo]

@metamatt great, this is a good start. I will look in detail tomorrow. Thank you!

[metamatt]

Here's my take on the remaining problems after the nan patch, from looking at deps/v8/include/v8-profiler.h in node 0.10.x (V8 3.14) and node 0.12.0 (V8 3.28):

Changed name (easy):

• interesting methods of HeapProfiler added “Heap” to the method name (GetSnapshot -> GetHeapSnapshot, etc)
• HeapProfiler::GetSnapshotsCount became GetSnapshotCount
• HeapProfiler::TakeHeapSnapshot (was TakeSnapshot) no longer takes “type” argument (and no longer has kFull constant to specify as “type” argument), has new global_object_name_resolver argument but it’s optional so hopefully not supplying it works out ok)

Became relative to Isolate instance (medium, just plumbing? But I don’t know where to plumb the Isolate instance in):

• the interesting methods of CpuProfiler (StartProfiling, StopProfiling, DeleteAllProfiles) and HeapProfiler (TakeHeapSnapshot, DeleteAllHeapSnapshots, GetHeapSnapshot, FindHeapSnapshot, GetSnapshotCount) were static methods, and are now instance methods, require the object instance which you can get from the Isolate (GetCpuProfiler(), GetHeapProfiler())

Removed with no obvious replacement (hard, if things need them? easy, if that means they won’t be called any more?):

• CpuProfile class has no getUid or GetBottomRoot (no mention of bottom-up profiling anywhere)
• CpuProfiler has no GetProfilesCount, GetProfile or FindProfile (no access to existing CpuProfile objects via CpuProfiler? Only way to get CpuProfile object is at time of acquiring profile, via CpuProfiler::StopProfiling?)
• CpuProfileNode has no GetTotalTime, GetSelfTime, GetTotalSamplesCount, GetSelfSamplesCount

[c4milo]

Very useful information @metamatt. It seems to me we will have to dig into Chromium's code as well, to see how they are currently using v8's profiler API, specifically for the Chrome Devtools backend. A lot has changed since this project was originally written. For instance, Isolates weren't really a thing when this project started.

Regarding your hard points:

• CpuProfile class has no getUid or GetBottomRoot (no mention of bottom-up profiling anywhere)

Was removed here: https://github.com/v8/v8/commit/acf9edb6d379237140c9b91c120694ef7287de3f#diff-f2be5615de603fba6f3f54c2c346f58d

CpuProfiler has no GetProfilesCount, GetProfile or FindProfile (no access to existing CpuProfile objects via CpuProfiler? Only way to get CpuProfile object is at time of acquiring profile, via CpuProfiler::StopProfiling?)

It seems you can get the CpuProfiler from the current Isolate instance, and the actual profile output once you stop profiling, exactly like you described it. Disregard if you already know, you get the current isolate like so: Isolate* isolate = Isolate::GetCurrent();

https://github.com/v8/v8/commit/0da5cad97c7df108a2ad20bd3d6c46e115cf91e7#diff-f2be5615de603fba6f3f54c2c346f58d

CpuProfileNode has no GetTotalTime, GetSelfTime, GetTotalSamplesCount, GetSelfSamplesCount

These were deprecated here:

• https://github.com/v8/v8/commit/55c661941de25717807c4d1651885db5b5e14ac6#diff-f2be5615de603fba6f3f54c2c346f58d
• https://github.com/v8/v8/commit/fb2d3630bcd5c04021ea6a2681e20c76c7b1e07c#diff-f2be5615de603fba6f3f54c2c346f58d
• https://github.com/v8/v8-git-mirror/commit/1a6dd16270640be7c114d1f70501789d37fe5c66#diff-f2be5615de603fba6f3f54c2c346f58d

[metamatt]

For the removed methods, I'm hoping a newer version of the frontend might not want to call these methods on the backend. For the Isolate instance, we can get the current Isolate and we can supply it to the V8 profiler API, as long as that's the correct one to supply; I'm wondering if we ever need to deal with a case where there's more than one and we have to keep track of multiple instances.

[c4milo]

For the removed methods, I'm hoping a newer version of the frontend might not want to call these methods on the backend.

It seems that's the main reason they took them out.

For the Isolate instance, we can get the current Isolate and we can supply it to the V8 profiler API, as long as that's the correct one to supply; I'm wondering if we ever need to deal with a case where there's more than one and we have to keep track of multiple instances.

No need to deal with more than one as far as I know. If people want to profile isolated processes they will have to import node-webkit-agent in those processes too.

[metamatt]

@c4milo do you want to call this fixed for 0.12 and publish a new version to npm?

[c4milo]

@metamatt is the CPU profiler still working or we will be loosing it for Node 0.12.x?

[c4milo]

@metamatt the other thing I haven't tested is whether the changes are backwards compatible with previous versions of node.

[metamatt]

CPU profiler is not working, but I don't see how to get it to work. If you can figure it out, great; otherwise I think heap profiler is better than nothing.

The changes I made are backwards compatible with node 0.10 (well, they work for me but if you have a better way to verify that, please do!).

[c4milo]

CPU profiler is not working, but I don't see how to get it to work. If you can figure it out, great; otherwise I think heap profiler is better than nothing.

Agreed. I'm going to give it another round of tests and release a new version as soon as I can. Thanks!