923275 - Add a memory monitor widget to the developer toolbar

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

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

Attachment: Screenshot

Bug 918207 will introduce a fast API to measure the memory footprint of a window. It would be useful to have this information surface in a monitoring widget like the ones we find in operating systems and window managers.

In the attached screenshot you can see my prototype widget in action. It appears on the right side of the developer toolbar and is controlled by an option in the toolbox's option panel.

Comment 1

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

Attachment: Add a memory monitor widget to the developer toolbar

Here is the patch that implements this widget. It needs to be applied on top of the patch from bug 918207. I get some rooting-related assertions on gmail and twitter, but github and various other pages work fine. I'm currently only drawing the total memory consumption of the page, due to the small size of the widget, but I'm considering adding some of the other measurements (primarily JS objects) as well.

I still need to take switching tabs into account, but navigating inside a tab works.

Comment 2

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

:D

Could you also add the total size (overlaid) on top of the graph? Need a number to make sense of the scale of things.

AWESOME.

Comment 3

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

Comment on attachment 813303 [details] [diff] [review]
Add a memory monitor widget to the developer toolbar

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

::: browser/devtools/shared/MemoryMonitor.jsm
@@ +56,5 @@
> +  document.getElementById("memory-monitor").appendChild(canvas);
> +};
> +
> +MemoryMonitor.prototype = {
> +  sampling: 1,

is this ms? seconds?

Would like to play with this value a bit to find a sweet spot.

@@ +64,5 @@
> +
> +    let browser = this._chromeWindow.getBrowser().selectedBrowser;
> +    browser.addEventListener("DOMWindowCreated", this.onWindowCreated, false);
> +
> +    this._interval = this._chromeWindow.setInterval(this.worker, this.sampling * 1000);

ok, so, seconds. :)

::: browser/themes/linux/devtools/common.css
@@ +141,5 @@
> +  background-position: 4px center;
> +  box-shadow: 0 1px 1px hsla(206,37%,4%,.2) inset,
> +              1px 0 0 hsla(206,37%,4%,.2) inset,
> +              -1px 0 0 hsla(206,37%,4%,.2) inset;
> +}

need to vet these colors with someone who knows color. Should probably be more pink.

Comment 4

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #2)
> Could you also add the total size (overlaid) on top of the graph? Need a
> number to make sense of the scale of things.

You mean like 50% transparent or something like that? I'll give it a try to see if I can find a good balance between making the number readable and not obscuring the graph too much.

(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> > +  sampling: 1,
> 
> is this ms? seconds?
> 
> Would like to play with this value a bit to find a sweet spot.

Yes it is seconds and it seems that with the new API we can get it down to a few hundred milliseconds in most cases if we wanted. I also considered adding a slider in the options panel to tweak this value, but I thought it might be overkill (mostly since the tradeoffs might not be apparent to a user). What do you think?

> ::: browser/themes/linux/devtools/common.css
> @@ +141,5 @@
> > +  background-position: 4px center;
> > +  box-shadow: 0 1px 1px hsla(206,37%,4%,.2) inset,
> > +              1px 0 0 hsla(206,37%,4%,.2) inset,
> > +              -1px 0 0 hsla(206,37%,4%,.2) inset;
> > +}
> 
> need to vet these colors with someone who knows color. Should probably be
> more pink.

Haha. FWIW I copied these values from the gcli input box for consistency, since they are adjacent and I used the blue color for the graph from shorlander's palette.

Comment 5

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

Attachment: Add a memory monitor widget to the developer toolbar v2

Now with support for switching tabs.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Is there some way to make this remotable? i.e show the memory usage of the current debugger target instead of the running Firefox instance? I'd suspect sampling might need to be handled a bit differently.

Comment 7

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

Yes, remoting is something I plan on fixing before this lands. Nick and I have been discussing some of the challenges involved in monitoring B2G, but basic remote monitoring for desktop and Android is a hard requirement for landing this.

Comment 8

[Joe Walker [:jwalker]]

When JSTerm lands, I was thinking we could remove the developer toolbar. Certainly there's no need for the GCLI input area any more, and right now there's not a lot left if you remove that.
Perhaps there's space for this widget in the toolbox? I'm not against this landing, but I wouldn't like to add moving the widget to the list of things that could block the landing of JSTerm.

Comment 9

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

Attachment: Add a memory monitor widget to the developer toolbar v3

Add a second line graph for the memory consumed by JS objects and a tooltip to display the actual numbers for both measurements.

Comment 10

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

Putting the memory monitor in the toolbox tab strip was the backup plan for when the developer toolbar is retired. Do we have an estimate for when that will happen? This bug and its dependency need a few more weeks, so I could switch plans if JSTerm is going to land in the next month or so.

Comment 11

[Joe Walker [:jwalker]]

(In reply to Panos Astithas [:past] from comment #10)
> Putting the memory monitor in the toolbox tab strip was the backup plan for
> when the developer toolbar is retired. Do we have an estimate for when that
> will happen? This bug and its dependency need a few more weeks, so I could
> switch plans if JSTerm is going to land in the next month or so.

JSTerm is close, but I fear it's going to take a while to land because there's a lot of code to get right, so I'd hazard a guess at 6 weeks before landing.

Comment 12

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

Attachment: Add a memory monitor widget to the developer toolbar v4

Updated to work with the latest sizeOfTab changes.

Comment 13

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

Attachment: Screenshot

Latest screenshot with tooltip and the additional JS object graph.

Comment 14

[Paul Rouget [:paul]]

What about drawing the graph over the content, like https://github.com/mrdoob/stats.js and Chrome (their FPS counter)?

Comment 15

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

I find both rather ugly to be honest, but that's not the main issue. What happens when we get another widget, say for displaying an FPS counter, and then a third one? Do we stack them horizontally or vertically? At which point does hiding the content become an issue? Also, how do you use those widgets against a remote target?

Comment 16

[Paul Rouget [:paul]]

(In reply to Panos Astithas [:past] from comment #15)
> I find both rather ugly to be honest, but that's not the main issue. What
> happens when we get another widget, say for displaying an FPS counter, and
> then a third one? Do we stack them horizontally or vertically? At which
> point does hiding the content become an issue? Also, how do you use those
> widgets against a remote target?

Shorlander (or someone else from UX) will look at this.

Comment 17

[Stephen Horlander [:shorlander]]

I think Darrin is going to look into this.

Comment 18

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

Attachment: Make the monitor widget remotable

This patch applied on top of the other one makes the widget use the remote protocol for getting the memory measurements. It doesn't use protocol.js because I was having problems getting it to work. The remote connection is now manually created from the widget, but depending on how and where we will be eventually displaying it, it may need to be changed to reuse a pre-existing connection.

I have also added a new toggle devtools.memory.monitor.browser (off by default) in lieu of a more reasonable UI to make the widget report the memory footprint of the entire browser. If we decide to integrate it with the toolbox, no extra switch will be necessary as we will be able to use the target of the toolbox.

The protocol interaction is pretty simple, here is the request/response:

{
  "to": "conn0.memory9",
  "type": "measure"
}

{
  "total": 18726808,
  "domSize": 3585968,
  "styleSize": 7169016,
  "jsObjectsSize": 1143904,
  "jsStringsSize": 3424,
  "jsOtherSize": 2465144,
  "otherSize": 4359352,
  "jsMilliseconds": "126.9",
  "nonJSMilliseconds": "11.2",
  "from": "conn0.memory9"
}

I've also added a test that makes sure the packet shape is as expected.

I've been seeing some crashes and hangs, especially when switching tabs, but I will file separate bugs for those.

Comment 19

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

Attachment: The protocol.js-based version

Here is my alternative version with protocol.js (minus good target switching and the chrome pref). If I get a chance to figure out what is wrong with the front, I will switch to this one. It applies on top of the first patch only.

Comment 20

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

Attachment: The protocol.js-based version v2

With dcamp's help I was able to get the protocol.js version to work, so I added the extra features from the other version as well.

Comment 21

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

Attachment: Add a memory monitor widget to the developer toolbar v5

Might as well merge the patches together.

Comment 22

[Paul Rouget [:paul]]

I'm still not convinced this should live in the dev toolbar. First, this will make this feature useless for Firefox OS development (only the toolbox is connected to the device), and second, the toolbar will disappear once jsterm lands.

Comment 23

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

Attachment: Add a memory actor for collecting memory usage data

I'm splitting the patch in two parts: the backend, which can land rigth away, and the frontend, which will have to wait until our UI plans are finalized.

This is the server-side part, with the memory actor and the test.

Comment 24

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

Attachment: Add a memory monitor widget to the developer toolbar

This is the frontend part, which can be used to verify the functionality of the other patch. We will probably have to grab some bits and pieces from this when we implement the final UI.

Comment 25

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

(In reply to Paul Rouget [:paul] from comment #22)
> I'm still not convinced this should live in the dev toolbar. First, this
> will make this feature useless for Firefox OS development (only the toolbox
> is connected to the device), and second, the toolbar will disappear once
> jsterm lands.

this isn't going to be our big memory profiling tool. This is a light-weight thing for monitoring desktop tab memory usage. While the data might be useful for a remote device I think we should take a different approach or use the full memory profiler when it's available.

I don't think we should block one for the other. Different uses.

Comment 26

[Paul Rouget [:paul]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #25)
> this isn't going to be our big memory profiling tool. This is a light-weight
> thing for monitoring desktop tab memory usage. While the data might be
> useful for a remote device I think we should take a different approach or
> use the full memory profiler when it's available.
> 
> I don't think we should block one for the other. Different uses.

This addresses my first point (good!).

What about the fact we'll be killing the toolbar soon?

What about using a Australis widget? See demo here (jump to 6:00): http://people.mozilla.org/~jwein/australis-widgets.webm

Comment 27

[Alexandre Poirot [:ochameau]]

Using a widget won't help making it work with remotes, is it?
Do we have a plan for a general timeline panel that can be used to prevent various data that makes better sense to be displayed on a time basis?

Anyway, landing the actor would make it easier to start pulling memory information.
If we don't feel confident about the UI, or feel it will mess up with upcoming perf tools story,
we can experiment various options through an Addon as soon as the remote ships the actor!
And we can offer such experimental tools to b2g perf team to help them debug gaia right away.

Comment 28

[Paul Rouget [:paul]]

We tested this actor on Firefox OS, and it works.

Comment 29

[Jan Keromnes [:janx]]

Awesome! +1 for landing the actor, I'm using the first patch on Firefox OS to display app memory info directly on the device.

Comment 31

[Nicholas Nethercote [inactive]]

BTW, bug 472209 has a couple of previous attempts at this feature. Might be worth looking at for additional ideas.

Comment 32

[Paul Rouget [:paul]]

Jimb, ping?

Comment 33

[Paul Rouget [:paul]]

Comment on attachment 8341668 [details] [diff] [review]
Add a memory actor for collecting memory usage data

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

Stealing Jim's review. This is basically just forwarding a JSON object. r+

Can you make sure this memory actor is correctly registered for B2G too?
Can we land the frontend part in a followup bug?

Comment 34

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

Attachment: Add a memory actor for collecting memory usage data v2

Fixed registration on b2g.

Comment 35

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

And to be clear, I have no plans to land the frontend part as it is. Our plans on that area have changed as discussed at the last weekly call.

Comment 36

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

Passed local tests on desktop and b2g. Landed:
https://hg.mozilla.org/integration/fx-team/rev/0a2824f4e03b

Comment 37

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

Attachment: Add a memory monitor widget to the developer toolbar v2

Here is a rebased frontend patch that is *not* going to land.

Comment 38

[Jim Blandy :jimb]

Thanks, Paul. Sorry, Panos.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a2824f4e03b

Comment 40

[Florian Bender]

Was this bug supposed to be closed?

Comment 41

[Nicholas Nethercote [inactive]]

> Was this bug supposed to be closed?

I was wondering the same thing. Seems like Panos landed some more supporting infrastructure, but not the actual widget?

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Paul Rouget [:paul] from comment #28)
> We tested this actor on Firefox OS, and it works.

Paul, does this allow the user to monitor memory usage of the parent process vs child content processes?

I had an idea to capture some memory values in the SPS profiler over in bug 964096.  I think overlaying this on the stack samples could be really useful to detect code causing memory spikes.

Anyway, just wondering if I am overlapping with the work here or working on something more orthogonal.  Thanks!

Comment 43

[Nicholas Nethercote [inactive]]

> Anyway, just wondering if I am overlapping with the work here or working on
> something more orthogonal.  Thanks!

AIUI, this bug is about measurements just for the current tab, whereas bug 964096 is about global measurements.

Comment 44

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

The decision was to not pursue this widget any further, but focus instead on the more comprehensive tool that is described here:

https://etherpad.mozilla.org/memory-tool

The bugs that track all that work can be seen here:

https://bugzilla.mozilla.org/showdependencygraph.cgi?id=960671%2C+960662%2C+960675&display=tree&rankdir=TB

The memory actor that laded in this bug will be part of all that, and can already be used by complementary tools, like the ones in bug 963498 (for Firefox OS) and bug 932609 (for the command line).

Comment 45

[Paul Rouget [:paul]]

(In reply to Ben Kelly [:bkelly] from comment #42)
> (In reply to Paul Rouget [:paul] from comment #28)
> > We tested this actor on Firefox OS, and it works.
> 
> Paul, does this allow the user to monitor memory usage of the parent process
> vs child content processes?

Yes. We can display parent memory and child memory.

Comment 46

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

(In reply to Panos Astithas [:past] from comment #44)
> The decision was to not pursue this widget any further, but focus instead on
> the more comprehensive tool that is described here:

Change of plans once more: after discussions with PM, EM & UX we will do both the comprehensive tool and the small monitoring widget. I have incorporated the uncommitted patch here into the larger one in bug 975675.