Dave Hunt [:davehunt] [he/him] ⌚BST Assignee
	Description • 13 years ago

The fix for bug 656869 requires some changes in order for the dashboard to display the results.

	Dave Hunt [:davehunt] [he/him] ⌚BST Assignee
	Comment 1 • 13 years ago

Initial patch submitted for feedback.

	u279076
	Comment 2 • 13 years ago

Comment on attachment 532785 [details] [diff] [review]
Updated dashboard to show results from Firefox 6.0. v1

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

Code-wise this patch looks fine to me -- was there some more specific feedback you were looking for?

	Dave Hunt [:davehunt] [he/him] ⌚BST Assignee
	Comment 3 • 13 years ago

Comment on attachment 532785 [details] [diff] [review]
Updated dashboard to show results from 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.

	u415141
	Comment 4 • 13 years ago

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

	Dave Hunt [:davehunt] [he/him] ⌚BST Assignee
	Comment 5 • 13 years ago

At the moment the charts display all possible metrics (with the values of 0 for any not present). I would be interested to see if you have any ideas for suppressing this data - I couldn't see a way to do it in the template... A sample report can be seen here: http://mozmill.blargon7.com/#/endurance/report/88b31ff45aa5a2c9f30fa1462d02b80d

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 6 • 13 years ago

Comment on attachment 535362 [details] [diff] [review]
Updated dashboard to show results from Firefox 6.0. v2.0

>-          value.min_allocated_memory = Math.round(min(value.allocated_memory) * BYTE_TO_MEGABYTE);
>-          value.max_allocated_memory = Math.round(max(value.allocated_memory) * BYTE_TO_MEGABYTE);
>+          memory_label = (value.stats.explicit) ? "explicit" : "allocated";
>+          value.min_memory = Math.round(value.stats[memory_label].min * BYTE_TO_MEGABYTE);
>+          value.max_memory = Math.round(value.stats[memory_label].max * BYTE_TO_MEGABYTE); 

This needs the update I mentioned on bug 650382 comment 11. Probably it doesn't apply cleanly anymore.

>-        var allocatedMemoryResults = [];
>-        var mappedMemoryResults = [];
>-        var testAverageAllocatedMemoryResults = [];
>-        var testAverageMappedMemoryResults = [];
> 
>         for (var i=0; i < testCount; i++) {
>-            var testAllocatedMemoryResults = [];
>-            var testMappedMemoryResults = [];

Those line have already been removed with the patch on bug 650382.

Can you please update the patch, so I can better review the remaining changes?

	Dave Hunt [:davehunt] [he/him] ⌚BST Assignee
	Comment 7 • 13 years ago

Updated patch with changes from trunk. Also now only shows stats if they exist.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 8 • 13 years ago

Comment on attachment 536779 [details] [diff] [review]
Updated dashboard to show results from Firefox 6.0. v3.0

>+            var testMemory = {};
>+
>+            if (stats_available) {
>+
>+              if (tests[i].stats.allocated) {
>+                testMemory.allocated = {
>+                  min : Math.round(tests[i].stats.allocated.min * BYTE_TO_MEGABYTE),
>+                  max : Math.round(tests[i].stats.allocated.max * BYTE_TO_MEGABYTE),
>+                  average : Math.round(tests[i].stats.allocated.average * BYTE_TO_MEGABYTE)
>+                }
>+              }
>+  
>+              if (tests[i].stats.mapped) {
>+                testMemory.mapped = {
>+                  min : Math.round(tests[i].stats.mapped.min * BYTE_TO_MEGABYTE),
>+                  max : Math.round(tests[i].stats.mapped.max * BYTE_TO_MEGABYTE),
>+                  average : Math.round(tests[i].stats.mapped.average * BYTE_TO_MEGABYTE)
>+                }
>+              }
>+  
>+              if (tests[i].stats.explicit) {
>+                testMemory.explicit = {
>+                  min : Math.round(tests[i].stats.explicit.min * BYTE_TO_MEGABYTE),
>+                  max : Math.round(tests[i].stats.explicit.max * BYTE_TO_MEGABYTE),
>+                  average : Math.round(tests[i].stats.explicit.average * BYTE_TO_MEGABYTE)
>+                }
>+              }
>+  
>+              if (tests[i].stats.resident) {
>+                testMemory.resident = {
>+                  min : Math.round(tests[i].stats.resident.min * BYTE_TO_MEGABYTE),
>+                  max : Math.round(tests[i].stats.resident.max * BYTE_TO_MEGABYTE),
>+                  average : Math.round(tests[i].stats.resident.average * BYTE_TO_MEGABYTE)
>+                }
>+              }
>+
>+            }
>+

Can we move this block to a separate function?

>+          if (resp.endurance.stats.allocated) {
>+            context.memory.allocated = {
>+              min : Math.round(resp.endurance.stats.allocated.min * BYTE_TO_MEGABYTE),
>+              max : Math.round(resp.endurance.stats.allocated.max * BYTE_TO_MEGABYTE),
>+              average : Math.round(resp.endurance.stats.allocated.average * BYTE_TO_MEGABYTE)
>+            }
>+          }
>+  
>+          if (resp.endurance.stats.mapped) {
>+            context.memory.mapped = {
>+              min : Math.round(resp.endurance.stats.mapped.min * BYTE_TO_MEGABYTE),
>+              max : Math.round(resp.endurance.stats.mapped.max * BYTE_TO_MEGABYTE),
>+              average : Math.round(resp.endurance.stats.mapped.average * BYTE_TO_MEGABYTE)
>+            }
>+          }
>+  
>+          if (resp.endurance.stats.explicit) {
>+            context.memory.explicit = {
>+              min : Math.round(resp.endurance.stats.explicit.min * BYTE_TO_MEGABYTE),
>+              max : Math.round(resp.endurance.stats.explicit.max * BYTE_TO_MEGABYTE),
>+              average : Math.round(resp.endurance.stats.explicit.average * BYTE_TO_MEGABYTE)
>+            }
>+          }
>+  
>+          if (resp.endurance.stats.resident) {
>+            context.memory.resident = {
>+              min : Math.round(resp.endurance.stats.resident.min * BYTE_TO_MEGABYTE),
>+              max : Math.round(resp.endurance.stats.resident.max * BYTE_TO_MEGABYTE),
>+              average : Math.round(resp.endurance.stats.resident.average * BYTE_TO_MEGABYTE)
>+            }
>+          }
>+
>+        }

... because we should get rid of this duplicated code.

>+++ b/_attachments/templates/endurance_report.mustache
>+        {{#stats_available}}
>+          {{#memory}}
>+            {{#allocated}}<tr>
>+              <th>Allocated memory (MB):</th>
>+              <td>Minimum: <strong>{{min}}</strong> / Maximum: <strong>{{max}}</strong> / Average: <strong>{{average}}</strong></td>
>+            </tr>{{/allocated}}
>+            {{#mapped}}<tr>
>+              <th>Mapped memory (MB):</th>
>+              <td>Minimum: <strong>{{min}}</strong> / Maximum: <strong>{{max}}</strong> / Average: <strong>{{average}}</strong></td>
>+            </tr>{{/mapped}}
>+            {{#explicit}}<tr>
>+              <th>Explicit memory (MB):</th>
>+              <td>Minimum: <strong>{{min}}</strong> / Maximum: <strong>{{max}}</strong> / Average: <strong>{{average}}</strong></td>
>+            </tr>{{/explicit}}
>+            {{#resident}}<tr>
>+              <th>Resident memory (MB):</th>
>+              <td>Minimum: <strong>{{min}}</strong> / Maximum: <strong>{{max}}</strong> / Average: <strong>{{average}}</strong></td>
>+            </tr>{{/resident}}
>+          {{/memory}}
>+        {{/stats_available}}

Please file a new bug for hiding the non-existent stats. Not sure yet, what's the best way to not show all metrics.

Otherwise looks good.

	Dave Hunt [:davehunt] [he/him] ⌚BST Assignee
	Comment 9 • 13 years ago

(In reply to comment #8)
> Comment on attachment 536779 [details] [diff] [review] [review]
> Updated dashboard to show results from Firefox 6.0. v3.0
> 
> >+            var testMemory = {};
> >+
> >+            if (stats_available) {
> >+
> >+              if (tests[i].stats.allocated) {
> >+                testMemory.allocated = {
> >+                  min : Math.round(tests[i].stats.allocated.min * BYTE_TO_MEGABYTE),
> >+                  max : Math.round(tests[i].stats.allocated.max * BYTE_TO_MEGABYTE),
> >+                  average : Math.round(tests[i].stats.allocated.average * BYTE_TO_MEGABYTE)
> >+                }
> >+              }
> >+  
> >+              if (tests[i].stats.mapped) {
> >+                testMemory.mapped = {
> >+                  min : Math.round(tests[i].stats.mapped.min * BYTE_TO_MEGABYTE),
> >+                  max : Math.round(tests[i].stats.mapped.max * BYTE_TO_MEGABYTE),
> >+                  average : Math.round(tests[i].stats.mapped.average * BYTE_TO_MEGABYTE)
> >+                }
> >+              }
> >+  
> >+              if (tests[i].stats.explicit) {
> >+                testMemory.explicit = {
> >+                  min : Math.round(tests[i].stats.explicit.min * BYTE_TO_MEGABYTE),
> >+                  max : Math.round(tests[i].stats.explicit.max * BYTE_TO_MEGABYTE),
> >+                  average : Math.round(tests[i].stats.explicit.average * BYTE_TO_MEGABYTE)
> >+                }
> >+              }
> >+  
> >+              if (tests[i].stats.resident) {
> >+                testMemory.resident = {
> >+                  min : Math.round(tests[i].stats.resident.min * BYTE_TO_MEGABYTE),
> >+                  max : Math.round(tests[i].stats.resident.max * BYTE_TO_MEGABYTE),
> >+                  average : Math.round(tests[i].stats.resident.average * BYTE_TO_MEGABYTE)
> >+                }
> >+              }
> >+
> >+            }
> >+
> 
> Can we move this block to a separate function?

Done.

> 
> >+++ b/_attachments/templates/endurance_report.mustache
> >+        {{#stats_available}}
> >+          {{#memory}}
> >+            {{#allocated}}<tr>
> >+              <th>Allocated memory (MB):</th>
> >+              <td>Minimum: <strong>{{min}}</strong> / Maximum: <strong>{{max}}</strong> / Average: <strong>{{average}}</strong></td>
> >+            </tr>{{/allocated}}
> >+            {{#mapped}}<tr>
> >+              <th>Mapped memory (MB):</th>
> >+              <td>Minimum: <strong>{{min}}</strong> / Maximum: <strong>{{max}}</strong> / Average: <strong>{{average}}</strong></td>
> >+            </tr>{{/mapped}}
> >+            {{#explicit}}<tr>
> >+              <th>Explicit memory (MB):</th>
> >+              <td>Minimum: <strong>{{min}}</strong> / Maximum: <strong>{{max}}</strong> / Average: <strong>{{average}}</strong></td>
> >+            </tr>{{/explicit}}
> >+            {{#resident}}<tr>
> >+              <th>Resident memory (MB):</th>
> >+              <td>Minimum: <strong>{{min}}</strong> / Maximum: <strong>{{max}}</strong> / Average: <strong>{{average}}</strong></td>
> >+            </tr>{{/resident}}
> >+          {{/memory}}
> >+        {{/stats_available}}
> 
> Please file a new bug for hiding the non-existent stats. Not sure yet,
> what's the best way to not show all metrics.

Done. See bug 661495

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 10 • 13 years ago

Comment on attachment 536854 [details] [diff] [review]
Updated dashboard to show results from Firefox 6.0. v3.1

Looks good. Please land it and mark as to be pushed. I will do that later.

	Dave Hunt [:davehunt] [he/him] ⌚BST Assignee
	Comment 11 • 13 years ago

Landed as:
https://github.com/whimboo/mozmill-dashboard/commit/22bb3e13795aa8829cbf2b2111ccdf82dbf9403b

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 12 • 13 years ago

Has been pushed to production.