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.
Created attachment 266794 [details] patch it begins.
Will you be recording stack shots? that would make me really really really happy
(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 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.
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).
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.
(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.
Created attachment 268057 [details] WIP Cleaned up some code. Also, we now keep track of the total time spent sampling.
Created attachment 268316 [details] WIP Cleaned up some code. Added a testing infrastructure and one test.
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.
Created attachment 268323 [details] WIP (seeing build errors on various platforms)
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 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.
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 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.
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 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.
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
Created attachment 268398 [details] WIP More tests. Remove some files and code that will be part of another patch.
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.
Created attachment 268399 [details] WIP one that probably builds.
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.
Created attachment 268404 [details] WIP check builds on non darwin platforms.
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 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(®s, &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.
Created attachment 268435 [details] patch
(In reply to comment #26) > Created attachment 268435 [details] > patch passes debug JSC tests now.
Created attachment 268436 [details] patch it has a changelog.
Created attachment 268437 [details] patch
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 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 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.
Created attachment 268484 [details] patch might be for landing, or I might beef up pointer validation
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 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 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.
Created attachment 268601 [details] patch addressing Geoff's comments.
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.
Created attachment 268602 [details] perf results with sampling profiler off
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 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.
(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.
(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.
landed in: http://trac.webkit.org/changeset/194840