Bug 158555 – Web Inspector: Call Trees view should have a 'Top Functions'-like mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Saam Barati]

Working on UI and implementation now

[Radar WebKit Bug Importer]

<rdar://problem/26712544>

[Saam Barati]

Created attachment 280869 [details]
screenshot 1

[Saam Barati]

Created attachment 280870 [details]
screenshot 2

[Saam Barati]

Created attachment 280871 [details]
screenshot 3

[Saam Barati]

Created attachment 280872 [details]
screenshot 4

[Saam Barati]

Anybody have ideas for the button titles?
Joe, Mark and I spent some time discussing this today, but haven't found a good solution yet.

[Saam Barati]

Created attachment 280873 [details]
WIP

[Joseph Pecoraro]

Comment on attachment 280873 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=280873&action=review

> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:72
> +        } else if (this._type === WebInspector.CallingContextTree.Type.TopFunctionsTopDown){

Style: At this point we should use a switch statement.

> Source/WebInspectorUI/UserInterface/Views/ScriptProfileTimelineView.js:199
> +    Hierarchy: "hierarchy",
> +    TopFunctions: "top-functions",

These should be in a separate enum. Probably something like:

    WebInspector.ScriptProfileTimelineView.ProfileViewType = { ... }

And update things that refer to it as orientation. The patch itself looks great!

[Timothy Hatcher]

Maybe I am dense, but I don't understand what this adds or shows that is different. Can you write up or link to something that explains this mode?

A suggestion on UI, we should use select menus if we are adding another picker on the side like this. Adding 4 total buttons like this takes up too much space.

[Saam Barati]

(In reply to comment #9)
> Maybe I am dense, but I don't understand what this adds or shows that is
> different. Can you write up or link to something that explains this mode?

Top Functions is the best view I've found for doing performance analysis work.
Specifically, I've been heavily using this view for the past 2 weeks in Instruments
when doing some performance work. Geoff turned me onto it, and now I use it almost
exclusively over the Bottom Up view.

It's hard to explain exactly what Top Functions is in just words (I will try though).
I encourage everyone to apply this patch locally and try it out.

Top Functions works by treating every frame as a root in the tree.
Top functions view then presents a list of "roots". This is the same
as all other views, which also present a list of roots, but in this case,
every frame is a root. I find that this view is like a grown up version of Bottom Up view.
Bottom Up is great for nailing in specific performance problems in exactly
one function. But Bottom Up doesn't give you good context about where
a specific frame is in the call tree and how frames are related by having
a shared callers or some path of shared callers. For example, consider this call tree:
         (program)
         /        \
        /          \
   (many nodes...)  
      / 
     /
    (parent)
    /   \
   /     \
 (leaf1)  (leaf2)

Suppose that 'leaf1' is super hot, and 'leaf2' is moderately hot.
If we look at this through Bottom Up view, we will see 'leaf1'
is super hot, but it will take more scrolling to see that 'leaf2'
is moderately hot. Lets say that 'parent' is also moderately hot,
but that the majority of its time isn't self time.
With Bottom Up view, there is no good way to see that 'leaf1' and 'leaf2'
are both nodes under 'parent'. With Top Down, you can find this information,
but it requires a ton of drilling down into the tree (i.e, you must
expand past the 'many nodes...' I drew above). It's inconvenient to
use Top Down here for indentation alone. Bottom up will tell you that
'leaf1' is super hot, and that 'leaf2' and 'parent' are moderately
hot, but it doesn't show how they're related in the original tree.
It's important to see that 'parent's total time is very high
because it itself is moderately hot, and it has a child node
that is super hot, and another child that's moderately 'hot'.
For the sake of this example, let's pretend that 85% of the program's
time is spend inside 'parent'. Seeing this information through
'Top Functions' is super easy. Specifically, when using 'Top Functions'
sorted by Total Time. Because every node is a root, there will be a top-level
entry for every frame in the program. Specifically, there will be a top-level
node for 'parent' in my above example. Because I've sorted this view
by Total Time, I will see '(program)' first. That's because 100% of execution
time is under '(program)' frame (obviously). Then, I might see a few other nodes that
also run the entire time because '(program)' calls them, and they eventually
call into other things that never leave the stack. These will also have 
time ranges near 100%. But, only a few nodes after that, I'll see 'parent'
in the list because it accounts for 85% of execution time. Immediately,
I will see that it has some self time, and that it has two child nodes that have
self time. This is really helpful.

Let's consider another example where not even Top Down view works:
           (program)
            /  |  \
         (... many nodes...)
          /           \
    (many nodes...)   (many nodes...)
         /             \
       parent         parent
         |              |
        leaf1          leaf2


If we viewed this program in Top Down, we don't get a full picture of 'parent'
because it has its time distributed in two different subsections of the tree.
Specifically, lets say it has 70% of time in the leaf1 path, and 30% of the
time in the leaf2 path. We want a way to see these things together. It's impossible
to do this Top Down or Bottom Up. But, in Top Functions view, we get the view that
we want to see because we treat 'parent' as a root of the tree. Because we do this,
we will create the following sub tree in the Top Functions view:
        parent
       /      \
     leaf1   leaf2
This happens naturally because when 'parent' is a root, we add all its children
to its subtree.

Again, it's somewhat hard to explain this, it's really helpful to play
around with the view.

Constructing this tree is actually really easy. What we do is take
any arbitrary stack trace of length n, and treat is as n separate stack
traces. Specifically, we do the following thing for any stack S.

S = [A, B, C, D]
(A is the entry frame, and D is the top of the stack).
We will transform this into a list of stack traces S' like so:
S' = [[A, B, C, D], [B, C, D], [C, D], [D]]

If we then run the normal top down tree algorithm on this set of stack
traces, all nodes get treated as roots.

(Note that I've specifically been talking about Top Functions--Top Down
view in my above examples. Similar principles apply to Top Functions--Bottom Up view.
Though I personally find Top Functions--Top Down more useful, having 
Top Functions--Bottom Up provides context and where things are called from.)


> 
> A suggestion on UI, we should use select menus if we are adding another
> picker on the side like this. Adding 4 total buttons like this takes up too
> much space.
I tried this with Joe, and I actually think it's worse. It takes more clicks to
switch between views; it's less discoverable; and I think it discourages users
from using the "non default" views. If you apply the patch locally you can easily
flip it to that mode by switching the booleans in the ScopeBar constructor.
I'm not 100% against having Select Menus, but I wasn't in love with it.
Maybe there is a different paradigm entirely that would work better here.

[Saam Barati]

Created attachment 280896 [details]
WIP

[Saam Barati]

Comment on attachment 280896 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=280896&action=review

> Source/WebInspectorUI/UserInterface/Views/ScriptProfileTimelineView.js:35
>  {
> -    constructor(timeline, extraArguments)
> -    {
> -        super(timeline, extraArguments);
> -
> -        console.assert(timeline.type === WebInspector.TimelineRecord.Type.Script);
> +    const ProfileOrientation = {
> +        BottomUp: "bottom-up",
> +        TopDown: "top-down",
> +    };
> +
> +    const ProfileViewType = {
> +        Hierarchy: "hierarchy",
> +        TopFunctions: "top-functions",
> +    };

I tried something new with the Inspector's enum style for enums that are local to a file.
I figured it's both easier/concise to type this out and also safer because the enum is local to this file.
I just put the entire class inside a block scope. I figured JS now has block scope, and JS has always
had closures, so putting them together makes this pattern much nicer IMO.
What do you think?
(It makes the diff look crazy, unfortunately)

[Timothy Hatcher]

Comment on attachment 280896 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=280896&action=review

>> Source/WebInspectorUI/UserInterface/Views/ScriptProfileTimelineView.js:35
>> +    };
> 
> I tried something new with the Inspector's enum style for enums that are local to a file.
> I figured it's both easier/concise to type this out and also safer because the enum is local to this file.
> I just put the entire class inside a block scope. I figured JS now has block scope, and JS has always
> had closures, so putting them together makes this pattern much nicer IMO.
> What do you think?
> (It makes the diff look crazy, unfortunately)

I can see us doing this for cases where the enum is internal. This case is internal, but there is nothing preventing us from adding public methods or a setter/setter to change the modes. In that case it would need to be public again.

I think keeping them on the class like we have for other enums is best.

[Timothy Hatcher]

(In reply to comment #10)
> (In reply to comment #9)
> > A suggestion on UI, we should use select menus if we are adding another
> > picker on the side like this. Adding 4 total buttons like this takes up too
> > much space.
> I tried this with Joe, and I actually think it's worse. It takes more clicks
> to
> switch between views; it's less discoverable; and I think it discourages
> users
> from using the "non default" views. If you apply the patch locally you can
> easily
> flip it to that mode by switching the booleans in the ScopeBar constructor.
> I'm not 100% against having Select Menus, but I wasn't in love with it.
> Maybe there is a different paradigm entirely that would work better here.

Good point. Joe mentioned https to me too (the amount of clicking needed).

Another approach would be to make them just toggle buttons. [Bottom Up] | [Top Functions]. They are either on or off, with off being Top Down and Hierarchy, which is the 100% normal and expect call tree mode. Toggling on one of both of the other modes would make it abnormal, and highlight with a blue background why it is abnormal. I like the sound of that.

[Saam Barati]

You can view a recording here:
https://www.dropbox.com/s/fkiw0iow763h1fu/top-functions-view-recording.mov?dl=0

[Saam Barati]

Created attachment 281083 [details]
patch

[Saam Barati]

Comment on attachment 281083 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281083&action=review

> Source/WebInspectorUI/ChangeLog:15
> +        Top Functions works by treating every frame as a root in the tree.

Is this too much changelog?
I thought it might be useful to write what I wrote in bugzilla but maybe it's overkill.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:99
>  localizedStrings["Back (%s)"] = "Back (%s)";

Are all these changes correct?
I just updated the localized strings and got all this.
Maybe someone didn't update localized strings when deleting some UIStrings?

[Saam Barati]

Created attachment 281084 [details]
patch

Don't know why it didn't apply.

[WebKit Commit Bot]

Comment on attachment 281084 [details]
patch

Clearing flags on attachment: 281084

Committed r202010: <http://trac.webkit.org/changeset/202010>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

View in context: https://bugs.webkit.org/attachment.cgi?id=281084&action=review

> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:66
> +        } else if (this._type === WebInspector.CallingContextTree.Type.BottomUp) {

I'd prefer a switch statement for these. A little easier to read.

> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:385
> +    TopFunctionsTopDown: Symbol("TopFunctionsTopDown"),
> +    TopFunctionsBottomUp: Symbol("TopFunctionsTopDown"),

Wrong symbol name for TopFunctionsBottomUp.

[Saam Barati]

(In reply to comment #21)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281084&action=review
> 
> > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:66
> > +        } else if (this._type === WebInspector.CallingContextTree.Type.BottomUp) {
> 
> I'd prefer a switch statement for these. A little easier to read.
> 
> > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:385
> > +    TopFunctionsTopDown: Symbol("TopFunctionsTopDown"),
> > +    TopFunctionsBottomUp: Symbol("TopFunctionsTopDown"),
> 
> Wrong symbol name for TopFunctionsBottomUp.

Thanks. I forgot you commented on the switch. I'll land a follow up fix when I'm at a computer.

[Saam Barati]

Follow up in:
http://trac.webkit.org/changeset/202055