975675 - Add widgets to the developer toolbar for monitoring memory, jank and reflows

Status: RESOLVED WONTFIX

(DevTools :: General, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

The developer toolbar can serve as a fine host for small, non-intrusive widgets that monitor the performance characteristics of a web app. Unlike the toolbox, it can be left open without obscuring a significant part of the content window, providing insights beyond those of a short debugging session. The devtools widgets in Firefox OS offer a similar proposition, but since desktop screens do not impose such severe size constraints, we can be more feature-rich and useful as a result.

We will be starting with three widgets that monitor memory consumption, jank and reflows, but more should follow.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Add widgets to the developer toolbar for monitoring memory, jank and reflows

This patch works well, but I still need to refactor the code a little bit (more DRY, less copy & paste) and add some tests. I also intend to introduce a target abstraction just like the one in the toolbox, so that we will be able to target the toolbar to a remote device in the future.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Screenshot

Looks like this right now.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v2

Rewrote everything twice this week and now things are in much better shape:

- all widgets have realtime-updated tooltips with detailed stats
- simplified the tooltip text to make it easier to parse at a glance
- added some padding between widgets to separate them visually
- widgets extend a base GraphWidget class instead of being separate implementations
- converted widgets from JSM to SDK modules
- serialized creation and destruction of all widgets in the DeveloperToolbar.jsm
- used Promise.jsm instead of SDK promises

Still need to do a few more cleanups and write tests.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v3

More CSS sharing to simplify extending the base widget and more comments to explain what implementors will likely want to override.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v4

Used the DeveloperToolbar's shared Target object for handling the widgets' remote connections. There is one more bug I need to fix (related to toggling widgets) before asking for feedback.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v5

Turns out that gDevTools registers a pref-change observer that always calls DeveloperToolbar.show no matter what. This is probably something that we should reconsider, but in the meantime I arranged for my widgets to expect that. There are a few more bugs that I'm looking into, but I'd like to get some feedback on the overall approach and functionality before starting to write tests for all this.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Funny bug: closing the toolbar now closes any open toolbox, because target.js assumes too much. Also, the EventLoopLag actor doesn't get properly destroyed. I think these are the last two bugs that I need to fix.

Comment 8

[Rob Campbell [:rc] (:robcee)]

this needs a slight rebasin' in toolkit/devtools/server/actors/webbrowser.js

Comment 9

[Rob Campbell [:rc] (:robcee)]

(In reply to Panos Astithas [:past] from comment #7)
> Funny bug: closing the toolbar now closes any open toolbox, because
> target.js assumes too much. Also, the EventLoopLag actor doesn't get
> properly destroyed. I think these are the last two bugs that I need to fix.

presumably I can hold off on review for an updated patch?

Playing around with this, I'm not seeing any way to activate the widgets. Haven't dug into the code to look for a button, but would appreciate a user guide.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Sorry, I only noted that in the previous bug: the widgets can be toggled on/off from the options panel.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v6

Fixed EventLoopLag actor shutdown. Fixing target.js needs more thought, because there are a few interesting options for reuse and handoff (between the toolbar and the toolbox) that I need to think through first.

What I'm mostly looking for now is feedback on the feature set, not so much on the code. That's because writing tests will be pointless if we're going to do something significantly different.

You make a valid point about a user guide. Right now the available interactions with the widgets are toggling (via the options panel) and hovering with the mouse pointer, which presents more detailed readings. That's about it. In the future we will definitely support clicking on the widget that will take you to the full-featured tool and perhaps context-clicking for modifying the behavior of the widget (e.g. toggle some individual measurements from the memory widget).

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8389990 [details] [diff] [review]
v6

Oops.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Oh, one other feature that I neglected to mention is the presence of prefs for using the widgets for browser debugging. Specifically, setting devtools.{memory,reflow}.monitor.browser (from about:config) will let a browser or add-on developer use these widgets for monitoring the entire browser process. The jank widget is targeting the entire browser by definition.

I'd especially like feedback about this, as I feel it's something of real value to a lot of people, but I haven't thought of a good way to expose that in the UI yet. I think adding the toolbar to the browser toolbox would make the most sense, but we haven't really discussed that either.

Comment 14

[Rob Campbell [:rc] (:robcee)]

ok, I'll rebuild with the newer patch and play around. Thanks for the helpful notes.

Comment 15

[Rob Campbell [:rc] (:robcee)]

These seem to be working well, though the jank meter can be confusing with a lot of tabs open. I had one lockup yesterday while running these on a webgl page with the canvas inspector open (which might have been the actual culprit). I need to do some more investigation.

Another big question is what do we do about remote targets? The developer toolbar doesn't make a lot of sense for those as a host. I don't have a good answer for that right now but am open to suggestions.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Here are my thoughts on remote targets. I think including the toolbar in the bottom of the App Manager's toolbox would work. It's a straightforward step from our current UX for that audience without any surprises.

More generally, I think treating the toolbar as an extension of the toolbox would also work. By that I mean including the toolbar markup inside toolbox.xul (instead of browser.xul) and providing 3 states for devtools: large toolbox, mini toolbar, both.

I imagine switching between these 3 states to happen via keybindings (F12, Shift-F2), UI buttons (like the wrench in the toolbar) and even a checkbox in the options panel, although the latter could be traded for a button in the "command buttons" area of the toolbox toolbar. State is preserved just like the toolbox location state is.

This would naturally let the toolbar appear in the browser toolbox targeting chrome code, which would make the chrome prefs in this patch redundant.

The combination would be even more natural in my view when we deliver the full-fledged performance and memory tools that would also open from clicks on the toolbar widgets.

Comment 17

[Rob Campbell [:rc] (:robcee)]

I think your suggestion to include this as part of the toolbox is a good one. It allows us to do the dynamic theming trick and it gives us a more unified target resolution.

One of the funny quirks with this patch right now is that if you open the toolbox and close it with the developer toolbar opened, the toolbar closes with the toolbox. Surprising. :)

I like the tools overall, but wonder if we shouldn't try to combine jank + reflows somehow. Having the two of them feels wasteful especially when they're so similar. Though maybe a person would be interested in just one at a time?

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #17)
> One of the funny quirks with this patch right now is that if you open the
> toolbox and close it with the developer toolbar opened, the toolbar closes
> with the toolbox. Surprising. :)

I don't see the toolbat closing, but I do see the protocol connection closing, which is a variation on the same theme from comment 7 :-)

This is my next TODO item when I get back at this bug.

> I like the tools overall, but wonder if we shouldn't try to combine jank +
> reflows somehow. Having the two of them feels wasteful especially when
> they're so similar. Though maybe a person would be interested in just one at
> a time?

It's an interesting idea. I'm not sure that the scale of the graphs wouldn't be too far apart (rendering one of them tiny), but that could be perceived as a good thing, as it gives you an obvious hint of where you should focus your optimizations first.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Thinking about it some more, I'm worried that displaying two area graphs with unrelated data would often hide the one in the back even when its values would be concerningly high, because it happened to be overshadowed by a sudden steep climb of the front graph.

The memory graph does not have this problem, because the graph in the back is always the total amount of memory. A possible way out of this would be to use the same trick as the memory graph tooltip, which uses an area chart for the total memory and line charts for the subcategories. I'm concerned however that this would de-emphasize the importance of the front graph or imply a relationship between the two graphs that doesn't exist.

Comment 20

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8389990 [details] [diff] [review]
v6

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

we've talked about this. It's awesome. Most of my concerns are around the toolbar and the weird way we have to wedge targets into it to make these work. We spoke yesterday about moving the toolbar into the toolbox. With that as a prereq, I think this makes sense.

::: toolkit/xre/EventTracer.cpp
@@ -64,5 @@
>  #include <prinrval.h>
>  #include <prthread.h>
>  #include <prtime.h>
>  
> -#ifdef MOZ_WIDGET_GONK

are these includes only needed for GONK? scratch that, it's already in the shipping version of this file. Rebase'll clear it.

Comment 21

[Paul Rouget [:paul]]

The jank monitor will be misleading as its scope will be *everything* (not just the tab).

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

That already applies to the profiler as well, but I would argue that it is still useful information to have. Profiling an app should always happen in a browser instance with a single tab open to minimize interference, and even then care should be taken to minimize interference from other OS processes.

Comment 23

[Rob Campbell [:rc] (:robcee)]

we're also hoping to focus the profiler on only the active tab. See bug 899853.

Comment 24

[Dave Huseby [:huseby]]

I'm tagging this bug so that the b2g perf team can track the work.

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

We have changed our plans here a long time ago. The partially landed performance and timeline tools are our way forward here.