The default bug view has changed. See this FAQ.

Disallow non-leaf memory reporters

RESOLVED FIXED in mozilla12

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Memory reporters have a path.  Many of them have segments that form part of a tree, e.g. the "explicit" tree.  Bug 653630 allowed non-leaf reporters, whereby you could have something like "explicit/foo" and "explicit/foo/a" and aboutMemory.js would add in "explicit/foo/other" which was "explicit/foo" - "explicit/foo/a".

This was necessary for the SQLite reporters, because SQLite provides a single reporter for its entire memory use, and then reporters for each of its connections, but we don't have a single list of those connections anywhere so we had to do the arithmetic in aboutMemory.js rather than creating an "sqlite/other" reporter.

Unfortunately, allowing non-leaf reporters made aboutMemory.js more complicated and I've been annoyed by it for ages.  Bug 700217 is the catalyst for me filing this bug.

The only hard part is dealing with the SQLite reporters.  One way would be to replace the "storage/sqlite" reporter with a "storage/sqlite/other" reporter, which would require the aforementioned list of all SQLite connections.  Probably not too hard.  A hacky alternative would be to special-case "storage/sqlite", making it the only allowed non-leaf reporter.  (Brendan has a philosophy that it's generally ok to have one exception to a rule, which I think is an interesting idea!)  That would still allow a lot of simplification to occur.
(Assignee)

Comment 1

5 years ago
Created attachment 573750 [details] [diff] [review]
patch

> The only hard part is dealing with the SQLite reporters.  
> [...]
> A hacky alternative would be to
> special-case "storage/sqlite", making it the only allowed non-leaf reporter.

I did this, it was easier than working out how to track all the SQLite connections.  aboutMemory.js's complexity didn't change much -- it no longer has to deal with non-leaf nodes, which is good, but it has to special case "explicit/storage/sqlite", which is bad.

GetExplicit is much simpler, though, and avoids the quadratic case.

I need to test the sqlite special-casing in test_aboutmemory.xul, I'll do that on Monday.
Assignee: nobody → nnethercote
Attachment #573750 - Flags: review?(justin.lebar+bug)
I'm not certain that this is the right way to go -- who's to say we won't have another SQLite-like situation on our hands -- but I don't see the harm in trying it out.  This one-exception rule is interesting...
(Assignee)

Comment 3

5 years ago
(In reply to Justin Lebar [:jlebar] from comment #2)
> I'm not certain that this is the right way to go -- who's to say we won't
> have another SQLite-like situation on our hands -- but I don't see the harm
> in trying it out.  

I think SQLite is pretty unusual.  It's a sub-system we can't modify, and yet it provides detailed memory reporting which we use to implement our memory reporters.  I don't think we'll have another case like it.  I can't prove this, of course, so in the meantime I'll claim YAGNI (or maybe "YAGNI... more than once").  It sure makes thinking about memory reporters easier.

> This one-exception rule is interesting...

Yeah, it's kind of a quasi-inverse of the "don't generalize something until you have at least two instances of it" rule of thumb.
Comment on attachment 573750 [details] [diff] [review]
patch

>-// There are two kinds of TreeNode.  Those that correspond to Reporters
>-// have more properties.  The remainder are just scaffolding nodes for the
>-// tree, whose values are derived from their children.
>+// There are two kinds of TreeNode.
>+// - Leaf TreeNodes, that correspond to Reporters and have more properties.  
>+// - Non-leaf TreeNodes are just scaffolding nodes for the tree;  their values
>+//   are derived from their children.

Nit: Parallelism.  "Non-leaf TreeNodes, *which* are just scaffolding"

>@@ -413,28 +420,26 @@ TreeNode.compare = function(a, b) {
>  * @return The built tree.
>  */
> function buildTree(aReporters, aTreeName)
> {
>   // We want to process all reporters that begin with |aTreeName|.  First we
>   // build the tree but only fill the properties that we can with a top-down
>   // traversal.
> 
>-  // Is there any reporter which matches aTreeName?  If not, we'll create a
>-  // dummy one.
>+  // Is there any reporter which matches aTreeName?  (This should never happen
>+  // for "explicit", only for the "map" trees.)  If not, bail.

What should never happen for "explicit"?  Shouldn't you reverse this?  "We
should always find a reporter when aTreeName == explicit, but some 'map' trees
might be missing".  In fact, you could

  assert(aTreeName != 'explicit' || foundReporter).

>+  var heapUnclassifiedT = new TreeNode("heap-unclassified");

Kind of a weird variable name...  I guess it's supposed to be like |aT|?  Maybe
|heapUnclassifiedNode| would be better?

>+   *   Each reporter can be viewed as representing a leaf node in a tree
>+   *   rooted at "explicit".  Internal nodes of the tree don't have
>+   *   reporters.  associated reporter.  So, for example, the reporters

Typo.

>+   *   "explicit/a/b", "explicit/a/c", "explicit/d/e", and "explicit/d/f"
>+   *   define this tree:
>    *
>    *     explicit
>    *     |--a
>    *     |  |--b [*]
>    *     |  \--c [*]
>-   *     \--d [*]
>+   *     \--d
>    *        |--e [*]
>    *        \--f [*]
>    *
>-   *   Nodes marked with a [*] have a reporter.  Notice that "explicit/a" is
>-   *   implicitly defined.
>+   *   Nodes marked with a [*] have a reporter.  Notice that the internal
>+   *   nodes are implicitly defined by the paths.

I'd add an explicit mention of the fact that non-leaf reporters other than
sqlite/other are not allowed.  (The sqlite/other special-case is now part of
our interface, so we might as well document it here.)

> NS_IMETHODIMP
> nsMemoryReporterManager::GetExplicit(PRInt64 *aExplicit)
> {
>-    InfallibleTArray<MemoryReport> nonheap;
>-    PRInt64 heapUsed = PRInt64(-1);
>+    *aExplicit = 0;

We usually do |NS_ENSURE_ARG_POINTER(aExplicit)| first, just to be paranoid.

>@@ -648,76 +634,54 @@ nsMemoryReporterManager::GetExplicit(PRI
> 
>         nsCString path;
>         rv = r->GetPath(path);
>         NS_ENSURE_SUCCESS(rv, rv);
> 
>         // We're only interested in NONHEAP explicit reporters and
>         // the 'heap-allocated' reporter.
>         if (kind == nsIMemoryReporter::KIND_NONHEAP &&
>-            path.Find("explicit") == 0) {
>-
>+            path.Find("explicit") == 0)
>+        {

This doesn't seem to be the prevailing style in this file.

r=me, but we need to test the sql reporter hack.
Attachment #573750 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Updated

5 years ago
Depends on: 702815
(Assignee)

Comment 5

5 years ago
Bug 702815 will allow the sqlite hack to be removed :)
(Assignee)

Comment 6

5 years ago
Created attachment 574773 [details] [diff] [review]
patch without the sqlite hack

This can't land until bug 702815 is done, obviously.
Attachment #573750 - Attachment is obsolete: true
Attachment #574773 - Flags: review?(justin.lebar+bug)
Comment on attachment 574773 [details] [diff] [review]
patch without the sqlite hack

Is this the right patch?
(Assignee)

Comment 8

5 years ago
Created attachment 574989 [details] [diff] [review]
patch without the sqlite hack (actual)

Sorry, here's the right patch.
Attachment #574773 - Attachment is obsolete: true
Attachment #574773 - Flags: review?(justin.lebar+bug)
Attachment #574989 - Flags: review?(justin.lebar+bug)
Comment on attachment 574989 [details] [diff] [review]
patch without the sqlite hack (actual)

Wow, bugzilla's interdiff actually worked.  That was cool.
Attachment #574989 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 10

5 years ago
I've found the 'interdiff' that comes with patchutils on Linux is much more robust than Bugzilla's.
Yeah, I usually do

  interdiff <(curl -L http://attachment-url-1) <(curl -L http://attachment-url-2) | vim -
(Assignee)

Updated

5 years ago
Depends on: 703143
(Assignee)

Updated

5 years ago
No longer depends on: 702815
(Assignee)

Updated

5 years ago
Blocks: 698846
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/61d41436beb3
https://hg.mozilla.org/mozilla-central/rev/61d41436beb3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Comment 14

5 years ago
Backed out in order to back out bug 703143:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c52a726dc939
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/c52a726dc939
Target Milestone: mozilla11 → ---
(Assignee)

Comment 16

5 years ago
Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4779d8df054b
Backed out for linux64 moth orange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4779d8df054b

This patch pushed the number of assertions in about:memory up from 24K to 34K, making the log exceed the 52428800 bytes limit.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f17624f1ab7
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 18

5 years ago
> This patch pushed the number of assertions in about:memory up from 24K to
> 34K, making the log exceed the 52428800 bytes limit.

There were already 24K assertions?  I'm having trouble viewing the log, can you cut+paste some of them for me?
I was unable to load the log either earlier unfortunately, so those figures were from philor's analysis before he had to head out (presuming I've interpreted his comments correctly).

I've since looked up the raw tinderbox log url, saves trying to convince tbpl to load it (uncompressed the log is 70MB, so not surprising it was timing out):
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-debug/1325827944/mozilla-inbound_fedora64-debug_test-mochitest-other-build620.txt.gz

Hope that helps :-)
(Assignee)

Comment 20

5 years ago
With the first patch in bug 715453 having removed this assertion, I thought I'd give this another spin:

https://hg.mozilla.org/integration/mozilla-inbound/rev/797a60d83008
https://hg.mozilla.org/mozilla-central/rev/797a60d83008
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.