src: fix v8::CpuProfiler idle sampling

[OleksandrChekhovskyi]

Correctly mark idle wait as "(idle)" instead of "(program)", and take
samples during that time.

This makes profiling with node-inspector (via v8-profiler) actually useful.

---

Simple test program:

var http = require("http");

http.createServer(function(req, res) {
    res.end("Hello World\n");
}).listen(1337, "127.0.0.1");

console.log("node.js version: " + process.version);
console.log("Server running at http://127.0.0.1:1337/");

Testing was done by starting profiling, waiting few seconds, loading page once in the browser, waiting few more seconds and then stopping profiling.

What profiler showed before the fix (omg, several seconds of CPU time to handle one trivial request?):

After the fix (now actually looks like reality):

Please cherry-pick this fix to 2.x and 3.x - having working convenient profiling can be really useful.
The fix itself was in fact tested off 2.5.0, since v8-profiler package has not been updated yet for module ABI v45.

[thefourtheye]

What will happen in Windows?

[OleksandrChekhovskyi]

Probably it will always collect samples during idle time (I have not tested Windows).
Note that the original code above was POSIX-only as well. It is moved here so that it can be conditional on the command line flag '--prof' (so only block SIGPROF if using v8.log-style profiling).

[bnoordhuis]

Can you add a regression test? You can use test/addons/heap-profiler as a template.

[OleksandrChekhovskyi]

What would you exactly test in such a test? The API 'worked', it just returned data that didn't match the reality. It's not obvious how to verify that.

[bnoordhuis]

The v8::CpuProfile contains the raw timeline data, you can get it with v8::CpuProfile::GetSampleTimestamp(). I think that if the test calls setTimeout(function() {}, 100), it should see approximately 100 samples with your patch and zero or almost zero without.

[OleksandrChekhovskyi]

Possible indeed. 'Approximately' and 'almost', however, make me nervous.
Are you sure it's worth the trouble?

[bnoordhuis]

A test would be good if you don't want it to regress over time.

[OleksandrChekhovskyi]

Ok, I'll try to make something on Monday.

[OleksandrChekhovskyi]

The patch has been updated.
Now includes a test that verifies that samples collected with v8::CpuProfiler are evenly distributed.

In the future it might be possible to make it work with uneven samples. v8::CpuProfiler now reports sample timestamps, and they are supported by current Chrome DevTools, however it'll require updating v8-profiler and node-inspector, and that's much more work. Also since that would be a breaking behavior change, ideally it should only be enabled explicitly by the user.

So let's keep it simple for now, as it enables easy/convenient profiling immediately (assuming the patch is cherry-picked to stable branches).

[bnoordhuis]

There are a bunch of style errors in this file but once #2427 lands, make cpplint should tell you about them.

[OleksandrChekhovskyi]

Style errors have been fixed, unconditional idle notifier undone, commit message updated.

[bnoordhuis]

LGTM and thanks. Linking to #2428 because I'd like to see that land first.

[bnoordhuis]

CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/152/

[OleksandrChekhovskyi]

It looks like the build failed because it failed to fetch the source code from git.

[bnoordhuis]

GH is pretty flaky today. Second try: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/154/

[bnoordhuis]

The freebsd101-64 failure is #2464 but I see the test from this PR failing on the fedora22 buildbot: https://jenkins-iojs.nodesource.com/job/node-test-commit-linux/272/nodes=fedora22/console (grep for 'not ok'.)

[OleksandrChekhovskyi]

Not sure why would it fail. This might just mean that testing timing-related stuff in a unit test is not a good idea.

[bnoordhuis]

V8 has CPU profiler tests that run on a wide range of platforms so it must be possible to write something that isn't timing sensitive.

[OleksandrChekhovskyi]

I couldn't think of any reliable way to validate this in a unit test, so I have dropped the test in the updated patch.

It needs to check either number of samples, timings between samples, or presence of samples within certain intervals, and neither of these are reliable.

I have checked V8 CPU profiler tests and those only check for specifics only if samples have been manually recorded by the code in the test.

[OleksandrChekhovskyi]

Changes to v8-profiler have been upstreamed now (node-inspector/v8-profiler#73), so now only this patch is missing from the 'working profiler' picture.

[bnoordhuis]

Thanks Oleksandr, landed in 8e14135.