Bug 153663 – JSC Sampling Profiler: (host) is confusing in cases where I would expect to see JS name

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

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

Created attachment 270241 [details]
[IMAGE] Sampling Profiler shows "(host)"

[Joseph Pecoraro]

Created attachment 270242 [details]
[IMAGE] Legacy Profiler shows "Error"

[Radar WebKit Bug Importer]

<rdar://problem/24415092>

[Saam Barati]

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.

[Saam Barati]

*** Bug 153636 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 270611 [details]
patch

[Saam Barati]

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.

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

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.

[Saam Barati]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 270669 [details]
patch

[Geoffrey Garen]

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.

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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

[Saam Barati]

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

[Saam Barati]

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

[Joseph Pecoraro]

> 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

[Joseph Pecoraro]

Another relevant discussion:
https://code.google.com/p/chromium/issues/detail?id=559532

[Saam Barati]

I'm going to implement .displayName

[Saam Barati]

Created attachment 270686 [details]
patch

with displayName implemented.

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 270687 [details]
patch

[WebKit Commit Bot]

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.

[Saam Barati]

Created attachment 270690 [details]
patch

more displayName tests.

[WebKit Commit Bot]

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.

[Geoffrey Garen]

Comment on attachment 270690 [details]
patch

r=me

[Saam Barati]

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

[Joseph Pecoraro]

(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());
...
}

[Saam Barati]

I will fix this later tonight.

[Saam Barati]

follow up speculative fix:
http://trac.webkit.org/changeset/196188