Bug 163463 – Web Inspector: Heatmap Profiler: Highlight function definitions

• Component: Web Inspector
• Status: NEW

[Nikita Vasilyev]

Created attachment 291673 [details]
[Image] WIP

As a first minimum viable product, highlight only function definitions (function expressions, class methods, arrow functions, etc.) and nothing inside the function or expressions in the global scope.

[Radar WebKit Bug Importer]

<rdar://problem/28783046>

[Nikita Vasilyev]

Created attachment 291722 [details]
Patch

Reduction: http://nv.github.io/webkit-inspector-bugs/163463_heatmap/
With this WIP patch applied, you should see the same as on the attached screenshot.

[Nikita Vasilyev]

Created attachment 291837 [details]
[Image] With patch applied

[Nikita Vasilyev]

Created attachment 291838 [details]
[Patch] WIP

[Nikita Vasilyev]

(In reply to comment #4)
> Created attachment 291838 [details]
> [Patch] WIP

Things to fix:

1. Arrow functions aren't highlighted.

2. For:
    
    blah()
    {
        ...
    }

The duration token is placed on the line below the method name:

    blah()
    [6.31ms] {
        ...
    }

[Nikita Vasilyev]

Created attachment 291884 [details]
Patch

[Nikita Vasilyev]

(In reply to comment #5)
> (In reply to comment #4)
> > Created attachment 291838 [details]
> > [Patch] WIP
> 
> Things to fix:
> 
> 1. Arrow functions aren't highlighted.
> 
> 2. For:
>     
>     blah()
>     {
>         ...
>     }
> 
> The duration token is placed on the line below the method name:
> 
>     blah()
>     [6.31ms] {
>         ...
>     }

Neither of these addressed in the patch above. I'm going to fix it in the follow up patch.

[Devin Rousso]

Comment on attachment 291884 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:49
> +            // FIXME: Use "for-of" loop once it's as fast as a "for" loop.

Really?  Where (and how) did you test this?

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:55
> +                if (visited.has(node))

NIT: you could merge this with the previous `if`.

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:82
> +

NIT: extra space.

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:113
> +                css: "background: " + cssColorValue,

NIT: template string?

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:132
> +        const NodeType = WebInspector.ScriptSyntaxTree.NodeType;

NIT: I would say that if you plan on making this "alias", it should be placed at the top of the function (above all logic).

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:134
> +        let rangeForFunction = (token) => {

NIT: this could just be a regular function, since the context isn't needed.

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:139
> +        // function foo() {}

NIT: I think you should clarify this comment by saying "this is an example".

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:143
> +        // class AppClass() {constructor() {}}

Putting comments in between "if...else" seems very awkward.

> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:129
> +        let visited = new Set;

It seems like you aren't ever adding to `visited`.  Wouldn't it be easier to just use `visited` and `Array.from`?

let visited = new Set;
this.forEachNode((node) => visited.add(node)});
return Array.from(visited);

> Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:71
> +        let i = this._durations.length;

Is there a reason why you need to start at the end and iterate backwards?  Could you just use `reduce`?

return this._durations.reduce((previousValue, currentValue) => previousValue + currentValue);

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:54
> +    findTokenAtOffset(offset)

This function seems very complicated and could use some more in-depth comments.  Also, I feel like it may be cleaner to make local functions for each node type to let you use early returns.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:59
> +        for (let i = 0; i < body.length; i++) {

NIT: `for...of` instead?

[Timothy Hatcher]

Comment on attachment 291884 [details]
Patch

Based on feedback from the contributors meeting, we should highlight all ranges, not just function calls.

[Nikita Vasilyev]

(In reply to comment #9)
> Comment on attachment 291884 [details]
> Patch
> 
> Based on feedback from the contributors meeting, we should highlight all
> ranges, not just function calls.

Yes, we should highlight all ranges. The patch above is a stepping stone to this.

I'll post an updated patch later today.

[Nikita Vasilyev]

(In reply to comment #8)
> Comment on attachment 291884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291884&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:49
> > +            // FIXME: Use "for-of" loop once it's as fast as a "for" loop.
> 
> Really?  Where (and how) did you test this?

A "for" loop is >20 times faster that a "for-of" loop.
https://jsperf.com/for-of-vs-for-loops

Generally, operations inside the loop are performance sensitive, not the loop code itself.
However, we use "for" loops in code that runs very often, such as WebInspector.Object#dispatchEventToListeners.

>
> > Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:71
> > +        let i = this._durations.length;
> 
> Is there a reason why you need to start at the end and iterate backwards? 
> Could you just use `reduce`?

`reduce` is slower. Again, this is a micro optimization which is only noticeable in a "hot" code.

[Nikita Vasilyev]

(In reply to comment #8)
> Comment on attachment 291884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291884&action=review
>
> > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:143
> > +        // class AppClass() {constructor() {}}
> 
> Putting comments in between "if...else" seems very awkward.

Any suggestions on where to put them?

> 
> > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:129
> > +        let visited = new Set;
> 
> It seems like you aren't ever adding to `visited`.  Wouldn't it be easier to
> just use `visited` and `Array.from`?
> 
> let visited = new Set;
> this.forEachNode((node) => visited.add(node)});
> return Array.from(visited);

Array.from(aSet) is at least 4x slower.
https://jsperf.com/array-from-set-vs-array-push/1

[Nikita Vasilyev]

Created attachment 294258 [details]
Patch

[Timothy Hatcher]

Comment on attachment 294258 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:43
> +            // FIXME: >10k nodes can be slow to process.
> +            let nodes = WebInspector.timelineManager.activeRecording.topFunctionsBottomUpCallingContextTree.toFlatList();

This does seem like it would be very expensive. Why does it need flattened first? Flattening it seems like it would make it harder to fi the other FIXME in this patch that only annotates the viewport lines.

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:75
> +    clearAnnotations()
> +    {
> +        // No-op
> +    }

Why a no-op?

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:82
> +

Nit: Extra line.

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:83
> +            if (node.name === "(program)")

Is there not a node type for this instead of checking for the name?

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:87
> +            let token = syntaxTree.findTokenAtOffset(offset);

This is very expensive! _highlightNodes is called in a loop, and findTokenAtOffset does two expensive walks on the nodes.

> Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:126
> +    _createDurationWidget(totalDuration)
> +    {
> +        let widgetElement = document.createElement("span");
> +        widgetElement.classList.add("duration-token");
> +        widgetElement.textContent = Number.secondsToString(totalDuration);
> +        return widgetElement;
> +    }

I don't think the durations widget holds up well once we start highlighting all ranges, not just function calls. It would be too noisy. What other design should be considered for this info?

> Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:133
> +                console.warn("Visited:", node);

What is this warning about?

> Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:43
> +        // FIXME: Store leaf node objects in an array.

Why?

> Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:74
> +

Nit: Can remove this blank line.

> Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:67
> +        let containers = this.containersOfOffset(offset);
> +        let index = containers.length;
> +        let container;
> +
> +        while (index--) {
> +            container = containers[index];
> +            if (supportedContainerTypes.has(container.type))
> +                break;
> +        }

This is an expensive operation. The containersOfOffset function already walks all the nodes. Then you walk them a second time. Perhaps containersOfOffset should have some options to support what you need during a single node walk?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:359
> +    markText(startPosition, endPosition, options)
> +    {
> +        this._codeMirror.getDoc().markText(startPosition, endPosition, options);
> +    }

_codeMirror is private to WebInspector.TextEditor, and should not be used here. This markText fiction should move to TextEditor.js.

This also leaks the underlying details of CodeMirror to the callers of this by just blindly passing options through. If we want to do this, then we would want an object or API for our own options that we could map to CodeMirror or other text editor in the future.

[Nikita Vasilyev]

Created attachment 294562 [details]
[Image] Heatmap of HeatmapAnnotator

I profiled HeatmapAnnotator.js using Heatmap Profiler (how cool is that?!).

Some things you can see:
1. toFlatList is very fast relative to _highlightNodes (123 times faster).
2. _highlightNodes is primarily slow because of setInlineWidget.
3. findTokenAtOffset fairly fast, 14 times faster than setInlineWidget.

This is setInlineWidget:

    setInlineWidget(position, inlineElement)
    {
        return this._codeMirror.setUniqueBookmark(position, {widget: inlineElement});
    }

I don't think I can make setUniqueBookmark faster, but if I only render widgets that are in the viewport that shouldn't be needed.

Please note, to take this screenshot I used a version of Heatmap Profiler that highlights all expressions, not just function definitions. It's different from the one in the attached patch but HeatmapAnnotator class is almost the same.

[Nikita Vasilyev]

(In reply to comment #14)
> > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:126
> > +    _createDurationWidget(totalDuration)
> > +    {
> > +        let widgetElement = document.createElement("span");
> > +        widgetElement.classList.add("duration-token");
> > +        widgetElement.textContent = Number.secondsToString(totalDuration);
> > +        return widgetElement;
> > +    }
> 
> I don't think the durations widget holds up well once we start highlighting
> all ranges, not just function calls. It would be too noisy. What other
> design should be considered for this info?

1. Only show duration widgets for function definitions.
2. Only show duration widgets for 20% slowest nodes. (I like this one!)

[Nikita Vasilyev]

(In reply to comment #14)
> Comment on attachment 294258 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294258&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:43
> > +            // FIXME: >10k nodes can be slow to process.
> > +            let nodes = WebInspector.timelineManager.activeRecording.topFunctionsBottomUpCallingContextTree.toFlatList();
> 
> This does seem like it would be very expensive. Why does it need flattened
> first? Flattening it seems like it would make it harder to fi the other
> FIXME in this patch that only annotates the viewport lines.

I should remove this FIXME, it actually not very expensive as noted above in comment #c15.

> 
> > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:75
> > +    clearAnnotations()
> > +    {
> > +        // No-op
> > +    }
> 
> Why a no-op?

I'll remove it.

> 
> > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:82
> > +
> 
> Nit: Extra line.
> 
> > Source/WebInspectorUI/UserInterface/Controllers/HeatmapAnnotator.js:83
> > +            if (node.name === "(program)")
> 
> Is there not a node type for this instead of checking for the name?

There's currently nothing like node.type. It does look a bit dirty. Maybe we can introduce "isProgram" getter.

> > Source/WebInspectorUI/UserInterface/Models/CallingContextTree.js:133
> > +                console.warn("Visited:", node);
> 
> What is this warning about?

This should be removed. I was making sure there are not duplicate nodes.

> 
> > Source/WebInspectorUI/UserInterface/Models/CallingContextTreeNode.js:43
> > +        // FIXME: Store leaf node objects in an array.
> 
> Why?

To highlight leaf nodes (all expressions).

> 
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:67
> > +        let containers = this.containersOfOffset(offset);
> > +        let index = containers.length;
> > +        let container;
> > +
> > +        while (index--) {
> > +            container = containers[index];
> > +            if (supportedContainerTypes.has(container.type))
> > +                break;
> > +        }
> 
> This is an expensive operation. The containersOfOffset function already
> walks all the nodes. Then you walk them a second time. Perhaps
> containersOfOffset should have some options to support what you need during
> a single node walk?

It isn't the most expensive operation in my patch, as I showed above, yet 
I agree this can be optimized to do only a single node walk.

> 
> > Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:359
> > +    markText(startPosition, endPosition, options)
> > +    {
> > +        this._codeMirror.getDoc().markText(startPosition, endPosition, options);
> > +    }
> 
> _codeMirror is private to WebInspector.TextEditor, and should not be used
> here. This markText fiction should move to TextEditor.js.
> 
> This also leaks the underlying details of CodeMirror to the callers of this
> by just blindly passing options through. If we want to do this, then we
> would want an object or API for our own options that we could map to
> CodeMirror or other text editor in the future.

You're right, I was a bit sloppy here.

[Nikita Vasilyev]

(In reply to comment #14)
> > Source/WebInspectorUI/UserInterface/Models/ScriptSyntaxTree.js:67
> > +        let containers = this.containersOfOffset(offset);
> > +        let index = containers.length;
> > +        let container;
> > +
> > +        while (index--) {
> > +            container = containers[index];
> > +            if (supportedContainerTypes.has(container.type))
> > +                break;
> > +        }
> 
> This is an expensive operation. The containersOfOffset function already
> walks all the nodes. Then you walk them a second time. Perhaps
> containersOfOffset should have some options to support what you need during
> a single node walk?

I walk nodes back to find the innermost node of a certain type. I can't exit early on the
first occurrence of a supported type, I have to backtrack. Consider the following example:

function foo() {
    function bar|() {return 42}
    return bar()
}

(| is the offset position)

containersOfOffset sees foo's FunctionDeclaration before bar's FunctionDeclaration.
If I exit early on the first occurrence of FunctionDeclaration, I'd get foo() and not bar().

[Nikita Vasilyev]

Created attachment 297486 [details]
[Image] Time Duration Tokens

Redesigned time duration tokens. Inspired by cssarrowplease.com, these are implemented in pure CSS (no images or SVGs). Let me know what you think!

[Nikita Vasilyev]

(In reply to comment #19)
> Created attachment 297486 [details]
> [Image] Time Duration Tokens
> 
> Redesigned time duration tokens. Inspired by cssarrowplease.com, these are
> implemented in pure CSS (no images or SVGs). Let me know what you think!

Same image: https://cloudup.com/cNNMQCHJHrC (Cloudup, unlike Bugzilla, detects retina images and displays them properly scaled).

[Matt Baker]

(In reply to comment #19)
> Created attachment 297486 [details]
> [Image] Time Duration Tokens
> 
> Redesigned time duration tokens. Inspired by cssarrowplease.com, these are
> implemented in pure CSS (no images or SVGs). Let me know what you think!

Think these look really nice! It looks like the left-arrow border color is slightly different from the rounded border used by the rest of the token.

[Nikita Vasilyev]

(In reply to comment #21)
> (In reply to comment #19)
> > Created attachment 297486 [details]
> > [Image] Time Duration Tokens
> > 
> > Redesigned time duration tokens. Inspired by cssarrowplease.com, these are
> > implemented in pure CSS (no images or SVGs). Let me know what you think!
> 
> Think these look really nice! It looks like the left-arrow border color is
> slightly different from the rounded border used by the rest of the token.

That's intentional. I may look strange on Bugzilla but it looks better on non-zoomed screenshot (https://cloudup.com/cNNMQCHJHrC). I may still make the border and arrow colors more similar.