Closed Bug 656869 Opened 8 years ago Closed 8 years ago

No memory results on endurance testrun with Nightly 6.0a1

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, major)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Lefty, Assigned: davehunt)

References

()

Details

(Whiteboard: [endurance-tests])

Attachments

(2 files, 6 obsolete files)

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
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.
Severity: normal → major
Status: UNCONFIRMED → NEW
Component: Mozmill Result Dashboard → Mozmill Tests
Ever confirmed: true
OS: Windows 7 → All
QA Contact: mozmill-result-dashboard → mozmill-tests
Hardware: x86_64 → All
Whiteboard: [endurance-tests]
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/13431087
Initial patch submitted for feedback.
Assignee: nobody → dave.hunt
Attachment #532781 - Flags: feedback?(anthony.s.hughes)
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?
Attachment #532781 - Flags: feedback?(anthony.s.hughes) → feedback+
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.
Attachment #532781 - Flags: review?(hskupin)
This patch looks right, but bug 657327 is going to invalidate it again :/
Depends on: 657327
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.
Attachment #532781 - Flags: review?(hskupin) → review-
(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.
Attachment #532781 - Attachment is obsolete: true
Attachment #533648 - Flags: review?(hskupin)
Fixed exports.
Attachment #533648 - Attachment is obsolete: true
Attachment #533648 - Flags: review?(hskupin)
Attachment #533655 - Flags: review?(hskupin)
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.
Attachment #533655 - Flags: review?(hskupin) → review+
(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.
Attachment #533655 - Attachment is obsolete: true
Attachment #533972 - Flags: review?(hskupin)
(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.
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 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.
Attachment #533972 - Flags: review?(hskupin)
This patch gathers the explicit and resident values as reported by Firefox 6.0.
Attachment #533972 - Attachment is obsolete: true
Attachment #535358 - Flags: review?(hskupin)
Attachment #535358 - Flags: feedback?
Attachment #535358 - Flags: feedback? → feedback?(nnethercote)
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.
Attachment #535358 - Flags: feedback?(nnethercote) → feedback+
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.
(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.
I've raised bug 661245 to deal with errors occurred when gathering metrics in endurance tests.
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.
Attachment #535358 - Flags: review?(hskupin) → review-
(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.
Attachment #535358 - Attachment is obsolete: true
Attachment #536786 - Flags: review?(hskupin)
Attachment #536786 - Flags: feedback?(nnethercote)
I just noticed that I forgot to remove the mapped used/unused from a previous version of the patch.
Attachment #536786 - Attachment is obsolete: true
Attachment #536786 - Flags: review?(hskupin)
Attachment #536786 - Flags: feedback?(nnethercote)
Attachment #536788 - Flags: review?(hskupin)
Attachment #536788 - Flags: feedback?(nnethercote)
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]:
-----------------------------------------------------------------
Attachment #536788 - Flags: feedback?(nnethercote) → feedback+
Nicholas, could you please comment on my question from comment 21? Thanks.
(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.
(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.
Status: NEW → ASSIGNED
Depends on: 660731
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.
Attachment #536788 - Flags: review?(hskupin) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.