Last Comment Bug 713916 - DOMGCFinishedCallback should schedule just GC buffer shrinking, not a shrinking GC
: DOMGCFinishedCallback should schedule just GC buffer shrinking, not a shrinki...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla12
Assigned To: Igor Bukanov
:
:
Mentors:
Depends on:
Blocks: 707162
  Show dependency treegraph
 
Reported: 2011-12-28 12:02 PST by Igor Bukanov
Modified: 2012-01-06 13:49 PST (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
affected
affected


Attachments
shrink GC buffer API (10.56 KB, patch)
2011-12-28 12:14 PST, Igor Bukanov
wmccloskey: review+
Details | Diff | Splinter Review
schedule shrinking of gc buffers (7.51 KB, patch)
2011-12-28 12:23 PST, Igor Bukanov
bent.mozilla: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2011-12-28 12:02:53 PST
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.
Comment 1 Igor Bukanov 2011-12-28 12:14:12 PST
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.
Comment 2 Igor Bukanov 2011-12-28 12:23:53 PST
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.
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-29 10:02:11 PST
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?
Comment 4 Igor Bukanov 2011-12-29 13:44:43 PST
(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 5 Bill McCloskey (:billm) 2011-12-29 14:54:50 PST
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."
Comment 6 Igor Bukanov 2011-12-29 15:30:59 PST
(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.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-30 04:53:15 PST
https://hg.mozilla.org/mozilla-central/rev/d720247c6f94
https://hg.mozilla.org/mozilla-central/rev/3a4643fe9f0f
Comment 9 Alex Keybl [:akeybl] 2012-01-06 13:39:11 PST
Please nominate for uplift to aurora/beta if deemed necessary. If not, we should no longer track.
Comment 10 Igor Bukanov 2012-01-06 13:49:45 PST
This should not go to aurora/beta -

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