Closed
Bug 1077444
Opened 10 years ago
Closed 10 years ago
Create Flamechart Widget
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 37
People
(Reporter: jsantell, Assigned: vporof)
References
Details
Attachments
(1 file)
(deleted),
patch
|
jsantell
:
review+
BenWa
:
review+
|
Details | Diff | Splinter Review |
For flame charts in new performance tool
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → vporof
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8544016 [details] [diff] [review]
v1
Review of attachment 8544016 [details] [diff] [review]:
-----------------------------------------------------------------
some nits and other enhancements, r+
::: browser/devtools/shared/test/browser_flame-graph-01.js
@@ +7,5 @@
> +let {DOMHelpers} = Cu.import("resource:///modules/devtools/DOMHelpers.jsm", {});
> +let {Promise} = devtools.require("resource://gre/modules/Promise.jsm");
> +let {Hosts} = devtools.require("devtools/framework/toolbox-hosts");
> +
> +let test = Task.async(function*() {
We should probably use the new `add_task(function*(){})` function for doing tests to be in sync with all the other DT tests recently converted
::: browser/devtools/shared/test/browser_flame-graph-03.js
@@ +93,5 @@
> + y /= window.devicePixelRatio;
> + graph._onMouseMove({ clientX: x, clientY: y });
> +}
> +
> +function dragStart(graph, x, y = 1) {
dragStart and dragStop are defined in a handful of these graph tests -- maybe good time to pull these out into head.js, or atleast a bug for it
::: browser/devtools/shared/widgets/FlameGraph.jsm
@@ +28,5 @@
> +const TIMELINE_TICKS_SPACING_MIN = 75; // px
> +
> +const OVERVIEW_HEADER_HEIGHT = 18; // px
> +const OVERVIEW_HEADER_SAFE_BOUNDS = 50; // px
> +const OVERVIEW_HEADER_TEXT_COLOR = "#18191a";
Colors should be defined on the prototype by default, not directly called via `ctx.fillStyle()` or something, so we can easily add the ability to change these colors, like the other themeable graphs. Like, in the prototype:
`headerTextColor: OVERVIEW_HEADER_TEXT_COLOR`
Then just called via `this.headerTextColor` in the actual drawing.
::: browser/devtools/shared/widgets/Graphs.jsm
@@ +12,3 @@
>
> this.EXPORTED_SYMBOLS = [
> + "GraphCursor",
is GraphCursor or GraphSelectionResizer used anywhere?
Reporter | ||
Updated•10 years ago
|
Attachment #8544016 -
Flags: review?(jsantell) → review+
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> Comment on attachment 8544016 [details] [diff] [review]
> v1
>
> ::: browser/devtools/shared/test/browser_flame-graph-03.js
> @@ +93,5 @@
> > + y /= window.devicePixelRatio;
> > + graph._onMouseMove({ clientX: x, clientY: y });
> > +}
> > +
> > +function dragStart(graph, x, y = 1) {
>
> dragStart and dragStop are defined in a handful of these graph tests --
> maybe good time to pull these out into head.js, or atleast a bug for it
>
I'm a bit wary of moving them into head.js, since this is a shared devtools test folder, and those functions are only useful for a handful of tests.
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8544016 [details] [diff] [review]
v1
BenWa said he might want to take a look at this too.
Attachment #8544016 -
Flags: review?(bgirard)
Comment 5•10 years ago
|
||
Comment on attachment 8544016 [details] [diff] [review]
v1
Review of attachment 8544016 [details] [diff] [review]:
-----------------------------------------------------------------
Looks excellent. Great work!
::: browser/devtools/shared/widgets/FlameGraph.jsm
@@ +333,5 @@
> + let ctx = this._ctx;
> +
> + let fontSize = FLAME_GRAPH_BLOCK_TEXT_FONT_SIZE * this._pixelRatio;
> + let fontFamily = FLAME_GRAPH_BLOCK_TEXT_FONT_FAMILY;
> + let visibleBlocks = this._drawPyramidFill(dataSource, dataOffset, dataScale);
In theory it should be better performance if you fill+draw-text together since you'll get better data/cache locality. Not sure if it's worth making the change in practice.
Actually looking further in your patch if you make this change you'll need to do more canvas state change (i.e. change styles). I'm not sure which you want to optimize, perhaps the state change you minimize buys you more than cache locality does.
Attachment #8544016 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Updated•10 years ago
|
Whiteboard: [fixed-in-fx-team]
Updated•10 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 8•10 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 9•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 37
Comment 10•10 years ago
|
||
Nice flame chart!
The code refers to "flame graph". While the differentiation between "flame chart" and "flame graph" may be accidental, it is useful to have two different terms for two different visualizations. FWIW, a flame graph is similar, but maximizes grouping frames by sorting stacks alphabetically on the x-axis. Ie, the x-axis is no longer time. (see http://www.slideshare.net/brendangregg/blazing-performance-with-flame-graphs/35). Since the data and infrastructure is here, it would be great (if it doesn't already), to have a toggle button to switch between flame chart (time-ordered) and flame graph (alphabet-sorted). I could file a separate ticket...
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Brendan Gregg from comment #10)
> Since the data and infrastructure is here, it would be great (if it
> doesn't already), to have a toggle button to switch between flame chart
> (time-ordered) and flame graph (alphabet-sorted). I could file a separate
> ticket...
That's a good idea. A few bugs were fixed in the meantime to reduce quite a bit of noise from the visualization (e.g. bug 1119932, bug 1119933), but having an "alphabetize" toggle could prove to be a secondary interesting visualization of the call tree.
It's probably a good first bug as well (i.e.: easy to implement).
Feel free to file the bug, and even hack around if you want to. I can mentor if needed.
Comment 12•10 years ago
|
||
(In reply to Victor Porof [:vporof][:vp] from comment #11)
> (In reply to Brendan Gregg from comment #10)
> > Since the data and infrastructure is here, it would be great (if it
> > doesn't already), to have a toggle button to switch between flame chart
> > (time-ordered) and flame graph (alphabet-sorted). I could file a separate
> > ticket...
>
> That's a good idea. A few bugs were fixed in the meantime to reduce quite a
> bit of noise from the visualization (e.g. bug 1119932, bug 1119933), but
> having an "alphabetize" toggle could prove to be a secondary interesting
> visualization of the call tree.
>
> It's probably a good first bug as well (i.e.: easy to implement).
>
> Feel free to file the bug, and even hack around if you want to. I can mentor
> if needed.
Ok, filed 1123495. Thanks for the offer, and I'd like to hack Mozilla one day, but probably won't have time for a while. :)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•