Closed
Bug 699279
Opened 13 years ago
Closed 13 years ago
On memory pressure, JS should do a SHRINK GC
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: mccr8, Assigned: terrence)
References
Details
(Keywords: addon-compat, Whiteboard: [MemShrink:P2])
Attachments
(1 file, 1 obsolete file)
8.68 KB,
patch
|
Details | Diff | Splinter Review |
A shrink GC attempts to get rid of things more aggressively than a normal GC, so maybe we should do that in nsMemoryPressureObserver::Observe instead of a normal one. One goal of this is to get about:memory to dump more memory when the buttons are clicked. I'm not really sure of the right component for this. I think this will require exposing an additional JS API kind of thing, then modifying XPConnect, nsJSEnvironment and nsMemoryPressureObserver to also add in this additional hook, assuming the current convoluted path the call follows is the right way.
Reporter | ||
Comment 2•13 years ago
|
||
One reason to do SHRINK GC is to make decommits happen, as introduced in bug 670596.
Blocks: 670596
Comment 3•13 years ago
|
||
Another goal is to get Fennec to dump as much memory as possible when it's sent into the background. (Ironically enough, we currently have code not to trigger a GC on memory pressure when we're Fennec, because Fennec does its own memory pressure watching. But once the plumbing is there, we can get Fennec to do a shrink gc.)
Comment 4•13 years ago
|
||
Terrence, can you take this? It shouldn't be hard, just a bit of plumbing through xpconnect, as per comment 0.
Assignee: general → terrence
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 5•13 years ago
|
||
Yeah, with the existing pointers, I should be able to take this.
Assignee | ||
Comment 6•13 years ago
|
||
The call stack between the observer and the GC appears to be something like. -- in dom/base/nsJSEnvironment.cpp 1) nsMemoryPressureObserver::Observe 2) nsJSContext::GarbageCollectNow 3) nsContentUtils::XPConnect()->GarbageCollect -- in js/xpconnect/src/nsXPConnect.cpp 4) nsXPConnect::GarbageCollect 5) nsXPConnect::Collect -- in jsapi.cpp 6) JS_GC 7) JS_CompartmentGC -- in jsgc.cpp 8) js_GC 9) ... 10) profit All of these files appear to have access to jsgc.h (and jsprvtd.h too for that matter), so a lazier me could just use JSGCInvocationKind in all of these signatures right now and be done with it. I think what we probably want to do is either move JSGCInvocationKind to a public header or create a new public invocation kind, if the internal one has fields that shouldn't be exposed to the full browser. What do you think?
Status: NEW → ASSIGNED
Comment 7•13 years ago
|
||
Sounds reasonable to me.
The bottleneck here is JS_GC. We probably don't want to add a new parameter because it would break a lot of callers in Firefox and elsewhere. We can't add default parameters because jsapi.h is C-only. I would suggest adding a JS_ShrinkingGC function to jsfriendapi. Then you could add an extra bool parameter to the other ones above it.
Assignee | ||
Comment 9•13 years ago
|
||
This is less hideous than I thought it would be.
Attachment #573388 -
Flags: review?(wmccloskey)
Reporter | ||
Comment 10•13 years ago
|
||
Ah, yes, that's not too bad. You need to change the uuid thing when you change an idl. Seems a little weird to add a parameter to nsCycleCollectionJSRuntime we never really use, but I suppose the alternative is splitting off a new Collect interface in XPConnect, depending on whether you want to shrink or not which is probably gross.
Assignee | ||
Comment 11•13 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #10) > Ah, yes, that's not too bad. You need to change the uuid thing when you > change an idl. Seems a little weird to add a parameter to > nsCycleCollectionJSRuntime we never really use, but I suppose the > alternative is splitting off a new Collect interface in XPConnect, depending > on whether you want to shrink or not which is probably gross. Thanks for the feedback! I will update the uuid and start a try run when try is online again.
Comment on attachment 573388 [details] [diff] [review] v1: appears to work as intended. Looking this over, I'm probably not the best person to review. Do we need to worry about add-on compatibility since we're changing IDL?
Attachment #573388 -
Flags: review?(wmccloskey) → review?(mrbkap)
Reporter | ||
Comment 13•13 years ago
|
||
It seems unlikely that any addons manually call the GC, but I threw a flag on there anyways.
Keywords: addon-compat
Comment 15•13 years ago
|
||
Comment on attachment 573388 [details] [diff] [review] v1: appears to work as intended. Review of attachment 573388 [details] [diff] [review]: ----------------------------------------------------------------- r=me with some nits picked. ::: dom/base/nsJSEnvironment.h @@ +182,5 @@ > > static void LoadStart(); > static void LoadEnd(); > > + static void GarbageCollectNow(bool shrinkingGC=false); Uber-nit: spaces around the = sign please. ::: xpcom/base/nsCycleCollector.h @@ +91,5 @@ > > /** > * Runs the JavaScript GC. > */ > + virtual void Collect(bool shrinkingGC=false) = 0; Same uber-nit about spaces around the equals sign here.
Attachment #573388 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 16•13 years ago
|
||
Thanks for the review!
Attachment #573388 -
Attachment is obsolete: true
Assignee | ||
Comment 17•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/5a2fc54f90a4
Comment 18•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5a2fc54f90a4
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in
before you can comment on or make changes to this bug.
Description
•