Closed Bug 960671 Opened 8 years ago Closed 6 years ago

Live Memory View

Categories

(DevTools :: Memory, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: fitzgen, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [apps watch list])

Attachments

(1 file, 12 obsolete files)

13.70 KB, patch
fitzgen
: review+
fitzgen
: checkin+
Details | Diff | Splinter Review
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.
Attached image DevTools-i04-Performance@2x.png (obsolete) —
Most recent mock up iteration.
Do you know about the fast per-tab measurements added in bug 918207?
(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?
It gives a few buckets:
- JS objects
- JS strings
- JS other
- DOM
- Style
- Other
Depends on: 961288
Depends on: 961323
Depends on: 961324
Depends on: 961325
No longer depends on: 961288, 961323, 961324
Whiteboard: [apps watch list]
Attachment #8361231 - Attachment is obsolete: true
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Depends on: 1039952
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
Attachment #8464382 - Flags: review?(past)
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"?
Attachment #8464382 - Flags: review?(past) → review+
Whiteboard: [apps watch list] → [apps watch list][status:inflight]
Updated based on review comments. Carrying over r+.
Attachment #8464382 - Attachment is obsolete: true
Attachment #8464889 - Flags: review+
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
Attachment #8465088 - Flags: review?(past)
Depends on: 1001678
No longer depends on: memory-platform
https://hg.mozilla.org/integration/fx-team/rev/20c0341fb3ac
Keywords: checkin-needed
Whiteboard: [apps watch list][status:inflight] → [apps watch list][status:inflight][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/20c0341fb3ac
Whiteboard: [apps watch list][status:inflight][fixed-in-fx-team] → [apps watch list][status:inflight]
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.
Attachment #8465088 - Flags: review?(past) → review+
Woops, added the comment that used to be a TODO.

Carrying over r+.
Attachment #8465088 - Attachment is obsolete: true
Attachment #8465598 - Flags: review+
Darrin, can you make an icon for the memory tool? Thanks!
Flags: needinfo?(dhenein)
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?
Attachment #8465778 - Flags: review?(vporof)
Attachment #8465778 - Flags: review?(gps)
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 += ...
Attachment #8465778 - Flags: review?(vporof) → feedback+
Attachment #8465778 - Flags: feedback+ → review+
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.
Attachment #8465778 - Flags: review?(gps)
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!
Attachment #8468109 - Flags: review?(jryans) → review+
Updated based on review comments.
Attachment #8465778 - Attachment is obsolete: true
Attachment #8468777 - Flags: review+
Attachment #8468777 - Attachment description: bug-960671-part-3-empty-memory-panel.patch → Part 3: Add an empty memory panel.
Whiteboard: [apps watch list][status:inflight] → [status:inflight][apps watch list]
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
Attachment #8474700 - Flags: review?(vporof)
Depends on: 1055206
No longer depends on: 961325
Attached patch Part 6: Add a GC button (obsolete) — Splinter Review
Attachment #8474751 - Flags: review?(vporof)
Attachment #8474751 - Attachment description: Bug 960671 - Part 6: Add a GC button → Part 6: Add a GC button
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.
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
Attachment #8465598 - Attachment is obsolete: true
Attachment #8475454 - Flags: review?(jryans)
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... ;)
Attachment #8475454 - Flags: review?(jryans) → review+
Attachment #8474751 - Flags: review?(vporof)
Attachment #8474700 - Flags: review?(vporof)
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 :)
Flags: needinfo?(dhenein) → needinfo?(shorlander)
Attached patch memory-frontend.patch (obsolete) — Splinter Review
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.
Attachment #8468109 - Attachment is obsolete: true
Attachment #8468777 - Attachment is obsolete: true
Attachment #8474700 - Attachment is obsolete: true
Attachment #8474751 - Attachment is obsolete: true
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.
Flags: needinfo?(nfitzgerald)
Attached patch quick and dirty rebase (obsolete) — Splinter Review
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.
Flags: needinfo?(nfitzgerald)
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
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.)
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.
Depends on: 1067491
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.
Assignee: nfitzgerald → nobody
Status: ASSIGNED → NEW
Depends on: 1024774
Flags: needinfo?(shorlander)
Whiteboard: [status:inflight][apps watch list] → [apps watch list]
Attachment #8487436 - Attachment is obsolete: true
Attachment #8487814 - Attachment is obsolete: true
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.
Attachment #8475454 - Attachment is obsolete: true
Duplicate of this bug: 1120237
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.
Closing as invalid -- memory tool shipping in fall release handled in bug 1195323
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.