Graph memory statistics in the GC ubench

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

In addition to the current end user-visible latency chart, I want to graph gcBytes and the limits and triggers and stuff.
A wrong and horrible way to expose these metrics. The worst part about it is that it creates new objects every frame, perturbing what we're measuring. But it was enought to start work on the display.

I'll have to talk to people for what the right way to do this is. This mostly came out of the idea that it would be cool to only require a pref change instead of installing an addon. But perhaps it's still problematic to allow this to escape to release. And if I'm really going to stick this on the (specced) performance object, which is almost certainly a bad idea, then I'd want this to be a proper wrapped thingamabobber that smells like similar objects and can take expandos or whatever.

Or maybe I should revert to exposing gcparam(), which is our current mechanism for this stuff. Too many decisions; for now I'll just forge ahead with my known broken mechanism.
Posted patch Draw a graph of memory usage (obsolete) — Splinter Review
Rewrite the graph display to be flexible wrt size and use prototypal inheritance and things so I can split off a separate graph to display memory (just gcBytes for now, later to add in malloc bytes on the same graph with a different line.) This already exposes plenty of weirdness in behavior, some of which is probably because I'm showing the wrong triggers etc. It's fun to watch, though.

Note that this still isn't directly showing GCs. I plan to use the existing mechanisms for that.
Olli, I need to hang this data off of *somewhere*. I really don't know where the right place is. I don't know if webidl should only be used for specced stuff, which this isn't and should never be. Let me know what the rules are.
Attachment #8580123 - Flags: review?(bugs)
Attachment #8580123 - Flags: superreview?(bugs)
Well, this seems vaguely sane now, though I strongly suspect that it isn't completely right (from staring at the graph; the trigger thresholds don't seem to match actual behavior very well.) But it doesn't seem like an awful thing to land.

The big open question is whether this should exist at all. The shell already has gcparam(), after all, and had I known about it in the first place I might never have written this.
Attachment #8580370 - Flags: review?(terrence)
Attachment #8554025 - Attachment is obsolete: true
May as well get this landed too, though I suspect the other patches with the underpinnings are going to need to be rewritten with different names, at least.
Attachment #8580372 - Flags: review?(terrence)
Attachment #8554028 - Attachment is obsolete: true
Comment on attachment 8580370 [details] [diff] [review]
Expose an object for inspecting GC memory values

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

Maybe we should make this nightly-only as well as behind a pref.

::: js/src/jsapi.cpp
@@ +5933,5 @@
> +JS_PUBLIC_API(JSObject *)
> +JS::NewMemoryInfoObject(JSContext *cx)
> +{
> +    return js::gc::NewMemoryStatisticsObject(cx);
> +}

I think we should just expose this directly in HeapAPI.h, in the js::gc namespace.
Attachment #8580370 - Flags: review?(terrence) → review+
Comment on attachment 8580372 [details] [diff] [review]
Draw a graph of memory usage

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

Very nice!

::: js/src/devtools/gc-ubench/harness.js
@@ +26,5 @@
>  // Global defaults
>  var globalDefaultGarbageTotal = "8M";
>  var globalDefaultGarbagePerFrame = "8K";
>  
> +function Graph(ctx) {

In theory, classes have landed -- since this is nightly only for the moment anyway....

@@ +32,3 @@
>  }
>  
> +Graph.prototype.xpos = (index) => { return index * 2 };

Please use the => syntax throughout. Also, for a single expression you can dump most of the extra syntax: e.g. This can be as short as:

Graph.prototype.xpos = index => index * 2;

@@ +138,5 @@
>      ctx.stroke();
>  
>      ctx.fillStyle = 'rgb(255,0,0)';
>      if (worst)
> +        ctx.fillText(''+worst.toFixed(2)+'ms', this.xpos(worstpos) - 10, this.ypos(worst) - 14);

Please use a format string here: `${worst.toFixed(2)}ms`

@@ +143,4 @@
>  
>      ctx.beginPath();
>      var where = sampleIndex % numSamples;
> +    ctx.arc(this.xpos(where), this.ypos(delays[where]), 5, 0, Math.PI*2, true);

Why do we not have a Math.TAU constant? The mind boggles.
Attachment #8580372 - Flags: review?(terrence) → review+
Comment on attachment 8580123 [details] [diff] [review]
Add a pref-gated performance.mozMemory for accessing internal memory usage/threshold info

I thought JS::Heap<JSObject*> doesn't need to be initialize explicitly to null in ctor.


Please use Mozilla coding style for argument names.
So, JSContext* aCx, JS::MutableHandle<JSObject*> aObj
Attachment #8580123 - Flags: superreview?(bugs)
Attachment #8580123 - Flags: superreview+
Attachment #8580123 - Flags: review?(bugs)
Attachment #8580123 - Flags: review+
Depends on: 1149739
Depends on: 1160884
No longer depends on: 1160884
You need to log in before you can comment on or make changes to this bug.