Created attachment 270240 [details] [TEST] Host Reduction * SUMMARY "(host)" is confusing in cases where I would expect to see JS name. * TEST: <script> setTimeout(function() { for (var i = 0; i < 10000; ++i) new Error(); }, 100); </script> * NOTES - Legacy Profiler shows "Error" - Sampling Profiler shows "(host)" Is there some way we can get some display name for certain host functions?
Created attachment 270241 [details] [IMAGE] Sampling Profiler shows "(host)"
Created attachment 270242 [details] [IMAGE] Legacy Profiler shows "Error"
<rdar://problem/24415092>
I think this should be doable. After my PC=>CodeOrigin mapping patch lands, we'll have an easy mechanism to do this. We can always log callee when we're taking a stack trace, and then get properties from callee (if it won't execute JS code) while transforming an unprocessed stack frame into a processed stack frame.
*** Bug 153636 has been marked as a duplicate of this bug. ***
Created attachment 270611 [details] patch
Comment on attachment 270611 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270611&action=review > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:325 > + DeferGCForAWhile deferGC(m_vm.heap); this isn't needed, removed locally.
Attachment 270611 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:40: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:368: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270611 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270611&action=review Looks good to me, but should probably get a JSC reviewer for JS object lifetimes. > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:583 > + name = getPropertyIfPureOperation(vm.propertyNames->displayName); Oh cool! Does this mean if you set the `displayName` property of a function it will use that name? If so, it seems `displayName` should be preferred over `name`, and there should be a test for it. It used to be used in the old profiler. If you created an anonymous function you could name it: var f = function(){}; f.displayName = "myFunction"; // in profiler it would display as `myFunction` > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:595 > + { > + String name = nameFromCallee(vm); > + if (!name.isEmpty()) > + return name; > + } Why is this in an anonymous block? > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:622 > + { > + String name = nameFromCallee(vm); > + if (!name.isEmpty()) > + return name; > + } Why is this in an anonymous block? > Source/JavaScriptCore/runtime/SamplingProfiler.h:97 > ExecutableBase* executable; Nit: Might as well make executable use initialize syntax as well.
After speaking with Joe offline, I've decided to get rid of querying the "displayName" property. To do the query the Right Way, we'd need to keep around all Callees that we see. It's better not to do that and just rely on CodeBlock having a sensible name.
Comment on attachment 270611 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=270611&action=review >> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:595 >> + } > > Why is this in an anonymous block? It's because it's not needed below this, so I just like the block to explicitly create a correspondence between where the variable is in scope with where the variable is needed.
Created attachment 270669 [details] patch
View in context: https://bugs.webkit.org/attachment.cgi?id=270611&action=review > Source/JavaScriptCore/ChangeLog:14 > + We then ask the callee for it's name when materializing its > Source/JavaScriptCore/ChangeLog:16 > + good names for frames with callee's being bound functions, callees > Source/JavaScriptCore/runtime/SamplingProfiler.cpp:586 > + if (!name.isEmpty()) > + return name; > + return String(); This code seems redundant. Do we have a good reason to turn the empty string into the null string? If not, you can just "return name" here. We should also query the inferred named, which I believe is stored on the executable.
Attachment 270669 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:365: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
> After speaking with Joe offline, I've decided > to get rid of querying the "displayName" > property. To do the query the Right Way, we'd need > to keep around all Callees that we see. It's better > not to do that and just rely on CodeBlock having > a sensible name. I don't understand this. What's wrong with keeping callees alive until we process the stack? Also, doesn't addCallee keep the callee alive? Also, doesn't stackFrame.callee keep the callee alive? .displayName and .name are valuable features, so it would be nice to keep them.
(In reply to comment #15) > > After speaking with Joe offline, I've decided > > to get rid of querying the "displayName" > > property. To do the query the Right Way, we'd need > > to keep around all Callees that we see. It's better > > not to do that and just rely on CodeBlock having > > a sensible name. > > I don't understand this. > > What's wrong with keeping callees alive until we process the stack? > > Also, doesn't addCallee keep the callee alive? > > Also, doesn't stackFrame.callee keep the callee alive? > > .displayName and .name are valuable features, so it would be nice to keep > them. This patch only keeps the callee alive if it doesn't find a CodeBlock in the call frame. We're able to get the name from most Executables, hence, we don't need to keep around all Callee instances. If we don't have a CodeBlock in the CallFrame, only then will we keep Callee alive and later ask it for its name. Obviously this is work-load dependent, but it's somewhat rare that we keep around the Callee. I don't think there is a strong and compelling use case for us keeping around all callee's just to support the .displayName property. JavaScript spec already mandates assigning good names to the .name property, which we support if we don't find a CodeBlock. Even then, there are actually some cases where we propagate better information to our Executable's inferredName() than to individual JSFunction instances' .name property (this might be a bug in our implementation of the spec). I just don't see compelling enough of use cases for supporting .displayName, maybe you have some use cases that I'm not thinking of?
(In reply to comment #13) > View in context: > https://bugs.webkit.org/attachment.cgi?id=270611&action=review > We should also query the inferred named, which I believe is stored on the > executable. We do query inferredName() already.
> I don't think there is a strong and compelling use case for us > keeping around all callee's just to support the .displayName property. > JavaScript spec already mandates assigning good names to the .name property, > which we support if we don't find a CodeBlock. Even then, there are actually > some > cases where we propagate better information to our Executable's > inferredName() than > to individual JSFunction instances' .name property (this might be a bug in > our implementation > of the spec). I just don't see compelling enough of use cases for supporting > .displayName, > maybe you have some use cases that I'm not thinking of? I think the most compelling case of using displayName was with Objective-J. They set function names to something akin to the Objective-C selector names, which contain spaces, not possible in a typical JavaScript function name: http://www.alertdebugging.com/2009/04/29/building-a-better-javascript-profiler-with-webkit/ JavaScript libraries have seemed to settle on "name inference is good enough". Here are a couple examples: https://github.com/jashkenas/coffeescript/issues/1767 https://github.com/marionettejs/backbone.marionette/issues/2425 But clearly there are some people looking forward to displayName, as the Chromium bug on this has discussion as of a couple days ago with relation to arrow functions: https://code.google.com/p/chromium/issues/detail?id=17356
Another relevant discussion: https://code.google.com/p/chromium/issues/detail?id=559532
I'm going to implement .displayName
Created attachment 270686 [details] patch with displayName implemented.
Attachment 270686 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:348: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:372: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270687 [details] patch
Attachment 270687 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:348: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:372: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270690 [details] patch more displayName tests.
Attachment 270690 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:348: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/SamplingProfiler.cpp:372: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270690 [details] patch r=me
landed in: http://trac.webkit.org/changeset/196147
(In reply to comment #28) > landed in: > http://trac.webkit.org/changeset/196147 inspector/sampling-profiler/many-call-frames.html Seems this is causing an assert on the Debug Bots: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r196163%20(2743)/inspector/sampling-profiler/many-call-frames-crash-log.txt Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 WTFCrash + 39 (Assertions.cpp:321) 1 JSC::MarkedAllocator::allocateSlowCase(unsigned long) + 85 (MarkedAllocator.cpp:146) 2 JSC::MarkedAllocator::allocate(unsigned long) + 74 (MarkedAllocator.h:115) 3 JSC::MarkedSpace::allocateWithDestructor(unsigned long) + 41 (MarkedSpace.h:220) 4 JSC::Heap::allocateWithDestructor(unsigned long) + 118 (HeapInlines.h:179) 5 void* JSC::Heap::allocateObjectOfType<JSC::PropertyTable>(unsigned long) + 29 (HeapInlines.h:198) 6 void* JSC::allocateCell<JSC::PropertyTable>(JSC::Heap&, unsigned long) + 143 (JSCellInlines.h:133) 7 void* JSC::allocateCell<JSC::PropertyTable>(JSC::Heap&) + 28 (JSCellInlines.h:145) 8 JSC::PropertyTable::create(JSC::VM&, unsigned int) + 31 (PropertyTable.cpp:42) 9 JSC::Structure::createPropertyMap(JSC::GCSafeConcurrentJITLocker const&, JSC::VM&, unsigned int) + 151 (Structure.cpp:1033) 10 JSC::Structure::materializePropertyMap(JSC::VM&) + 444 (Structure.cpp:349) 11 JSC::Structure::materializePropertyMapIfNecessary(JSC::VM&, JSC::PropertyTable*&) + 325 (Structure.h:634) 12 JSC::Structure::get(JSC::VM&, JSC::PropertyName, unsigned int&, bool&) + 195 (StructureInlines.h:99) 13 JSC::Structure::get(JSC::VM&, JSC::PropertyName, unsigned int&) + 57 (StructureInlines.h:89) 14 JSC::JSObject::inlineGetOwnPropertySlot(JSC::VM&, JSC::Structure&, JSC::PropertyName, JSC::PropertySlot&) + 71 (JSObject.h:1099) 15 JSC::JSObject::getOwnPropertySlot(JSC::JSObject*, JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) + 90 (JSObject.h:1140) 16 JSC::JSFunction::getOwnPropertySlot(JSC::JSObject*, JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) + 94 (JSFunction.cpp:364) 17 JSC::JSObject::fastGetOwnPropertySlot(JSC::ExecState*, JSC::VM&, JSC::Structure&, JSC::PropertyName, JSC::PropertySlot&) + 160 (JSObject.h:1151) 18 JSC::JSObject::getPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) + 156 (JSObject.h:1163) 19 JSC::SamplingProfiler::StackFrame::nameFromCallee(JSC::VM&)::$_7::operator()(JSC::Identifier const&) const + 148 (SamplingProfiler.cpp:583) 20 JSC::SamplingProfiler::StackFrame::nameFromCallee(JSC::VM&) + 138 (SamplingProfiler.cpp:593) 21 JSC::SamplingProfiler::StackFrame::displayName(JSC::VM&) + 62 (SamplingProfiler.cpp:604) void* MarkedAllocator::allocateSlowCase(size_t bytes) { ASSERT(m_heap->vm()->currentThreadIsHoldingAPILock()); ... }
I will fix this later tonight.
follow up speculative fix: http://trac.webkit.org/changeset/196188