1125412 - Graph memory statistics in the GC ubench

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

In addition to the current end user-visible latency chart, I want to graph gcBytes and the limits and triggers and stuff.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Expose an object for inspecting GC memory values

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.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Draw a graph of memory usage

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.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Add a pref-gated performance.mozMemory for accessing internal memory usage/threshold info

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.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Expose an object for inspecting GC memory values

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.

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Draw a graph of memory usage

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.

Comment 6

[Terrence Cole [:terrence]]

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.

Comment 7

[Terrence Cole [:terrence]]

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 9

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f6de7b18305
https://hg.mozilla.org/integration/mozilla-inbound/rev/26c1da818a1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ff2f017b17

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f6de7b18305
https://hg.mozilla.org/mozilla-central/rev/26c1da818a1e
https://hg.mozilla.org/mozilla-central/rev/c2ff2f017b17