Bug 151713 - Implement a sampling profiler
Summary: Implement a sampling profiler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on: 151954
Blocks: 152628 152758 152761 152629 152729 152766
  Show dependency treegraph
 
Reported: 2015-12-01 12:11 PST by Saam Barati
Modified: 2016-01-10 22:50 PST (History)
19 users (show)

See Also:


Attachments
patch (62.49 KB, patch)
2015-12-07 12:04 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (82.53 KB, patch)
2015-12-18 15:52 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (86.74 KB, patch)
2015-12-21 17:20 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (90.65 KB, patch)
2015-12-31 15:12 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (101.09 KB, patch)
2016-01-05 12:57 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (107.03 KB, patch)
2016-01-05 14:07 PST, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (1.05 MB, application/zip)
2016-01-05 15:02 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (872.74 KB, application/zip)
2016-01-05 15:03 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.02 MB, application/zip)
2016-01-05 15:08 PST, Build Bot
no flags Details
WIP (96.92 KB, patch)
2016-01-06 13:23 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (97.66 KB, patch)
2016-01-06 13:31 PST, Saam Barati
no flags Details | Formatted Diff | Diff
WIP (97.63 KB, patch)
2016-01-06 14:13 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (99.81 KB, patch)
2016-01-06 19:06 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (111.87 KB, patch)
2016-01-06 19:47 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (110.97 KB, patch)
2016-01-06 19:58 PST, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff
patch (111.51 KB, patch)
2016-01-07 14:00 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (116.76 KB, patch)
2016-01-08 19:38 PST, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff
perf results with sampling profiler off (55.34 KB, text/plain)
2016-01-08 19:39 PST, Saam Barati
no flags Details
perf results with sampling profiler on (56.14 KB, text/plain)
2016-01-08 19:43 PST, Saam Barati
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-12-01 12:11:38 PST
The current plan is to build off of the architecture of the watchdog timer.
We would like to get to the performance level where we can sample once per X ms where
X is somewhere in 1-10 and we can have a 1-2% performance impact.
Comment 1 Saam Barati 2015-12-07 12:04:47 PST
Created attachment 266794 [details]
patch

it begins.
Comment 2 Oliver Hunt 2015-12-07 12:10:12 PST
Will you be recording stack shots? that would make me really really really happy
Comment 3 Saam Barati 2015-12-07 12:18:28 PST
(In reply to comment #2)
> Will you be recording stack shots? that would make me really really really
> happy

That's the plan and also what's currently happening.
I've also done some reading about data structures on how to organize stack frame histories.
I'm going to consider what we want internally in JSC, what the inspector wants,
and how to join those things together.
Comment 4 Joseph Pecoraro 2015-12-07 14:28:20 PST
Comment on attachment 266794 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266794&action=review

> Source/JavaScriptCore/inspector/ScriptDebugServer.h:61
> +    void setSamplerProfilerInterface(JSC::SampleProfilerInterface* s) { m_samplerProfilerInterface = s; }

Just put this in Debugger.h and make the member private. There really isn't a need for anything in ScriptDebugServer anymore.

> Source/JavaScriptCore/inspector/protocol/Timeline.json:82
> +                { "name": "sourceID", "type": "integer", "description": "sourceID uniquely identifying a function" },

The type here should probably be "$ref: Debugger.ScriptId" since that is what the frontend knows for scripts.

> Source/JavaScriptCore/interpreter/StackFrame.h:31
> +#include <wtf/text/StringBuilder.h>

Put in the cpp!

> Source/JavaScriptCore/interpreter/StackFrame.h:60
> +    JS_EXPORT_PRIVATE String toString(ExecState*);

I wonder if the exports are still needed.
Comment 5 Saam Barati 2015-12-18 15:52:51 PST
Created attachment 267654 [details]
WIP

So I've moved this to be a full fledged sampling profiler instead of one built on top of the watchdog timer.

So now, we pause the JS thread from another thread and observe its stack.

There is still a lot to be done but this patch current passes JSC stress tests with the sampling profiler enabled (it fails the API tests).

Some things I need to do:
- gather better data about why we throw away samples.
- implement some platform dependent code (currently, this only works on x86_64).
- consider a different timer mechanism not built on top of WTF::WorkQueue
- implement the new inspector API (currently, this is still based on the calling context tree, which is no longer going to be implemented).
Comment 6 Saam Barati 2015-12-21 17:20:06 PST
Created attachment 267767 [details]
WIP

We can now use the sampling profiler to look at real data inside the inspector.
The data currently displayed lies because we don't have perfect data with the sampling profiler.
After all, the sampling profiler does sampling, and not tracing, so I've just molded
the data into "tracing" form, just so the current inspector UI can show it.

To give it a try, run safari with "JSC_usSamplingProfiler=1".

Also, this patch adds a CallingContextTree data structure into the web inspector, written in JS.
I did this just because it's a really easy data structure to write and manipulate. It also makes
generating a CPUProfile trivially easy.
Comment 7 Saam Barati 2015-12-21 19:40:35 PST
(In reply to comment #0)
> The current plan is to build off of the architecture of the watchdog timer.
> We would like to get to the performance level where we can sample once per X
> ms where
> X is somewhere in 1-10 and we can have a 1-2% performance impact.

We are no longer building this off of the watchdog. We're now doing real
sampling from another thread.
Comment 8 Saam Barati 2015-12-31 15:12:43 PST
Created attachment 268057 [details]
WIP

Cleaned up some code.
Also, we now keep track of the total time spent sampling.
Comment 9 Saam Barati 2016-01-05 12:57:09 PST
Created attachment 268316 [details]
WIP

Cleaned up some code.
Added a testing infrastructure and one test.
Comment 10 WebKit Commit Bot 2016-01-05 13:14:01 PST
Attachment 268316 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:59:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:62:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:84:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:89:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:106:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:168:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:169:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:187:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:265:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:304:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:428:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:433:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:442:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:457:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:470:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:484:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:498:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/heap/MachineStackMarker.h:118:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:188:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:26:  #ifndef header guard has wrong style, please use: SamplingProfiler_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/inspector/InspectorTimelineAgent.h:42:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/VM.h:468:  The parameter name "lastStackTop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:347:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 27 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Saam Barati 2016-01-05 14:07:54 PST
Created attachment 268323 [details]
WIP

(seeing build errors on various platforms)
Comment 12 WebKit Commit Bot 2016-01-05 14:10:07 PST
Attachment 268323 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:59:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:88:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:105:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:167:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:168:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:186:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:264:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:304:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:431:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/heap/MachineStackMarker.cpp:188:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:26:  #ifndef header guard has wrong style, please use: SamplingProfiler_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/inspector/InspectorTimelineAgent.h:42:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/VM.h:468:  The parameter name "lastStackTop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:347:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 16 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Build Bot 2016-01-05 15:02:08 PST
Comment on attachment 268323 [details]
WIP

Attachment 268323 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/654613

Number of test failures exceeded the failure limit.
Comment 14 Build Bot 2016-01-05 15:02:14 PST
Created attachment 268328 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 15 Build Bot 2016-01-05 15:03:48 PST
Comment on attachment 268323 [details]
WIP

Attachment 268323 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/654604

Number of test failures exceeded the failure limit.
Comment 16 Build Bot 2016-01-05 15:03:55 PST
Created attachment 268330 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 17 Build Bot 2016-01-05 15:07:56 PST
Comment on attachment 268323 [details]
WIP

Attachment 268323 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/654629

Number of test failures exceeded the failure limit.
Comment 18 Build Bot 2016-01-05 15:08:01 PST
Created attachment 268331 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 19 Saam Barati 2016-01-06 13:23:18 PST
Created attachment 268398 [details]
WIP

More tests.
Remove some files and code that will be part of another patch.
Comment 20 WebKit Commit Bot 2016-01-06 13:25:34 PST
Attachment 268398 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:58:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:87:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:104:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:271:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:440:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:347:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Saam Barati 2016-01-06 13:31:37 PST
Created attachment 268399 [details]
WIP

one that probably builds.
Comment 22 WebKit Commit Bot 2016-01-06 13:52:52 PST
Attachment 268399 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:58:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:87:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:104:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:271:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:423:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:433:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:440:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:347:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 10 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Saam Barati 2016-01-06 14:13:26 PST
Created attachment 268404 [details]
WIP

check builds on non darwin platforms.
Comment 24 WebKit Commit Bot 2016-01-06 14:15:20 PST
Attachment 268404 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:58:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:87:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:104:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:271:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:423:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:433:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:440:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:347:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 10 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Gyuyoung Kim 2016-01-06 18:55:01 PST
Comment on attachment 268404 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=268404&action=review

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:523
> +    int rc = pthread_attr_getstack(&regs, &stackBase, &stackSize);

Basically EFL port has been working on POSIX. We already have used some pthread_foo functions. Thus, in my humble opinion, you can use this function for EFL port if *pthread_attr_getstack()* can support what you need to do.
Comment 26 Saam Barati 2016-01-06 19:06:53 PST
Created attachment 268435 [details]
patch
Comment 27 Saam Barati 2016-01-06 19:07:16 PST
(In reply to comment #26)
> Created attachment 268435 [details]
> patch
passes debug JSC tests now.
Comment 28 Saam Barati 2016-01-06 19:47:31 PST
Created attachment 268436 [details]
patch

it has a changelog.
Comment 29 Saam Barati 2016-01-06 19:58:59 PST
Created attachment 268437 [details]
patch
Comment 30 WebKit Commit Bot 2016-01-06 20:01:27 PST
Attachment 268437 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:58:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:87:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:102:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:203:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:272:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:441:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:347:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Saam Barati 2016-01-06 20:02:35 PST
Comment on attachment 268437 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268437&action=review

I'm putting this up for review because the contents of this patch will stay the same
except I'm going to add code to put the sampling profiler behind an #ifdef
to guard against unsupported linux functions (and a FIXME to turn it on under gtk/efl ports).

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:172
> +            // For the most part, we only fail here when we're looking
> +            // at the top most call frame. All other parent call frames
> +            // should have set the CallSiteIndex when making a call.
> +            //
> +            // FIXME:
> +            // Sometimes, though, we fail when we're at depth 1. Why is this?
> +            // RELEASE_ASSERT(m_depth == 0); // All frames at depth > 0 should have call site index bits in an accurate state.

Anybody know why this is?

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:539
> +    UNUSED_PARAM(startLine);
> +    UNUSED_PARAM(startColumn);
> +    UNUSED_PARAM(sourceID);
> +    UNUSED_PARAM(url);

I'll move these functions into another patch for when we land the inspector bits.

> Source/JavaScriptCore/tests/stress/sampling-profiler-basic.js:1
> +load("./sampling-profiler/samplingProfiler.js");

I also want to add a test and testing mechanism to make sure
we can test when the top-most frame is in RegExp code.

> Tools/Scripts/run-jsc-stress-tests:746
> +    run("ftl-no-cjit-validate", "--validateGraph=true", "--useSamplingProfiler=true", *(FTL_OPTIONS + NO_CJIT_OPTIONS)) if $enableFTL

Does this seem like a reasonable test run for this?
Or what would be better?
Comment 32 Filip Pizlo 2016-01-07 12:28:52 PST
Comment on attachment 268437 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268437&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        This patch implements a sampling profiler for JavaScriptCore.

Please clarify that this profiler is for end-user profiler UI and not for JIT profiling.

>> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:172
>> +            // RELEASE_ASSERT(m_depth == 0); // All frames at depth > 0 should have call site index bits in an accurate state.
> 
> Anybody know why this is?

Because we don't store the CallSiteIndex except when we are about to make a call or do something shady.  So, the CallSiteIndex for the top frame will usually be BS.  It will either be garbage or it will be the CallSiteIndex of the dynamically most recent shady thing that we did.

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:242
> +    SamplingProfiler::FrameType tryToGetExecutableFromCallee(JSValue value, ExecutableBase*& outExecutable)
> +    {
> +        auto isAllocatedGCObject = [&] (JSCell* cell) {
> +            return m_vm.heap.objectSpace().blocks().set(m_markedBlockSetLocker).contains(MarkedBlock::blockFor(cell));
> +        };
> +
> +        outExecutable = nullptr;
> +
> +        if (!value.isCell())
> +            return FrameType::Unknown;
> +        JSCell* calleeCell = value.asCell();
> +        if (!calleeCell)
> +            return FrameType::Unknown;
> +        if (!MarkedBlock::isAtomAligned(calleeCell))
> +            return FrameType::Unknown;
> +        if (!isAllocatedGCObject(calleeCell))
> +            return FrameType::Unknown;
> +
> +        auto frameTypeFromCallData = [&] () -> FrameType {
> +            FrameType result = FrameType::Unknown;
> +            CallData callData;
> +            CallType callType;
> +            callType = getCallData(calleeCell, callData);
> +            if (callType == CallTypeHost)
> +                result = FrameType::Host;
> +
> +            return result;
> +        };
> +        if (calleeCell->type() != JSFunctionType)
> +            return frameTypeFromCallData();
> +        ExecutableBase* executable = static_cast<JSFunction*>(calleeCell)->executable();
> +        if (!executable)
> +            return frameTypeFromCallData();
> +        if (!MarkedBlock::isAtomAligned(calleeCell))
> +            return frameTypeFromCallData();
> +        if (!isAllocatedGCObject(executable))
> +            return frameTypeFromCallData();
> +        if (!ExecutableBase::isExecutableType(executable->type()))
> +            return frameTypeFromCallData();
> +
> +        outExecutable = executable;
> +        return FrameType::HasExecutable;

This code is glorious.
Comment 33 Saam Barati 2016-01-07 14:00:24 PST
Created attachment 268484 [details]
patch

might be for landing, or I might beef up pointer validation
Comment 34 WebKit Commit Bot 2016-01-07 14:15:40 PST
Attachment 268484 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:60:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:89:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:104:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:205:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:275:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:444:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jsc.cpp:1648:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:86:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:347:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 9 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Geoffrey Garen 2016-01-07 14:21:25 PST
Comment on attachment 268484 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268484&action=review

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:228
> +            callType = getCallData(calleeCell, callData);

I think this line of code is a bug, and I'd like to suggest a small design change as a result.

First, it's a bug in practice because getCallData() calls value.asCell()->methodTable()->getCallData(), but the object's method table might be garbage.

Second, it's a bug in theory because the virtual getCallData() might do anything, and we can't guarantees safety if it does.

Here is a sad getCallData that you wouldn't want to encounter in a back alley, which you might sometimes encounter given C++ destructor rules:

CallType InternalFunction::getCallData(JSCell*, CallData&)
{
    RELEASE_ASSERT_NOT_REACHED();
    return CallTypeNone;
}

As a result, I think we need a post-processing step on the main thread that uses the heap's 100% reliable conservative GC functions to determine if objects are valid and of the right type. Any interesting interrogation of an object should happen after post-processing. Any interrogation of an object on the profiler thread should just be an optimization to avoid initially collecting lots of invalid stack traces.

You mentioned on IRC that CodeBlocks can always be 100% validated, so perhaps we only need to add the step I've described in the case where a CodeBlock is not present (assuming CodeBlocks are all we need for good stack traces).

As a separate matter, GC needs to mark our backtraces and be aware that it needs to mark them conservatively.
Comment 36 Geoffrey Garen 2016-01-07 14:24:35 PST
Comment on attachment 268484 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268484&action=review

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:371
> +void SamplingProfiler::visit(const LockHolder&, SlotVisitor& slotVisitor)
> +{
> +    ASSERT(m_lock.isLocked());
> +    for (ExecutableBase* executable : m_seenExecutables)
> +        slotVisitor.appendUnbarrieredReadOnlyPointer(executable);
> +}

Yeah, this looks bad. We need to validate that our executable is a 100% valid GC object and an ExecutableBase* before marking it. 99% certainty is OK for sampling, but not for GC. We should treat the sampling data structures just like we treat the conservative stack scan, and we should throw out samples that don't scan.
Comment 37 Saam Barati 2016-01-08 19:38:02 PST
Created attachment 268601 [details]
patch

addressing Geoff's comments.
Comment 38 WebKit Commit Bot 2016-01-08 19:38:59 PST
Attachment 268601 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:63:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:106:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:241:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:317:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.h:112:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/dfg/DFGNodeType.h:347:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 39 Saam Barati 2016-01-08 19:39:33 PST
Created attachment 268602 [details]
perf results with sampling profiler off
Comment 40 Saam Barati 2016-01-08 19:43:33 PST
Created attachment 268603 [details]
perf results with sampling profiler on

it's definitely getting more pwned than I expected.
I'm going to open up another bug after this that
will be targeted at making this faster. This has gotten
slower since the last time I checked its performance.

There look to be some programs that just get tank
in performance by having this on. I'll figure out why
that is. I think it's still worth landing this now,
and then addressing the performance in follow-up patches.
Comment 41 Saam Barati 2016-01-08 20:02:07 PST
Comment on attachment 268601 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268601&action=review

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:511
> +    m_vm.heap.stopAllocation();

This isn't quite right. I'll investigate. I'm getting some debug failures.
Also, reading its implementation, it seems wrong to call this outside
of GC.
Comment 42 Saam Barati 2016-01-08 20:12:41 PST
(In reply to comment #41)
> Comment on attachment 268601 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268601&action=review
> 
> > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:511
> > +    m_vm.heap.stopAllocation();
> 
> This isn't quite right. I'll investigate. I'm getting some debug failures.
> Also, reading its implementation, it seems wrong to call this outside
> of GC.

I think the solution is to use a HeapIterationScope. It
does the proper bookkeeping.
Comment 43 Saam Barati 2016-01-10 13:45:16 PST
(In reply to comment #42)
> (In reply to comment #41)
> > Comment on attachment 268601 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=268601&action=review
> > 
> > > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:511
> > > +    m_vm.heap.stopAllocation();
> > 
> > This isn't quite right. I'll investigate. I'm getting some debug failures.
> > Also, reading its implementation, it seems wrong to call this outside
> > of GC.
> 
> I think the solution is to use a HeapIterationScope. It
> does the proper bookkeeping.

Btw, it is the right thing to do. And that's what I have locally.
Comment 44 Saam Barati 2016-01-10 22:50:31 PST
landed in:
http://trac.webkit.org/changeset/194840