Bug 153636

Summary: JSC Sampling Profiler: First Bound Function name inadvertently appears where all bound functions get call
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: joepeck, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[TEST] Reduction
none
[IMAGE] Profile View none

Description Joseph Pecoraro 2016-01-28 21:21:15 PST
Created attachment 270178 [details]
[TEST] Reduction

* SUMMARY
First Bound Function name inadvertently appears where all bound functions get call.

* STEPS TO REPRODUCE
1. Profile attached test case [bind-reduction.html]

* TEST
The test creates two bound functions:

    this._boundMethodOne = this.methodOne.bind(this);
    this._boundMethodTwo = this.methodTwo.bind(this);

It then calls both of them, which spins for 100ms each:

    this._boundMethodOne();
    this._boundMethodTwo();

The CallingContextTree contains a Native frame above the actual functions. This native frame, unfortunately, gets the name of the first bound function that was sampled. So the sample looks like:

    => [f] doStuff
      => [N] methodOne (this is the magic bound function)
        => [f] methodOne
        => [f] methodTwo

I would have expected either of these, probably preferring (1):

    (1) The "bound" function implementation detail doesn't appear in the trace:

    => [f] doStuff
      => [f] methodOne
      => [f] methodTwo

    (2) The "bound" function appears in the trace, with the name of each instance

    => [f] doStuff
      => [N] methodOne
        => [f] methodOne
      => [N] methodTwo
        => [f] methodTwo

It would be unfortunate for each bound function call to have multiple call frames, so I think (2) should be avoided.
Comment 1 Joseph Pecoraro 2016-01-28 21:21:37 PST
Created attachment 270179 [details]
[IMAGE] Profile View
Comment 2 Joseph Pecoraro 2016-01-28 21:25:50 PST
Also reasonable, and probably the most accurate:

    (3) The "bound" function appears in the trace, with its name demarcated:

    => [f] doStuff
      => [N] bound methodOne
        => [f] methodOne
      => [N] bound methodTwo
        => [f] methodTwo

One of the things in the ES6 compat table is that (foo.bind()).name is "bound foo", so this would make sense. But we would probably offer some frontend convenience to merge bound function call frames with the call to the target function, and still end up with (1) eventually.
Comment 3 Radar WebKit Bug Importer 2016-01-28 21:26:15 PST
<rdar://problem/24404539>
Comment 4 Saam Barati 2016-02-02 15:43:55 PST
This will be solved in:
https://bugs.webkit.org/show_bug.cgi?id=153663
now that http://trac.webkit.org/changeset/196033 landed

*** This bug has been marked as a duplicate of bug 153663 ***
Comment 5 Saam Barati 2016-02-02 15:44:27 PST
(In reply to comment #2)
> Also reasonable, and probably the most accurate:
> 
>     (3) The "bound" function appears in the trace, with its name demarcated:
> 
>     => [f] doStuff
>       => [N] bound methodOne
>         => [f] methodOne
>       => [N] bound methodTwo
>         => [f] methodTwo
> 
> One of the things in the ES6 compat table is that (foo.bind()).name is
> "bound foo", so this would make sense. But we would probably offer some
> frontend convenience to merge bound function call frames with the call to
> the target function, and still end up with (1) eventually.

This is what it will look like