Bug 151713 – Implement a sampling profiler

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 266794 [details]
patch

it begins.

[Oliver Hunt]

Will you be recording stack shots? that would make me really really really happy

[Saam Barati]

(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.

[Joseph Pecoraro]

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.

[Saam Barati]

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).

[Saam Barati]

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.

[Saam Barati]

(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.

[Saam Barati]

Created attachment 268057 [details]
WIP

Cleaned up some code.
Also, we now keep track of the total time spent sampling.

[Saam Barati]

Created attachment 268316 [details]
WIP

Cleaned up some code.
Added a testing infrastructure and one test.

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 268323 [details]
WIP

(seeing build errors on various platforms)

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Saam Barati]

Created attachment 268398 [details]
WIP

More tests.
Remove some files and code that will be part of another patch.

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 268399 [details]
WIP

one that probably builds.

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 268404 [details]
WIP

check builds on non darwin platforms.

[WebKit Commit Bot]

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.

[Gyuyoung Kim]

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.

[Saam Barati]

Created attachment 268435 [details]
patch

[Saam Barati]

(In reply to comment #26)
> Created attachment 268435 [details]
> patch
passes debug JSC tests now.

[Saam Barati]

Created attachment 268436 [details]
patch

it has a changelog.

[Saam Barati]

Created attachment 268437 [details]
patch

[WebKit Commit Bot]

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.

[Saam Barati]

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?

[Filip Pizlo]

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.

[Saam Barati]

Created attachment 268484 [details]
patch

might be for landing, or I might beef up pointer validation

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Saam Barati]

Created attachment 268601 [details]
patch

addressing Geoff's comments.

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 268602 [details]
perf results with sampling profiler off

[Saam Barati]

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.

[Saam Barati]

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.

[Saam Barati]

(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.

[Saam Barati]

(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.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/194840