Last Comment Bug 699279 - On memory pressure, JS should do a SHRINK GC
: On memory pressure, JS should do a SHRINK GC
Status: RESOLVED FIXED
[MemShrink:P2]
: addon-compat
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla11
Assigned To: Terrence Cole [:terrence]
:
:
Mentors:
: 700291 703067 (view as bug list)
Depends on:
Blocks: 670596 699489
  Show dependency treegraph
 
Reported: 2011-11-02 16:31 PDT by Andrew McCreight [:mccr8]
Modified: 2011-11-29 04:56 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1: appears to work as intended. (8.14 KB, patch)
2011-11-09 17:31 PST, Terrence Cole [:terrence]
mrbkap: review+
Details | Diff | Splinter Review
v3: With review feedback. (8.68 KB, patch)
2011-11-28 13:14 PST, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2011-11-02 16:31:40 PDT
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.
Comment 1 Andrew McCreight [:mccr8] 2011-11-07 09:44:41 PST
*** Bug 700291 has been marked as a duplicate of this bug. ***
Comment 2 Andrew McCreight [:mccr8] 2011-11-07 09:46:36 PST
One reason to do SHRINK GC is to make decommits happen, as introduced in bug 670596.
Comment 3 Justin Lebar (not reading bugmail) 2011-11-07 10:58:39 PST
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 Nicholas Nethercote [:njn] 2011-11-08 14:25:50 PST
Terrence, can you take this?  It shouldn't be hard, just a bit of plumbing through xpconnect, as per comment 0.
Comment 5 Terrence Cole [:terrence] 2011-11-08 14:51:54 PST
Yeah, with the existing pointers, I should be able to take this.
Comment 6 Terrence Cole [:terrence] 2011-11-08 15:41:21 PST
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?
Comment 7 Nicholas Nethercote [:njn] 2011-11-08 15:59:52 PST
Sounds reasonable to me.
Comment 8 Bill McCloskey (:billm) 2011-11-08 16:09:44 PST
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.
Comment 9 Terrence Cole [:terrence] 2011-11-09 17:31:55 PST
Created attachment 573388 [details] [diff] [review]
v1: appears to work as intended.

This is less hideous than I thought it would be.
Comment 10 Andrew McCreight [:mccr8] 2011-11-09 17:43:00 PST
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.
Comment 11 Terrence Cole [:terrence] 2011-11-09 18:36:11 PST
(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 12 Bill McCloskey (:billm) 2011-11-10 08:16:32 PST
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?
Comment 13 Andrew McCreight [:mccr8] 2011-11-10 08:24:05 PST
It seems unlikely that any addons manually call the GC, but I threw a flag on there anyways.
Comment 14 Justin Lebar (not reading bugmail) 2011-11-16 13:32:01 PST
*** Bug 703067 has been marked as a duplicate of this bug. ***
Comment 15 Blake Kaplan (:mrbkap) 2011-11-25 07:22:50 PST
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.
Comment 16 Terrence Cole [:terrence] 2011-11-28 13:14:22 PST
Created attachment 577344 [details] [diff] [review]
v3: With review feedback.

Thanks for the review!
Comment 17 Terrence Cole [:terrence] 2011-11-28 13:16:34 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/5a2fc54f90a4
Comment 18 Marco Bonardo [::mak] 2011-11-29 04:56:43 PST
https://hg.mozilla.org/mozilla-central/rev/5a2fc54f90a4

Note You need to log in before you can comment on or make changes to this bug.