[webkit-reviews] review cancelled: [Bug 20175] Fixes for Windows and non-AllInOne file build with SamplingTool, plus review fixes. : [Attachment 22514] Alternative patch - single build version.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has cancelled Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 20175: Fixes for Windows and non-AllInOne file build with SamplingTool,
plus review fixes.
https://bugs.webkit.org/show_bug.cgi?id=20175

Attachment 22514: Alternative patch - single build version.
https://bugs.webkit.org/attachment.cgi?id=22514&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
There's a lot of overlap between attachment 22487, which fixes a bug and does
some cleanup, and this patch, which does those things and also adds two new
features: per-line sampling, and sampling built in without a special binary.

I'd recommend landing attachment 22487 first. Then, it will be easier discuss
the specifics of a patch that just implements per-line sampling without a
special binary (probably worth a separate bug report).

Generally, the DoSample opcode code in this patch looks sound. I'm surprised
that the extra branches for callingHostFunction are not costly in the
non-sampling case, but oh well :).

I don't really think "ENABLE_SAMPLING_TOOL_ALWAYS" captures the difference
between these two modes well. One mode samples each opcode, to show VM
engineers what's hot in the engine; the other mode samples each source line, to
show webpage engineers what's hot in their JavaScript. At least, that's how I
think of it. Do you think of per-line sampling as a feature for VM engineers?

To be fully viable, I think this second mode -- the per-line mode -- would need
to solve three more problems:

(1) Dynamically recompile functions that were compiled before sampling started,
so they get sampled, too. (The current user-visible profiler is able to profile
a page without reloading it, so the per-line profiler would need to match that
ability.)

(2) Remove the existing JavaScript profiler hooks and / or merge with them, to
provide a consistent JavaScriptCore API to the Web Inspector. (This will also
be a ~1% performance win.)

(3) Web Inspector UI for per-line profiles. Tim Hatcher, Kevin McCullough, and
Adam Roben are the experts on this.

I'm going to clear the review flag, since I think attachment 22487 should land
first, and the tasks I've outlined here are pretty beefy, deserving their own
bugs.