960671 - Live Memory View

Status: RESOLVED INVALID

(DevTools :: Memory, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We should have a live memory overview that includes a graph of memory usage over time with different events overlaid. The overlaying of events enables developers to correlate event handling with memory allocation. If an event handler is allocating a lot of memory and it isn't reclaimed at the next full GC, that is indicative of a leak.

The memory graph should also be able to be broken down by groups. Whether this is an overlay of multiple graphs at a time (turning different groups on or off) or toggling which graph you are viewing is not clear at the moment. Will defer to the next round of UI mockups and discussion with Darren.

We might also overlay some stats on peak memory usage and allow for setting a memory budget and highlighting whenever total memory usage goes past that. These can probably be follow ups.

njn says that counting allocations isn't reliable, and if we want exact numbers, we need to do a full heap traversal. Full heap traversals are too expensive to perform at the frequency we need to make a live graph. The compromise is to do full traversals at a moderately paced polled interval (as frequent as is practical) and then augment that data with frequent polling of allocations and de-allocations. Every time we do a full traversal, we know the truth and can reset our aggregate numbers, and in between while we are counting allocations we will have a good estimate.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: [e-mail]

Most recent mock up iteration.

Comment 2

[Nicholas Nethercote [inactive]]

Do you know about the fast per-tab measurements added in bug 918207?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Do you know about the fast per-tab measurements added in bug 918207?

My understanding is that this is useful for drawing the totals, but we wouldn't be able to break it down and categorize memory usage into different groups. Is this correct?

Comment 4

[Nicholas Nethercote [inactive]]

It gives a few buckets:
- JS objects
- JS strings
- JS other
- DOM
- Style
- Other

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8361231 [details]
[e-mail]

More recent mock up: https://people.mozilla.org/~dhenein/devtools/memtools/#/memory

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add attach/detch RDP requests to the MemoryActor.

This is just for giving the memory actor a debugger instance that is enabled and disabled on attach and detach respectively. Later patches will actually use the debugger for things like accessing allocation logs and taking censuses.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=43f9ab7aa909

Comment 7

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

Comment on attachment 8464382 [details] [diff] [review]
Part 1: Add attach/detch RDP requests to the MemoryActor.

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

::: toolkit/devtools/server/actors/memory.js
@@ +6,5 @@
>  
>  const {Cc, Ci, Cu} = require("chrome");
>  let protocol = require("devtools/server/protocol");
>  let {method, RetVal} = protocol;
> +const { reportException } = require("devtools/toolkit/DevToolsUtils");

Now the destructuring assignments in this file have inconsistent spacing :'(

@@ +27,5 @@
> +function expectState(expectedState, method) {
> +  return function(...args) {
> +    if (this.state !== expectedState) {
> +      return Promise.reject(new Error("Wrong State: Expected '" + expectedState
> +                                      + "'. Current state is '" + this.state

Wouldn't this be better as "Wrong state: expected foo, but current state is bar"?

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add attach/detch RDP requests to the MemoryActor.

Updated based on review comments. Carrying over r+.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add RDP requests for recording allocations.

This patch adds a request to start and stop the recording of allocations in a memory actor's debuggees and a way to get a list of the recent allocations.

Note that the Debugger API for providing the log of allocations hasn't landed yet, and is in bug 1001678.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c7b833dbb80a

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/20c0341fb3ac

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20c0341fb3ac

Comment 12

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

Comment on attachment 8465088 [details] [diff] [review]
Part 2: Add RDP requests for recording allocations.

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

I'm trying hard to find something important to point out, but it's all good, sorry!

::: toolkit/devtools/server/actors/memory.js
@@ +166,5 @@
> +    response: RetVal("json")
> +  }),
> +
> +  /**
> +   * TODO FITZGEN

Forgotten?

::: toolkit/devtools/server/tests/mochitest/test_memory_allocations_01.html
@@ +48,5 @@
> +      var frame = response.frames[alloc];
> +      return frame.functionDisplayName === "inner"
> +        && (frame.line === alloc1.line
> +            || frame.line === alloc2.line
> +            || frame.line === alloc3.line);

I must say I've always preferred the style with || in the end as it lines up the expressions nicely, but I'm not picky about it.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add RDP requests for recording allocations.

Woops, added the comment that used to be a TODO.

Carrying over r+.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Darrin, can you make an icon for the memory tool? Thanks!

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Add an empty memory panel

Adds an empty memory panel. Subsequent patches will actually do stuff here.

:gps, can you review the build stuff?

:vp, can you review everything else?

Comment 16

[Victor Porof [:vporof][:vp]]

Comment on attachment 8465778 [details] [diff] [review]
Part 3: Add an empty memory panel

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

Good core.

::: browser/devtools/memory/moz.build
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +JS_MODULES_PATH = 'modules/devtools/memory'

We don't need this anymore.

@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +JS_MODULES_PATH = 'modules/devtools/memory'
> +
> +EXTRA_JS_MODULES += [

This should change to EXTRA_JS_MODULES.devtools.memory += ...

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8465778 [details] [diff] [review]
Part 3: Add an empty memory panel

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

My comments have already been stated by vporof. Rebasing this on top of central should fail the build without those comments addressed.

Generally speaking, you don't need build peer review for just moz.build changes. 90% of our attitude is "if moz.build lets you do it, it is OK." Makefile.in files, however, should always get build peer review.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Add a way to force GC to the memory actor.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8468109 [details] [diff] [review]
Part 4: Add a way to force GC to the memory actor.

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

Looks good!

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Add an empty memory panel.

Updated based on review comments.

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8465598 [details] [diff] [review]
Part 2: Add RDP requests for recording allocations.

Landed part 2 in fx-team: https://hg.mozilla.org/integration/fx-team/rev/085acacbd031

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 5: Add a memory overview graph.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_memory_allocations_01.html crashes.
https://hg.mozilla.org/integration/fx-team/rev/97242044fbae

https://tbpl.mozilla.org/php/getParsedLog.php?id=46184815&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=46184904&tree=Fx-Team

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 6: Add a GC button

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Darrin, can you provide an icon for the GC button? I was going to use the one from the mockup, but it seems to be gone now.

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add RDP requests for recording allocations.

Updating this patch to

* Handle name change s/flushAllocationsLog/drainAllocationsLog/ (which caused the timeouts that got the last version backed out)

* Handle null frames (which we needed to introduce in bug 1055206)

* Keep an aggregate/summary allocation count for each frame

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8475454 [details] [diff] [review]
Part 2: Add RDP requests for recording allocations.

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

Seems good overall!

::: toolkit/devtools/server/actors/memory.js
@@ +103,5 @@
> +   *
> +   * @param Boolean shouldRecord
> +   *        True to enable recording, false to disable it.
> +   */
> +  recordAllocations: method(expectState("attached", function(shouldRecord) {

Maybe a separate method to stop instead of a boolean arg?  If this is a common pattern in other places, then okay I suppose, but it reads a bit strangely.

@@ +255,5 @@
> +      if (!this._allocationCounts.has(stack)) {
> +        this._allocationCounts.set(stack, 1);
> +      } else {
> +        let count = this._allocationCounts.get(stack);
> +        this._allocationCounts.set(stack, count + 1);

Always makes me sad that it takes this many lines to "increment or init" with a map... ;)

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Stephen, dcamp says you're the person to go to now for UI/UX stuff. We need an icon for the soon-to-exist memory panel :)

Comment 29

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: memory-frontend.patch

WIP combined patch of what I have so far. Sorry future-reviewers but it was too much to keep separate (although I will factor out the actor changes from the actual UI once I submit for review).

I know that the graph disappears once you come back to the overview tab. Just ahven't gotten around to fixing it yet.

I know that there is this weird buffer in the allocations tree, and I can't figure it out. I'm 99% sure it isn't the buffer elements used to maintain the correct scroll position for the whole only-render-items-in-view thing; I think it is something else. CSS is hard. Help would be appreciated if anyone happens to have the time.

Comment 30

[Nicholas Nethercote [inactive]]

I want to try this. The patch has some conflicts with current mozilla-inbound, which I resolved, but when I run it and open the devtools panel I don't see anything about memory profiling. What am I missing? Thanks.

Comment 31

[Paul Rouget [:paul]]

Attachment: quick and dirty rebase

Nicholas, here is a rebased-patch you can use. I found that to make sure it works properly, the memory panel needs to be opened as the default tool, so open the devtools, select the memory panel, and close the devtools. Now open the devtools again, it should work.

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yeah, I've been working off of fx-team from sometime last week.

I've just been using Cmd+Opt+I to open the last used panel. Will look into the bug where it breaks if you start from a different panel.

Nicholas, you also have to open the devtools options panel and enable the memory panel, as it is pref'd off by default. It should be a checkbox under "Default Firefox Developer Tools" here: https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Settings

Comment 33

[Nicholas Nethercote [inactive]]

I tried the patch out on pdf.js, which is a codebase for which I've done a lot
of memory profiling (using my grungy home-made tools) and so I understand it
quite well. I'd say the tool is looking promising, but there are some
improvements needed before it provides significant value.

Also it's interesting to see the overlap in the UI between this tool and
about:memory. There's a lot of the same issues in play when presenting large
trees of measurements.

Here are my comments. Some of this stuff you may already have plans to address;
I just reviewed the patch in its current form without any knowledge of what
else you're planning to add.

Both tabs:

- Right-justifying numeric columns makes them easier to read. (Fixed-width
  fonts help even more, though I can see that this may be more a matter of
  taste.)

- All the numbers need commas (or whatever separator the locale requires). This
  makes them *much* more readable, esp. when they get big. Mentally parsing
  numbers like "25878454" is no fun.
  
- For byte counts, reporting MiB instead of bytes is often a better option.

"Overview" tab:

- "js::Shape", "js::BaseShape", and "js::types::TypeObject" are meaningless to
  the typical JS programmer. First, the namespace prefixes should be dropped.
  And then some way (a tool-tip) of briefly explaining what they are would be
  helpful.

- No measurement of scripts? They're on the GC heap and can take up a
  significant amount of space.

"Allocations" tab:

- This just measures objects, right? That's not obvious. (In contrast, the
  "Overview" tab measures objects, strings, shapes, type objects.)

- I'd love it if strings could be measured too. I know the JS engine doesn't
  have infrastructure for this, and it would be hard to add... but for pdf.js,
  at least, strings are often a big fraction of the total memory use.

- Going from object counts to byte counts would make this *much* more
  interesting. E.g. sometimes you have typed arrays that are multiple MiBs in
  size, so treating them the same as tiny objects is misleading. Of all the
  suggestions I've made, this is the most important -- for me, it would take
  this tool from an interesting curiosity to something that I would likely use
  with some regularity.

  (I know this is expensive. But given the choice between live updates and byte
  counts, I'll take byte counts every time.)

- The live updating of the allocation tree surprised me. It's interesting to
  see how it changes, but if you want to take a close look it's annoying when
  it changes while you're reading it. Is there a way to take a snapshot?

- Percentages as well as counts would be very helpful. I found myself basically
  constantly computing percentages in my head, and probably getting them wrong.

- The call tree is shown partially expanded. It's not clear to me what dictates
  which nodes are expanded and which are collapsed... does it just expand
  everything to a particular depth?
  
  The functions with high "self allocations" counts are the interesting ones,
  and sometimes I have to click several times to unfold to the interesting
  function. It would be nice if the tree automatically expanded to show all the
  functions with self-counts higher than particular threshold.

  Similarly, a huge fraction of the tree is taken up showing functions that
  have tiny numbers of allocations, which are boring. It would be nice if these
  could be hidden. It might speed things up a lot, too.

  (about:memory deals with both of these issues by only expanding sub-trees
  that account for at least 1% of the total, and inserting "tiny" nodes as a
  way of hiding the parts of sub-trees that are below this threshold. It also
  has a verbose mode that just shows everything fully expanded, for when you
  want that.)

- And because the "self allocation" counts are the most interesting, being able
  to invert the tree would be really cool. (Shark on Mac used to let you do
  that.) In fact, I think an inverted tree would be more useful than the normal
  tree, because much of the juicy stuff will be right there up at the first
  layer, and you can drill down if you need more context, but often you don't.
  (My grungy tools only provide a single stack frame of context, and 90% of the
  time that's enough.)

Comment 34

[Nicholas Nethercote [inactive]]

To summarize comment 33:

- Most of my comments are about presentation, and so should be relatively easy to address.

- The two big omissions are:

  * No measurement of strings in the "Allocation" tab. (Nor of shapes or scripts, but those are internal structures, which means they are harder to reason about, harder to directly influence, and thus less important.)

  * It only measures object counts, not bytes.

  There's machinery in place that can be used to address the latter, but not the former.

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Just an update for people wondering what happened here, and why there hasn't been any new patches: I talked with product/management and we came to a consensus that the prototype I built here isn't going to ship, the allocations/GC-pressure stuff belongs in the holistic performance/profiler/timeline tool (bug 1066361), and that we won't build/ship a full blown memory panel until heap snapshots are implemented (bug 1024774).

njn: Thanks for the very detailed feedback! I appreciate it -- sorry I didn't reply earlier. I'll make sure that the folks who integrate the allocations/GC-pressure stuff into the holistic performance tool take it into consideration.

Comment 36

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8475454 [details] [diff] [review]
Part 2: Add RDP requests for recording allocations.

A different revision of this patch landed in bug 1067491.

Comment 38

[Alex Art]

Have you ever seen Java's Netbeans profiling tool?
It's a pleasure to work with it when you hunting a memory leak or something.
You are turning on the memory profiler for a while, it collects information from memory about objects their quantity, lifetime, size (recursively summarized if developer selects only certain package) and generations. And the most greatest thing, it shows them in code-view and has a plenty of greatest filters to display objects by name, package name etc.

Comment 39

[Jordan Santell [:jsantell] [@jsantell]]

Closing as invalid -- memory tool shipping in fall release handled in bug 1195323

Comment 40

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.