Bug 111162 - Web Inspector: implement Flame Chart for CPU profiler
: Web Inspector: implement Flame Chart for CPU profiler
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Ilya Tikhonovsky
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-01 04:33 PST by Ilya Tikhonovsky
Modified: 2013-03-04 04:41 PST (History)
8 users (show)

See Also:


Attachments
screenshot (17.16 KB, image/png)
2013-03-01 04:35 PST, Ilya Tikhonovsky
no flags Details
Patch (11.19 KB, patch)
2013-03-01 04:37 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (11.16 KB, patch)
2013-03-01 04:49 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (17.44 KB, patch)
2013-03-04 04:00 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (17.00 KB, patch)
2013-03-04 04:04 PST, Ilya Tikhonovsky
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2013-03-01 04:33:26 PST
Flame Chart may give to the developer a better clue what is going on with the performance without expanding the entire tree.

http://dtrace.org/blogs/brendan/2011/12/16/flame-graphs/
Comment 1 Ilya Tikhonovsky 2013-03-01 04:35:00 PST
Created attachment 190933 [details]
screenshot
Comment 2 Ilya Tikhonovsky 2013-03-01 04:37:34 PST
Created attachment 190934 [details]
Patch
Comment 3 Ilya Tikhonovsky 2013-03-01 04:49:57 PST
Created attachment 190938 [details]
Patch
Comment 4 Yury Semikhatsky 2013-03-03 22:56:17 PST
Comment on attachment 190938 [details]
Patch

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

> Source/WebCore/inspector/front-end/CPUProfileView.js:65
> +    } else {

style: no braces for one-line block.

> Source/WebCore/inspector/front-end/CPUProfileView.js:101
> +WebInspector.FlameChart = function(cpuProfileView)

Why not move it into its own file?

> Source/WebCore/inspector/front-end/CPUProfileView.js:107
> +    this._canvas = document.createElement("canvas");

this._canvas = this.element.createChild("canvas");

> Source/WebCore/inspector/front-end/CPUProfileView.js:183
> +    draw: function(height, width)

It is usually width, heigh (in reverse order).

> Source/WebCore/inspector/front-end/CPUProfileView.js:194
> +        this._color = initialColor;

Consider using WebInspector.Color

> Source/WebCore/inspector/front-end/CPUProfileView.js:211
> +    _forEach: function(callback)

Please rename _forEach -> _forEachNode

> Source/WebCore/inspector/front-end/CPUProfileView.js:227
> +                    nodes = nodes.concat(node.children);

This way the nodes will appear in reverse order, won't they?

> Source/WebCore/inspector/front-end/CPUProfileView.js:241
> +     * @param {!ProfilerAgent.CPUProfileNode} node

color annotation is missing, also I'd rename it to rgbColor

> Source/WebCore/inspector/front-end/CPUProfileView.js:900
> +

Revert this?

> Source/WebCore/inspector/front-end/CPUProfileView.js:934
> +//@ sourceURL=http://localhost/inspector/front-end/CPUProfileView.js

Remove this.
Comment 5 Ilya Tikhonovsky 2013-03-04 04:00:22 PST
Created attachment 191199 [details]
Patch
Comment 6 Ilya Tikhonovsky 2013-03-04 04:02:57 PST
Comment on attachment 190938 [details]
Patch

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

>> Source/WebCore/inspector/front-end/CPUProfileView.js:194
>> +        this._color = initialColor;
> 
> Consider using WebInspector.Color

done

>> Source/WebCore/inspector/front-end/CPUProfileView.js:211
>> +    _forEach: function(callback)
> 
> Please rename _forEach -> _forEachNode

done

>> Source/WebCore/inspector/front-end/CPUProfileView.js:227
>> +                    nodes = nodes.concat(node.children);
> 
> This way the nodes will appear in reverse order, won't they?

It is not a problem because children have no order.

>> Source/WebCore/inspector/front-end/CPUProfileView.js:241
>> +     * @param {!ProfilerAgent.CPUProfileNode} node
> 
> color annotation is missing, also I'd rename it to rgbColor

done: hslColor

>> Source/WebCore/inspector/front-end/CPUProfileView.js:900
>> +
> 
> Revert this?

done

>> Source/WebCore/inspector/front-end/CPUProfileView.js:934
>> +//@ sourceURL=http://localhost/inspector/front-end/CPUProfileView.js
> 
> Remove this.

done
Comment 7 Ilya Tikhonovsky 2013-03-04 04:04:40 PST
Created attachment 191200 [details]
Patch
Comment 8 Ilya Tikhonovsky 2013-03-04 04:41:57 PST
Committed r144618: <http://trac.webkit.org/changeset/144618>