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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: mccr8, Assigned: terrence)

References

Details

(Keywords: addon-compat, Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

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.
One reason to do SHRINK GC is to make decommits happen, as introduced in bug 670596.
Blocks: 670596
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.)
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]
Yeah, with the existing pointers, I should be able to take this.
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
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.
Attached patch v1: appears to work as intended. (obsolete) — Splinter Review
This is less hideous than I thought it would be.
Attachment #573388 - Flags: review?(wmccloskey)
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.
(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)
It seems unlikely that any addons manually call the GC, but I threw a flag on there anyways.
Keywords: addon-compat
Blocks: 699489
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+
Thanks for the review!
Attachment #573388 - Attachment is obsolete: true
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.