Last Comment Bug 700508 - Disallow non-leaf memory reporters
: Disallow non-leaf memory reporters
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Nicholas Nethercote [:njn]
:
:
Mentors:
Depends on: 703143
Blocks: 698846 700217
  Show dependency treegraph
 
Reported: 2011-11-07 16:39 PST by Nicholas Nethercote [:njn]
Modified: 2012-02-01 13:59 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (24.95 KB, patch)
2011-11-10 22:10 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Splinter Review
patch without the sqlite hack (3.14 KB, patch)
2011-11-15 19:24 PST, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
patch without the sqlite hack (actual) (18.06 KB, patch)
2011-11-16 13:51 PST, Nicholas Nethercote [:njn]
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-11-07 16:39:22 PST
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.
Comment 1 Nicholas Nethercote [:njn] 2011-11-10 22:10:17 PST
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.
Comment 2 Justin Lebar (not reading bugmail) 2011-11-11 08:52:35 PST
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...
Comment 3 Nicholas Nethercote [:njn] 2011-11-13 19:49:56 PST
(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 4 Justin Lebar (not reading bugmail) 2011-11-14 07:21:46 PST
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.
Comment 5 Nicholas Nethercote [:njn] 2011-11-15 17:59:39 PST
Bug 702815 will allow the sqlite hack to be removed :)
Comment 6 Nicholas Nethercote [:njn] 2011-11-15 19:24:33 PST
Created attachment 574773 [details] [diff] [review]
patch without the sqlite hack

This can't land until bug 702815 is done, obviously.
Comment 7 Justin Lebar (not reading bugmail) 2011-11-16 12:09:20 PST
Comment on attachment 574773 [details] [diff] [review]
patch without the sqlite hack

Is this the right patch?
Comment 8 Nicholas Nethercote [:njn] 2011-11-16 13:51:43 PST
Created attachment 574989 [details] [diff] [review]
patch without the sqlite hack (actual)

Sorry, here's the right patch.
Comment 9 Justin Lebar (not reading bugmail) 2011-11-16 14:52:47 PST
Comment on attachment 574989 [details] [diff] [review]
patch without the sqlite hack (actual)

Wow, bugzilla's interdiff actually worked.  That was cool.
Comment 10 Nicholas Nethercote [:njn] 2011-11-16 15:49:20 PST
I've found the 'interdiff' that comes with patchutils on Linux is much more robust than Bugzilla's.
Comment 11 Justin Lebar (not reading bugmail) 2011-11-16 16:18:50 PST
Yeah, I usually do

  interdiff <(curl -L http://attachment-url-1) <(curl -L http://attachment-url-2) | vim -
Comment 12 Nicholas Nethercote [:njn] 2011-12-06 18:59:34 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/61d41436beb3
Comment 13 Ed Morley [:emorley] 2011-12-07 02:31:49 PST
https://hg.mozilla.org/mozilla-central/rev/61d41436beb3
Comment 14 Nicholas Nethercote [:njn] 2011-12-08 11:51:08 PST
Backed out in order to back out bug 703143:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c52a726dc939
Comment 15 Ed Morley [:emorley] 2011-12-09 06:57:40 PST
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/c52a726dc939
Comment 16 Nicholas Nethercote [:njn] 2012-01-05 21:32:46 PST
Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4779d8df054b
Comment 17 Ed Morley [:emorley] 2012-01-06 08:32:03 PST
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
Comment 18 Nicholas Nethercote [:njn] 2012-01-06 14:41:42 PST
> 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?
Comment 19 Ed Morley [:emorley] 2012-01-06 15:03:47 PST
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 :-)
Comment 20 Nicholas Nethercote [:njn] 2012-01-07 02:59:35 PST
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
Comment 21 Ed Morley [:emorley] 2012-01-07 19:31:39 PST
https://hg.mozilla.org/mozilla-central/rev/797a60d83008

Note You need to log in before you can comment on or make changes to this bug.