Closed
Bug 1125412
Opened 9 years ago
Closed 9 years ago
Graph memory statistics in the GC ubench
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(3 files, 2 obsolete files)
8.48 KB,
patch
|
smaug
:
review+
smaug
:
superreview+
|
Details | Diff | Splinter Review |
13.60 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
13.85 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
In addition to the current end user-visible latency chart, I want to graph gcBytes and the limits and triggers and stuff.
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8580123 -
Flags: superreview?(bugs)
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8554025 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8554028 -
Attachment is obsolete: true
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2f6de7b18305 https://hg.mozilla.org/mozilla-central/rev/26c1da818a1e https://hg.mozilla.org/mozilla-central/rev/c2ff2f017b17
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•