656869 - No memory results on endurance testrun with Nightly 6.0a1

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Jarkko Seppälä [:Lefty]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110512 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110512 Firefox/6.0a1

After these bugs landed, endurance test results don't have memory usage information.

about:memory (bug 648490, bug 653630, bug 654041, bug 655638, bug 655583)

Reproducible: Always


Actual Results:  
http://mozmill-crowd.brasstacks.mozilla.com/#/endurance/report/4da4bc3ca7e3973156fe1173a781cb7c

http://mozmill-crowd.brasstacks.mozilla.com/#/endurance/report/4da4bc3ca7e3973156fe1173a749e936

Expected Results:  
http://mozmill-crowd.brasstacks.mozilla.com/#/endurance/report/4da4bc3ca7e3973156fe1173a723fd63

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Due to some back-end changes in Nightly builds we are probably no longer able to retrieve any memory information. Dave, we should fix it as soon as possible.

Comment 2

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/13431087

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add memory paths to fetch for Firefox 6.0. v1

Initial patch submitted for feedback.

Comment 4

[u279076]

Comment on attachment 532781 [details] [diff] [review]
Add memory paths to fetch for Firefox 6.0. v1

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

Code-wise this looks fine -- was there anything specific you wanted me to provide feedback on?

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 532781 [details] [diff] [review]
Add memory paths to fetch for Firefox 6.0. v1

Thanks. I just wanted to gain some confidence before submitting for review. If you didn't see anything obviously bad then that's great.

Comment 6

[Nicholas Nethercote [inactive]]

This patch looks right, but bug 657327 is going to invalidate it again :/

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 532781 [details] [diff] [review]
Add memory paths to fetch for Firefox 6.0. v1

>+    if (result.mapped_used) {
>+      resultString = result.timestamp.toUTCString() + " | | | " +
>+                     result.mapped_heap + " | " +
>+                     result.mapped_heap_used + " | " +
>+                     result.mapped_heap_unused + " | " +
>+                     result.label + "\n";
>+    } else {
>+      resultString = result.timestamp.toUTCString() + " | " +
>+                     result.allocated + " | " +
>+                     result.mapped + " | | | | " +
>+                     result.label + "\n";
>+    }

There is no need to differentiate the reporters here. This will for now only land on the default branch, where the old reporters are not available anymore.

> // Exported constants
>+exports.PATH_ALLOCATED = PATH_ALLOCATED;
> exports.PATH_MAPPED = PATH_MAPPED;
>-exports.PATH_ALLOCATED = PATH_ALLOCATED;
>+exports.PATH_MAPPED_HEAP_USED = PATH_MAPPED_HEAP_USED;
>+exports.PATH_MAPPED_HEAP_UNUSED = PATH_MAPPED_HEAP_UNUSED;
> exports.MEMORY_UNAVAILABLE = MEMORY_UNAVAILABLE;

We should define it as an enum, to only have to export it once.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add memory paths to fetch for Firefox 6.0. v1.2

(In reply to comment #6)
> This patch looks right, but bug 657327 is going to invalidate it again :/

Thanks Nick, I'm closely following that bug and will update endurance tests again as/when it lands.

(In reply to comment #7)
> Comment on attachment 532781 [details] [diff] [review] [review]
> Add memory paths to fetch for Firefox 6.0. v1
> 
> >+    if (result.mapped_used) {
> >+      resultString = result.timestamp.toUTCString() + " | | | " +
> >+                     result.mapped_heap + " | " +
> >+                     result.mapped_heap_used + " | " +
> >+                     result.mapped_heap_unused + " | " +
> >+                     result.label + "\n";
> >+    } else {
> >+      resultString = result.timestamp.toUTCString() + " | " +
> >+                     result.allocated + " | " +
> >+                     result.mapped + " | | | | " +
> >+                     result.label + "\n";
> >+    }
> 
> There is no need to differentiate the reporters here. This will for now only
> land on the default branch, where the old reporters are not available
> anymore.

I've updated to make this specific to the default branch.

> > // Exported constants
> >+exports.PATH_ALLOCATED = PATH_ALLOCATED;
> > exports.PATH_MAPPED = PATH_MAPPED;
> >-exports.PATH_ALLOCATED = PATH_ALLOCATED;
> >+exports.PATH_MAPPED_HEAP_USED = PATH_MAPPED_HEAP_USED;
> >+exports.PATH_MAPPED_HEAP_UNUSED = PATH_MAPPED_HEAP_UNUSED;
> > exports.MEMORY_UNAVAILABLE = MEMORY_UNAVAILABLE;
> 
> We should define it as an enum, to only have to export it once.

Done. Let me know if this is what you were thinking.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add memory paths to fetch for Firefox 6.0. v1.3

Fixed exports.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 533655 [details] [diff] [review]
Add memory paths to fetch for Firefox 6.0. v1.3

>     var result = {
>-      label:     aLabel,
>-      timestamp: new Date(),
>-      mapped:    this.getMemory(PATH_MAPPED),
>-      allocated: this.getMemory(PATH_ALLOCATED)
>-    };
>+      label : aLabel,
>+      timestamp : new Date()
>+    }
>+    var mapped_heap_used = this.getMemory(MEMORY_REPORTERS.MAPPED_HEAP_USED);
>+    var mapped_heap_unused = this.getMemory(MEMORY_REPORTERS.MAPPED_HEAP_UNUSED);
>+    result.mapped_heap_used = mapped_heap_used;
>+    result.mapped_heap_unused = mapped_heap_unused;
>+    result.mapped_heap = (mapped_heap_used + mapped_heap_unused);

Move the declaration and initialization of the temporary variables to the top of the function. That way you can populate the result object in one step without having to use 'result.XYZ'.

Otherwise looks good.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add memory paths to fetch for Firefox 6.0. v1.4

(In reply to comment #10)
> Comment on attachment 533655 [details] [diff] [review] [review]
> Add memory paths to fetch for Firefox 6.0. v1.3
> 
> >     var result = {
> >-      label:     aLabel,
> >-      timestamp: new Date(),
> >-      mapped:    this.getMemory(PATH_MAPPED),
> >-      allocated: this.getMemory(PATH_ALLOCATED)
> >-    };
> >+      label : aLabel,
> >+      timestamp : new Date()
> >+    }
> >+    var mapped_heap_used = this.getMemory(MEMORY_REPORTERS.MAPPED_HEAP_USED);
> >+    var mapped_heap_unused = this.getMemory(MEMORY_REPORTERS.MAPPED_HEAP_UNUSED);
> >+    result.mapped_heap_used = mapped_heap_used;
> >+    result.mapped_heap_unused = mapped_heap_unused;
> >+    result.mapped_heap = (mapped_heap_used + mapped_heap_unused);
> 
> Move the declaration and initialization of the temporary variables to the
> top of the function. That way you can populate the result object in one step
> without having to use 'result.XYZ'.

Done.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: JS code to find "explicit"

(In reply to comment #8)
> > This patch looks right, but bug 657327 is going to invalidate it again :/
> 
> Thanks Nick, I'm closely following that bug and will update endurance tests
> again as/when it lands.

It has landed.

Attached is a JS function to compute the new "explicit" value.  I tested it a bit against about:memory and it seems to work, though you should do some testing yourself just to be sure.

Comment 13

[Nicholas Nethercote [inactive]]

Oh, I should add that this'll work with the desktop browser where everything is in a single process.  For Fennec -- where the chrome and content processes are separate -- the reporters for the content process have something like "Content (12345): " prefixed to the reporter paths.  But that'll change with bug 656773!

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 533972 [details] [diff] [review]
Add memory paths to fetch for Firefox 6.0. v1.4

Then I will wait for an updated patch by Dave, so we can skip the interim cycle of the memory reporter paths.

>   addCheckpoint : function PerfTracer_addCheckpoint(aLabel) {
>+    var mapped_heap_used = this.getMemory(MEMORY_REPORTERS.MAPPED_HEAP_USED);
>+    var mapped_heap_unused = this.getMemory(MEMORY_REPORTERS.MAPPED_HEAP_UNUSED);
>     var result = {
>-      label:     aLabel,
>-      timestamp: new Date(),
>-      mapped:    this.getMemory(PATH_MAPPED),
>-      allocated: this.getMemory(PATH_ALLOCATED)

For the future we should move out all memory related methods to their own class and simply get the properties here. It can be raise confusion once we start to measure other types of dynamic information. Probably the class should also be renamed to 'Tracer', there is nothing about performance we measure right now. CC'ing Geo to also get his input.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add memory paths to fetch for Firefox 6.0. v2.0

This patch gathers the explicit and resident values as reported by Firefox 6.0.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

An example report can be seen here: http://mozmill.blargon7.com/#/endurance/report/88b31ff45aa5a2c9f30fa1462d02b80d

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 535358 [details] [diff] [review]
Add memory paths to fetch for Firefox 6.0. v2.0

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

Looks ok but see the comment below.

::: lib/performance.js
@@ +161,5 @@
> +          memoryUsed: r.memoryUsed
> +        }
> +        a.push(r2);
> +      } else if (r.path === MEMORY_REPORTERS.HEAP_USED) {
> +        heapUsed = r.memoryUsed;

One thing I realized is that heap-used can be -1 if something goes wrong.  For example, if you're on Linux or Windows and you build with --disable-jemalloc.  This code doesn't allow for that.  In that case the "explicit" result is going to be completely unreliable so you should probably just give up trying to report it.

Similarly, any of the MAPPED reporters could return -1, though none of them currently do, IIRC.

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Nick: I'm a little bit unclear. Are you suggesting that if heapUsed has a value of -1 that we should not attempt to compute an explicit value and just exit the function early? I'm fine with that, although we should probably log those occurrences somewhere for the report.

Comment 19

[Nicholas Nethercote [inactive]]

(In reply to comment #18)
> Nick: I'm a little bit unclear. Are you suggesting that if heapUsed has a
> value of -1 that we should not attempt to compute an explicit value and just
> exit the function early?

That's right.  Note that this will only happen on certain configurations, eg. Windows/Linux with --disable-jemalloc, or non Windows/Linux/Mac platforms like Solaris.  I.e. it's a statically-determined thing.

> I'm fine with that, although we should probably log
> those occurrences somewhere for the report.

Sounds reasonable.

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I've raised bug 661245 to deal with errors occurred when gathering metrics in endurance tests.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 535358 [details] [diff] [review]
Add memory paths to fetch for Firefox 6.0. v2.0

>+++ b/lib/performance.js
[..]
>- * Portions created by the Initial Developer are Copyright (C) 2010
>+ * Portions created by the Initial Developer are Copyright (C) 2011

Please keep the original date.

>+// Paths for memory reporters. Use as keys to access the appropriate metrics.
>+const MEMORY_REPORTERS = {
>+    HEAP_USED : "heap-used",
>+    RESIDENT : "resident"
>+}

Please add a comment to each path so we know for what it is used for.

>+  /**
>+   * Sum all MAPPED and HEAP ones that aren't children of other MAPPED/HEAP ones
>+   *
>+   * @returns Explicit allocated memory
>+   * @type {integer}
>+   */
>+  _computeExplicit : function PerfTracer_computeExplicit() {

I wonder why an excessive function like this is not part of the memory reporter or any other low-level interface. Nicholas, could this be an option? 

>+    var mgr = Cc["@mozilla.org/memory-reporter-manager;1"].
>+        getService(Ci.nsIMemoryReporterManager);

nit: fix the indentation

>+      if (r.kind === MR_MAPPED) {
>+        var r2 = {
>+          path:       r.path,
>+          memoryUsed: r.memoryUsed
>+        }
>+        a.push(r2);

Why don't we directly push r into the array?

>+      } else if (r.path === MEMORY_REPORTERS.HEAP_USED) {
>+        heapUsed = r.memoryUsed;
>+      }
[..]
>+        if (r1.path === r2.path) {
>+          // Duplicates;  include both, we'll sum them.
>+        } else if (isPrefix(r1.path, r2.path)) {
>+          // r1 is a parent of r2, zero r2.
>+          r2.memoryUsed = 0;
>+        } else if (isPrefix(r2.path, r1.path)) {
>+          // r2 is a parent of r1, zero r1.
>+          r1.memoryUsed = 0;
>+        }

Put else into the next line.

>+        if (r1.path === r2.path) {
>+          // Duplicates;  include both, we'll sum them.

Why do we have to sum them up? It's not clear for me given by this comment.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add memory paths to fetch for Firefox 6.0. v2.1

(In reply to comment #21)
> Comment on attachment 535358 [details] [diff] [review] [review]
> Add memory paths to fetch for Firefox 6.0. v2.0
> 
> >+++ b/lib/performance.js
> [..]
> >- * Portions created by the Initial Developer are Copyright (C) 2010
> >+ * Portions created by the Initial Developer are Copyright (C) 2011
> 
> Please keep the original date.

Done.

> >+// Paths for memory reporters. Use as keys to access the appropriate metrics.
> >+const MEMORY_REPORTERS = {
> >+    HEAP_USED : "heap-used",
> >+    RESIDENT : "resident"
> >+}
> 
> Please add a comment to each path so we know for what it is used for.

I don't understand what's unclear here? One returns the heap used, and the other returns the resident memory. I think the names are suitable enough to not need additional comments.

> >+    var mgr = Cc["@mozilla.org/memory-reporter-manager;1"].
> >+        getService(Ci.nsIMemoryReporterManager);
> 
> nit: fix the indentation

Done.

> >+      } else if (r.path === MEMORY_REPORTERS.HEAP_USED) {
> >+        heapUsed = r.memoryUsed;
> >+      }
> [..]
> >+        if (r1.path === r2.path) {
> >+          // Duplicates;  include both, we'll sum them.
> >+        } else if (isPrefix(r1.path, r2.path)) {
> >+          // r1 is a parent of r2, zero r2.
> >+          r2.memoryUsed = 0;
> >+        } else if (isPrefix(r2.path, r1.path)) {
> >+          // r2 is a parent of r1, zero r1.
> >+          r1.memoryUsed = 0;
> >+        }
> 
> Put else into the next line.

Done.

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add memory paths to fetch for Firefox 6.0. v2.2

I just noticed that I forgot to remove the mapped used/unused from a previous version of the patch.

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 536788 [details] [diff] [review]
Add memory paths to fetch for Firefox 6.0. v2.2

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

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Nicholas, could you please comment on my question from comment 21? Thanks.

Comment 26

[Nicholas Nethercote [inactive]]

(In reply to comment #21)
> 
> I wonder why an excessive function like this is not part of the memory
> reporter or any other low-level interface. Nicholas, could this be an
> option? 

See bug 660731 :)

> nit: fix the indentation
> 
> >+      if (r.kind === MR_MAPPED) {
> >+        var r2 = {
> >+          path:       r.path,
> >+          memoryUsed: r.memoryUsed
> >+        }
> >+        a.push(r2);
> 
> Why don't we directly push r into the array?

r.memoryUsed is a read-only property, and we might need to modify it later, so we make a copy.  I should have added a comment about this.

> >+        if (r1.path === r2.path) {
> >+          // Duplicates;  include both, we'll sum them.
> 
> Why do we have to sum them up? It's not clear for me given by this comment.

You can have duplicate reporters and summing them is the right thing to do.

Anyway, bug 660731 will render most of this code moot.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #26)
> > I wonder why an excessive function like this is not part of the memory
> > reporter or any other low-level interface. Nicholas, could this be an
> > option? 
> 
> See bug 660731 :)

That sounds great and answers all my questions. :) I will put it onto the dependency tree so we can track it and watch for changes on our side. Thanks for it.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 536788 [details] [diff] [review]
Add memory paths to fetch for Firefox 6.0. v2.2

Looks good. Dave, please land it on default and aurora.

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/fdc3f7774422 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/7b0af9a1b4c8 (mozilla-aurora)