The default bug view has changed. See this FAQ.

DOMGCFinishedCallback should schedule just GC buffer shrinking, not a shrinking GC

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(firefox9 unaffected, firefox10 affected, firefox11 affected)

Details

Attachments

(2 attachments)

10.56 KB, patch
billm
: review+
Details | Diff | Splinter Review
7.51 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Currently just to release empty GC chunks and decommit arenas DOMGCFinishedCallback schedules a full shrinking GC if the last one leaves some empty chunks. As the GC release and allocate GC chunks in the background that may mean that in some workloads there is always an empty GC chunks and a new GC would always be scheduled.

To mitigate that we should add a new API that just release empty GC chunks and decommit arenas and schedule it from DOMGCFinishedCallback, not a full GC call.
(Assignee)

Comment 1

5 years ago
Created attachment 584614 [details] [diff] [review]
shrink GC buffer API

The patch adds new friend API, JS_ShrinkGCBuffers, that starts empty chunk release and arena decommit in the background. The implementation piggy back on the current background sweep implementation.

The usage of the API is in a separated patch.
Attachment #584614 - Flags: review?(wmccloskey)
(Assignee)

Comment 2

5 years ago
Created attachment 584616 [details] [diff] [review]
schedule shrinking of gc buffers

The patch replaces scheduling of a full GC when three is at least one GC chunk with a 5 second delayed call to JS_ShrinkGCBuffers. If a GC happens before that, that call is canceled and a new one is scheduled.

The idea behind it is to keep empty arenas and chunks for few seconds in case the main thread want to allocate new GC things in near future.
Attachment #584614 - Attachment is obsolete: true
Attachment #584614 - Flags: review?(wmccloskey)
Attachment #584616 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Attachment #584614 - Attachment is obsolete: false
Attachment #584614 - Flags: review?(wmccloskey)
Comment on attachment 584616 [details] [diff] [review]
schedule shrinking of gc buffers

Review of attachment 584616 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsJSEnvironment.cpp
@@ +1111,5 @@
>    Preferences::UnregisterCallback(JSOptionChangedCallback,
>                                    js_options_dot_str, this);
>  
> +  if (mGCOnDestruction)
> +    PokeGC();

Nit: elsewhere you brace single-line if statements... Style in this file looks inconsistent, but at least your additions could be consistent :)

@@ +3389,5 @@
> +  if (sShrinkGCBuffersTimer) {
> +    return;
> +  }
> +
> +  CallCreateInstance("@mozilla.org/timer;1", &sShrinkGCBuffersTimer);

Hm. I guess we do this already, so maybe it's not so bad, but I would think constantly destroying and recreating a timer here (and for GC, and for CC) could be a lot of needless churn. The component manager isn't super speedy (it has to grab locks, hash lookups, virtual calls, CC-able addrefs maybe?) so maybe what you really want to do is create one timer and then just keep a boolean flag for whether it's armed or not.

Doing that for all three of these timers would probably save a bunch of time. Followup bug?
Attachment #584616 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 4

5 years ago
(In reply to ben turner [:bent] from comment #3)
> > +  if (mGCOnDestruction)
> > +    PokeGC();
> 
> Nit: elsewhere you brace single-line if statements... Style in this file
> looks inconsistent, but at least your additions could be consistent :)

I tried to preserve the style that a function uses even if it implies to use different styles in the patch for single file. In that case we have at the start of nsJSContext::DestroyJSContext() :

  if (!mContext)
    return;

That suggested to skip {} around PokeGC call. I guess I will simply add {} in both cases.
Comment on attachment 584614 [details] [diff] [review]
shrink GC buffer API

Review of attachment 584614 [details] [diff] [review]:
-----------------------------------------------------------------

This looks nice. Is there anything we can do to mitigate this problem on 10 or 11? I'm worried this stuff is too risky to land on branches.

::: js/src/jsgc.cpp
@@ +2524,5 @@
> +    ExpireChunksAndArenas(rt, shrinking);
> +    if (!shrinking && shrinkFlag) {
> +        /*
> +         * The main thread may require to shrink when we do non-shrink
> +         * expiration. Restart if so.

This confused me for a moment. Please change this to:
"The main thread may have called ShrinkGCBuffers while ExpireChunksAndArenas was running, so we recheck the flag afterwards."
Attachment #584614 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 6

5 years ago
(In reply to Bill McCloskey (:billm) from comment #5)
------------------
> 
> This looks nice. Is there anything we can do to mitigate this problem on 10
> or 11? I'm worried this stuff is too risky to land on branches.

For branches we can disable the background chunk allocation via one-liner change - I am waiting for bz tests to confirm that it was the main reason for the regression.
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d720247c6f94
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a4643fe9f0f
https://hg.mozilla.org/mozilla-central/rev/d720247c6f94
https://hg.mozilla.org/mozilla-central/rev/3a4643fe9f0f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12

Comment 9

5 years ago
Please nominate for uplift to aurora/beta if deemed necessary. If not, we should no longer track.
(Assignee)

Comment 10

5 years ago
This should not go to aurora/beta -
tracking-firefox10: + → ---
tracking-firefox11: + → ---
You need to log in before you can comment on or make changes to this bug.