Bug 20175 – Fixes for Windows and non-AllInOne file build with SamplingTool, plus review fixes.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

patch following.

[Gavin Barraclough]

Created attachment 22485 [details]
patch

I think this should fix the issues for the last patch ( https://bugs.webkit.org/show_bug.cgi?id=19865 ).

I hope adding ENABLE_SAMPLING_TOOL to config.h (defaulted to off, of course) looks sensible – it ensures the switch is made available in all .cpps compiled, and makes enabling the tool nice and straightforward.

WebKit tests pass, jsc built and tested on windows, no degradation on SunSpider.

[Gavin Barraclough]

Created attachment 22486 [details]
SunSpider results (no degradation)

[Gavin Barraclough]

Created attachment 22487 [details]
minor fix - add missing padding for DUMP_OPCODE_STATS

[Gavin Barraclough]

Two quick notes:

(1) Capability on Windows – the sampler is functional on windows, though faces one limitation – windows appears to be restricted to a 60hz clock, limiting the number of samples produced.  Setting the sampler to a 60hz sample frequency on OS X produces very similar results to those generated on Windows – both being proportional to high frequency sampling results taken on OS X.

(2) The sampler works fine within safari – it is just a question of hooking up the same init/destruct code, as in Shell.cpp  (however I plan on doing this in a separate patch).  A convenient point to do this was the web inspector – as a quick way of testing this I modified the inspector such that when ENABLE_SAMPLING_TOOL is set, the profiling controls in the inspector start/stop the sampling tool, instead.

[Adam Roben (:aroben)]

Comment on attachment 22487 [details]
minor fix - add missing padding for DUMP_OPCODE_STATS

-#include "JSClassRef.h"
+#include "API/JSClassRef.h"

r35310 should have made this unnecessary.

[Gavin Barraclough]

(In reply to comment #5)
> r35310 should have made this unnecessary.

cheers Adam, will revert this file and retest.

[Gavin Barraclough]

Created attachment 22514 [details]
Alternative patch - single build version.

Geoff,

As a Sunday afternoon project I've taken another swing at a version of the sampler that does not require a separate build – I've not marked this as obsoleting the previous patch in case you don't like this idea and would prefer I stuck with the previous version, but I hope this does look good.

The patch makes the sampler always available without impacting on performance when not in use.  Introducing a new bytecode op_sample opcode resulted in a minor regression, so I've use a form of op_debug for the sampling hook.

The presence of the SamplingTool::dump code to print results degrades performance (I'm guessing I'm getting unlucky, and the larger ro_data section for the strings is shuffling the binary around – just including the array of opcode names is enough to give a minor hit), so I've left this compiled out by default on a switch ENABLE(SAMPLING_TOOL_DUMP).  With or without the dump code there are two new methods available - SamplingTool::getScopeSampleRecords and ScopeSampleRecord::getLineCounts - to extract results from the sampler.

By default the tool uses the op_debug/DoSample method to trigger sampling, planting a hook at the beginning of every line of javascript source-code – but an I've left a compile option ENABLE(SAMPLING_TOOL_ALWAYS) to allow the sampler to operate as it currently does.  Enabling the switch results a lower cost per-event, but it updates state every bytecode rather than for every line in the JS sourcecode – and as a result the two mechanisms have about the same overhead – setting ENABLE(SAMPLING_TOOL_ALWAYS) giving us a higher precision mechanism at the cost of requiring a special build (and likely only useful to us, since end users should not care which bytecode op a sample is falling on).

Hope this look good to you,
cheers, G.

[Geoffrey Garen]

Comment on attachment 22487 [details]
minor fix - add missing padding for DUMP_OPCODE_STATS

+// Set to 1 to enable the sampler, SamplingTool.
+#define ENABLE_SAMPLING_TOOL 0

It's best to put #defines like this in wtf/Platform.h, rather than config.h. config.h is just there for crazy header hacks -- for example, the MSVC rand_s hack.

--- JavaScriptCore/VM/SamplingTool.cpp	(revision 0)
+++ JavaScriptCore/VM/SamplingTool.cpp	(revision 0)

I think this means that SamplingTool.cpp made its way into the project through a local filesystem copy, followed by an "svn add." It's better to use "svn cp Opcode.h SamplingTool.cpp," because the latter will preserve the history of your original commits within the new source file.

+    class SamplingTool
+    {

Our style guidelines call for the "{" in a class declaration to go on the same line as the declaration, as in "class SamplingTool {."

r=me -- great improvements!

[Geoffrey Garen]

Comment on attachment 22514 [details]
Alternative patch - single build version.

There's a lot of overlap between attachment 22487 [details], 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 [details] 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 [details] should land first, and the tasks I've outlined here are pretty beefy, deserving their own bugs.

[Gavin Barraclough]

(In reply to comment #9)

Sounds good, raising new bug for the second patch & copying your comments there.

G.

[Gavin Barraclough]

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/GNUmakefile.am
Sending        JavaScriptCore/JavaScriptCore.exp
Sending        JavaScriptCore/JavaScriptCore.pri
Sending        JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj
Sending        JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
Sending        JavaScriptCore/JavaScriptCoreSources.bkl
Sending        JavaScriptCore/VM/Machine.cpp
Sending        JavaScriptCore/VM/Machine.h
Sending        JavaScriptCore/VM/Opcode.cpp
Sending        JavaScriptCore/VM/Opcode.h
Adding         JavaScriptCore/VM/SamplingTool.cpp
Adding         JavaScriptCore/VM/SamplingTool.h
Sending        JavaScriptCore/kjs/Shell.cpp
Sending        JavaScriptCore/kjs/nodes.cpp
Sending        JavaScriptCore/wtf/Platform.h
Transmitting file data ................
Committed revision 35454.

[Gavin Barraclough]

Per line sampler work will be moved to:

https://bugs.webkit.org/show_bug.cgi?id=20228