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.

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.

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

(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."

(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.

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

Please nominate for uplift to aurora/beta if deemed necessary. If not, we should no longer track.

This should not go to aurora/beta -