On memory pressure, JS should do a SHRINK GC

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: terrence)

Tracking

({addon-compat})

Trunk
mozilla11
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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)

Updated

6 years ago
Duplicate of this bug: 700291
(Reporter)

Comment 2

6 years ago
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]
(Assignee)

Comment 5

6 years ago
Yeah, with the existing pointers, I should be able to take this.
(Assignee)

Comment 6

6 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
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

6 years ago
Created attachment 573388 [details] [diff] [review]
v1: appears to work as intended.

This is less hideous than I thought it would be.
Attachment #573388 - Flags: review?(wmccloskey)
(Reporter)

Comment 10

6 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

6 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

6 years ago
It seems unlikely that any addons manually call the GC, but I threw a flag on there anyways.
Keywords: addon-compat
Duplicate of this bug: 703067
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 16

6 years ago
Created attachment 577344 [details] [diff] [review]
v3: With review feedback.

Thanks for the review!
Attachment #573388 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/5a2fc54f90a4
https://hg.mozilla.org/mozilla-central/rev/5a2fc54f90a4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.