Closed Bug 1007460 Opened 7 years ago Closed 7 years ago

Create a calls minimap

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 6 obsolete files)

This minimap should be a representation of calls in some profile data. It will display the data as a layered bar graph (x axis: time, y axis: time spent doing certain things), with information like the amount of js/graphics/network/io/etc work performed on each sample (or some other resolution).

Note that this minimap shouldn't be a full blown flamechart (aka a "function calls pyramid"), but just a graph, similar to the framerate widget (bug 1007202).
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch v2 (obsolete) — Splinter Review
To test, grab the builds from here: [0] https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.com-af87c78fdaa2/ (tbpl link as well if you're interested: [1]) and use the addon from [2]. Open the "Framerate" tool and record.

[0] https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.com-af87c78fdaa2/
[1] https://tbpl.mozilla.org/?tree=Try&rev=af87c78fdaa2
[2] https://github.com/victorporof/FirefoxFramerateAddon
Attachment #8436276 - Attachment is obsolete: true
Attachment #8437797 - Flags: review?(pbrosset)
Comment on attachment 8437797 [details] [diff] [review]
v2

Review of attachment 8437797 [details] [diff] [review]:
-----------------------------------------------------------------

I've tested the graph with the provided build. It looks very nice.

A few things I discovered while playing with it (especially while resizing the window):
- the last bar is at times too wide, at times invisible. I guess the way ticks are grouped into bars doesn't play well with the last one. This is visible if you record a lot of ticks, wait for a few seconds before stopping the record.
- resizing the browser window after a big amount of ticks have been recorded is pretty laggy. I don't know if there is much to be done in this case, but right now, performance for big records aren't great. Granted, window resize isn't a key use case at all.
- again, when resizing the browser window, the Y scale changes, if your window is originally narrow, and you increase its size, then bars are going to be higher and higher, until eventually you can't see all of the blocks.

I'll include screenshots.

About the code, no major remarks, it looks beautiful.
I have just made a comment regarding the link between format and values, which I think isn't clear enough.

::: browser/devtools/shared/widgets/Graphs.jsm
@@ +1278,5 @@
> +    let ctx = canvas.getContext("2d");
> +    let width = canvas.width = this._width;
> +    let height = canvas.height = this._height;
> +
> +    let totalBlocks = this.format.length;

Here you're setting totalBlocks using this.format but by default this.format is an empty array. I would do one of 2 things:
1 - either use the first data tick's value array length instead, so that even if format isn't provided, something relevant will be drawn,
2 - or make format more obviously required: mentioning it in the example section of the jsdoc at the top and possibly throwing or outputting a warning if the value length found in the data doesn't match the format length.

I realize that this might actually be considered a feature: providing fewer format entries than there are values per tick, to only draw these ones, but as a feature it's not complete because you can't really choose which of the values you want to draw. So if it's not really required by the perf tools that will be built with this, I would choose solution 1.
Attachment #8437797 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Comment on attachment 8437797 [details] [diff] [review]
> v2
> 
> Review of attachment 8437797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've tested the graph with the provided build. It looks very nice.
> 

Thanks for being so thorough Patrick!

> A few things I discovered while playing with it (especially while resizing
> the window):
> - the last bar is at times too wide, at times invisible. I guess the way
> ticks are grouped into bars doesn't play well with the last one. This is
> visible if you record a lot of ticks, wait for a few seconds before stopping
> the record.

This is the addon's fault, trying to be too smart, not the graph's. Since the framerate and profiler data may not have the *exact* same start and end time intervals, because they're different backends, I'm trying to sync them up by "stretching" time for the first or last tick [0]. Granted, this ends up looking pretty weird in the graph, so maybe clamping is a better solution. Anyway, I'll fix this in the add-on.

> - resizing the browser window after a big amount of ticks have been recorded
> is pretty laggy. I don't know if there is much to be done in this case, but
> right now, performance for big records aren't great. Granted, window resize
> isn't a key use case at all.

Not sure if we can do much about it, at the moment. The data generated for the calls minimap graph has an obscene number of entries, on the order of hundreds of thousands for a few minutes, millions > 10 minutes. Iterating over it naturally takes a lot of time. There's nothing that can really be cached between resizes, since the drawing should differ every time. A few possible ways out:

1. switch to OMT worker canvas rendering, bug 709490
  - however, the transfer cost of the profiler data between the main thread and the worker is huge.
  - can't efficiently use transferable objects with no-copy because the source data isn't an array buffer

2. never resize horizontally when newSize < oldSize
  - scrollbars are ugly and inconvenient
  - every other bit of the profiler UI will need to accommodate this

3. increase the timeout between resizes
  - not really a fix

Bug 709490 seems like a good place to start experimenting, but that should probably be a followup.

> - again, when resizing the browser window, the Y scale changes, if your
> window is originally narrow, and you increase its size, then bars are going
> to be higher and higher, until eventually you can't see all of the blocks.
> 

I realized that we're calculating the Y-axis scaling in a completely incorrect way. We're scaling based on the largest sum of all blocks in a *tick*, but we're drawing averaged out ticks in *bars*, none of which will have the same max height as before. I'll fix this in this patch.

> I'll include screenshots.
> 
> About the code, no major remarks, it looks beautiful.
> I have just made a comment regarding the link between format and values,
> which I think isn't clear enough.
> 

Fair enough.

> ::: browser/devtools/shared/widgets/Graphs.jsm
> @@ +1278,5 @@
> > +    let ctx = canvas.getContext("2d");
> > +    let width = canvas.width = this._width;
> > +    let height = canvas.height = this._height;
> > +
> > +    let totalBlocks = this.format.length;
> 
> Here you're setting totalBlocks using this.format but by default this.format
> is an empty array. I would do one of 2 things:
> 1 - either use the first data tick's value array length instead, so that
> even if format isn't provided, something relevant will be drawn,

Actually, this is how I initially implemented this, but there were two problems, namely:

1. The first blocks array had to be of a specific size, because all those values arrays are bound to have holes if we can have any chance of being efficient.

2. Initializing arrays of specific size was inconventient and looked weird to special case. Since
> new Array(n).length === 0
that didn't work, we had to
> Array.apply(null, new Array(n)).map(e => 0)
to actually get what we want. This can be slow, so using a separate `format` trait for the data source seemed like a better idea.

> 2 - or make format more obviously required: mentioning it in the example
> section of the jsdoc at the top and possibly throwing or outputting a
> warning if the value length found in the data doesn't match the format
> length.
> 
> I realize that this might actually be considered a feature: providing fewer
> format entries than there are values per tick, to only draw these ones, but
> as a feature it's not complete because you can't really choose which of the
> values you want to draw. So if it's not really required by the perf tools
> that will be built with this, I would choose solution 1.

It *is* a feature. You can skip drawing values by providing arrays with holes, in fact, that's exactly how we're aggregating profiler data into categories before sending this to the graph to render [1]
let values = [];
values[2] = n;
values[5] = m;
This will only draw the second and the fifth block.

[0] https://github.com/victorporof/FirefoxFramerateAddon/blob/master/chrome/tool.js#L125
[1] https://github.com/victorporof/FirefoxFramerateAddon/blob/master/chrome/tool.js#L170
(In reply to Victor Porof [:vporof][:vp] from comment #9)
> 
> > 2 - or make format more obviously required: mentioning it in the example
> > section of the jsdoc at the top and possibly throwing or outputting a
> > warning if the value length found in the data doesn't match the format
> > length.
> > 

I'll make the format array mandatory and mention it in the constructor.
Attached patch v3Splinter Review
Addressed comments, made the `format` array mandatory, fixed Y scaling and a couple of failing tests.
Attachment #8436109 - Attachment is obsolete: true
Attachment #8436110 - Attachment is obsolete: true
Attachment #8437797 - Attachment is obsolete: true
Attachment #8439754 - Attachment is obsolete: true
Attachment #8439755 - Attachment is obsolete: true
Attachment #8439884 - Flags: review?(pbrosset)
(In reply to Victor Porof [:vporof][:vp] from comment #11)

Oh, and I also optimized the rendering a bit to stop drawing strokes; they can be easily faked by spacing out the filled blocks a few pixels apart, hopefully this makes rendering a bit faster.
(In reply to Victor Porof [:vporof][:vp] from comment #12)
> (In reply to Victor Porof [:vporof][:vp] from comment #11)
> 
> Oh, and I also optimized the rendering a bit to stop drawing strokes; they
> can be easily faked by spacing out the filled blocks a few pixels apart,
> hopefully this makes rendering a bit faster.
Thanks, it does seem faster. And yeah, I agree for working on any major performance-related refactor in follow-up bugs. The graph is nice like this and the window resize use case isn't the most important one now.

(In reply to Victor Porof [:vporof][:vp] from comment #9)
> 3. increase the timeout between resizes
>   - not really a fix
Actually, I think it's worth a try, it's not like users will need to actually look at the graph while resizing, so increasing the timeout a lot would make resizing a lot faster because then the graph wouldn't re-generate during the resize at all, and would only appear crisp again when the user let go of the resize handle.

(In reply to Victor Porof [:vporof][:vp] from comment #11)
> Created attachment 8439884 [details] [diff] [review]
> v3
> 
> Addressed comments, made the `format` array mandatory, fixed Y scaling and a
> couple of failing tests.
Thanks, I see all comments addressed. The `format` changes look good to me. And yeah, the Y scaling does work nice now.
Attachment #8439884 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/473a010048de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.