Bug 153663

Summary: JSC Sampling Profiler: (host) is confusing in cases where I would expect to see JS name
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ggaren, joepeck, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 152629    
Bug Blocks:    
Attachments:
Description Flags
[TEST] Host Reduction
none
[IMAGE] Sampling Profiler shows "(host)"
none
[IMAGE] Legacy Profiler shows "Error"
none
patch
none
patch
none
patch
none
patch
none
patch ggaren: review+

Description Joseph Pecoraro 2016-01-29 12:28:54 PST
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?
Comment 1 Joseph Pecoraro 2016-01-29 12:29:18 PST
Created attachment 270241 [details]
[IMAGE] Sampling Profiler shows "(host)"
Comment 2 Joseph Pecoraro 2016-01-29 12:29:39 PST
Created attachment 270242 [details]
[IMAGE] Legacy Profiler shows "Error"
Comment 3 Radar WebKit Bug Importer 2016-01-29 12:30:15 PST
<rdar://problem/24415092>
Comment 4 Saam Barati 2016-01-29 12:58:26 PST
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.
Comment 5 Saam Barati 2016-02-02 15:43:55 PST
*** Bug 153636 has been marked as a duplicate of this bug. ***
Comment 6 Saam Barati 2016-02-03 17:03:20 PST
Created attachment 270611 [details]
patch
Comment 7 Saam Barati 2016-02-03 17:04:27 PST
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.
Comment 8 WebKit Commit Bot 2016-02-03 17:05:47 PST
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 9 Joseph Pecoraro 2016-02-03 18:16:51 PST
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.
Comment 10 Saam Barati 2016-02-03 21:20:47 PST
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 11 Saam Barati 2016-02-04 10:52:49 PST
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.
Comment 12 Saam Barati 2016-02-04 10:57:43 PST
Created attachment 270669 [details]
patch
Comment 13 Geoffrey Garen 2016-02-04 10:58:06 PST
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.
Comment 14 WebKit Commit Bot 2016-02-04 10:58:51 PST
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.
Comment 15 Geoffrey Garen 2016-02-04 11:01:13 PST
> 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.
Comment 16 Saam Barati 2016-02-04 11:12:13 PST
(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?
Comment 17 Saam Barati 2016-02-04 11:13:18 PST
(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.
Comment 18 Joseph Pecoraro 2016-02-04 11:29:51 PST
> 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
Comment 19 Joseph Pecoraro 2016-02-04 11:31:13 PST
Another relevant discussion:
https://code.google.com/p/chromium/issues/detail?id=559532
Comment 20 Saam Barati 2016-02-04 11:36:56 PST
I'm going to implement .displayName
Comment 21 Saam Barati 2016-02-04 13:16:39 PST
Created attachment 270686 [details]
patch

with displayName implemented.
Comment 22 WebKit Commit Bot 2016-02-04 13:18:28 PST
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.
Comment 23 Saam Barati 2016-02-04 13:21:18 PST
Created attachment 270687 [details]
patch
Comment 24 WebKit Commit Bot 2016-02-04 13:23:07 PST
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.
Comment 25 Saam Barati 2016-02-04 13:27:15 PST
Created attachment 270690 [details]
patch

more displayName tests.
Comment 26 WebKit Commit Bot 2016-02-04 13:30:16 PST
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 27 Geoffrey Garen 2016-02-04 13:30:31 PST
Comment on attachment 270690 [details]
patch

r=me
Comment 28 Saam Barati 2016-02-04 13:52:01 PST
landed in:
http://trac.webkit.org/changeset/196147
Comment 29 Joseph Pecoraro 2016-02-04 18:42:07 PST
(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());
...
}
Comment 30 Saam Barati 2016-02-04 19:59:00 PST
I will fix this later tonight.
Comment 31 Saam Barati 2016-02-05 13:42:02 PST
follow up speculative fix:
http://trac.webkit.org/changeset/196188