In bug 850065, I'm making the Unlinking() and Unroot() phases of the cycle collector incremental to reduce pause times. I measured how long Unroot() is taking (which is going to often be the last reference, so this should mostly be the cost of the destructor), and on TechCrunch page teardown I see nsCSSStyleSheets taking a long time: CC: Object 0x11cd98550 took 107.48us to Unroot. <-- nsCSSStyleSheet CC: Object 0x11cd98430 took 213.98us to Unroot. <-- nsCSSStyleSheet CC: Object 0x12b23b3a0 took 117.04us to Unroot. <-- nsCSSStyleSheet CC: Object 0x12b23afb0 took 4255.67us to Unroot. <-- nsCSSStyleSheet CC: Object 0x12b23b0d0 took 878.75us to Unroot. <-- nsCSSStyleSheet The 0.1ms ones aren't a problem, but the 4.2ms one and even the 0.8ms one are pretty bad, given that I'm trying to make slices take under 5ms. I haven't investigated what is taking so long yet. It could just be tearing down a giant non-cycle-collected data structure.

Disclaimer: this is in an opt debug build...

I also see nsDOMAttributeMap taking 0.7ms to tear down, which I understand may also be style sheet related.

Looks like RemoveSheet is the slow thing here: QQQQ: Clearing children took 0.051000us QQQQ: DropRuleCollection took 3.765000us QQQQ: DropMedia took 3.220000us QQQQ: RemoveSheet took 5926.785000us (usually it is much faster, of course)

Digging further, this appears to be mostly time spent clearing mOrderedRules in ~nsCSSStyleSheetInner, which is an nsCOMArray. Clearing each element doesn't appear to take all that long, so we should be able to swap it into a runnable, and incrementally destroy it.

This is based on the lazy content unbinder, and defers the destruction of nsCSSStyleSheetInner::mOrderedRules to a runner, which does so in 4ms chunks. This knocks the max pause time on TechCrunch teardown from about 9 or 10ms to a little under 6ms.

Comment on attachment 742867 [details] [diff] [review] All hail the lazy rule destroyer! WIP Review of attachment 742867 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsCSSStyleSheet.cpp @@ +922,5 @@ > + { > + MOZ_ASSERT(nsCCUncollectableMarker::sGeneration, "Don't call append during shutdown."); > + if (sRuleDestroyer) { > + sRuleDestroyer->mLast->mNext = new RuleDestroyer(); > + sRuleDestroyer->mLast = sRuleDestroyer->mLast->mNext; How about a LinkedList?

(In reply to :Ms2ger from comment #6) > How about a LinkedList? Yeah, that's a good point. I was actually thinking that it might be good to add a general deferred release mechanism to the cycle collector, where things can register objects with two methods, IncrementalDestroy(int32 timeLeft) and DestroyAll(), with the CC, and then the CC could schedule them to run after it has done everything else it needs to do, instead of everybody who wants to do this having to implement their own thing. In addition to simplifying the implementation, it would probably be less janky if a single point was scheduling all of these, rather than everybody firing off their own runnables. When I was testing this, I noticed the Lazy Rule Destroyer was firing off while ICC was still going on.

Rebased, plus updated to take into account nsCSSStyleSheet being CCed now. With the large caveat that this was in an opt debug build, in my local measurement, unlinking nsCSSStyleSheets when closing TechCrunch takes 4.5ms. With this patch, it only takes 0.125ms. Other things that are worth looking at are GroupRule, taking 1.4ms, and FragmentOrElement, at about 2ms.

It looks like maybe the GroupRule unlink time is mostly spent in destroying its own list of CSS Rules. It is easy enough to make it also use the lazy rule destroyer.

Now with support for GroupRule

Oddly, this doesn't seem to be a problem on TechCrunch any more. The longest list of rules I see is 73, whereas I'm pretty sure it was like a thousand before. I wonder if this is still worth fixing.

I rewrote the lazy rule destroyer in a less gross way. Some debugging code is still present.

Oh, and you have to set beIncremental to true to have it actually do anything incrementally...

Oops, it turns out I wasn't seeing any performance improvements because I forgot to comment out the eager clearing of the style sheet list. This reduces unlink time by about 3ms, which is around what I was seeing before. Of course, the runnable ends up running immediately, while the CC is still going, which is not ideal.

It works a little better when the budget is set to 2ms rather than 2s...

I measured again with a newer version of the patch, and it is still pretty effective. On TechCrunch page close, it reduced unlink time by 4.5ms, which also reduced byte max pause time from 10ms to 6ms. with my patch: cc: ScanIncrementalRoots::fix nodes took 1.4ms cc: CollectWhite::Root took 1.1ms cc: CollectWhite::Unlink took 3.3ms cc: CleanupAfterCollection::mGraph.Clear() took 3.6ms cc: total cycle collector time was 601ms in 16 slices cc: visited 8692 ref counted and 62829 GCed objects, freed 8623 ref counted and 62783 GCed objects (99%). without: cc: ScanIncrementalRoots::fix nodes took 1.3ms cc: CollectWhite::Root took 1.0ms cc: CollectWhite::Unlink took 7.8ms cc: CleanupAfterCollection::mGraph.Clear() took 2.4ms cc: total cycle collector time was 443ms in 12 slices cc: visited 8378 ref counted and 51535 GCed objects, freed 8310 ref counted and 51491 GCed objects (99%).

This version builds on DeferredFinalize, so it avoids creating its own runnable. It builds on the patch in bug 866681 that makes it usable for CCed things. It also encapsulates the deferred destruction behavior in a new subclass of nsCOMArray, that only replaces the behavior for Clear() and the dtor. This is less error-prone, because you don't have to remember to fix up every use. In a prior revision of this code, I accidentally lost the deferred behavior when rebasing because some code had been added in another place to do a clear. It is still a little fragile in that you could remove large numbers of things from the array though some other method, but I think this is okay. The deferred finalizer keeps an array of arrays of rules. In each slice, it removes the last |aSlice| elements of the last array, then removing any newly-emptied arrays. One little detail is that I had to ensure that we don't defer anything if we're in the middle of shutdown. We synchronously clear the deferred finalizer stuff out at the end of the CC, which I thought would be enough, but it turns out (bug 1155454) that there's a cache that holds alive CSS stuff after the cycle collector is gone.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=84795dd6d38e I'm going to give the content unbinder changes a few more days to settle in before I put this up for review.

I don't want to land this until content process telemetry is working.

Comment on attachment 8611288 [details] [diff] [review] Incremental css::Rule destroyer. Given that you're putting it in namespace mozilla, please don't use the "ns" prefix on the class name (or filename). >+ while(aSliceBudget > 0 && newOuterLen > 0) { "while (" with a space >+ mozilla::DeferredFinalize(AppendRulesArrayPointer, Doesn't seem like you need the "mozilla::" given the "using namespace mozilla" above. Regarding the callback functions -- I mildly prefer the convention of making variables with their real types for all the void* parameters as the very first thing in the function, then a blank line, and then continuing with other things -- but it's ok as is if you don't want to change that. (Mixing with argument-validation assertions is fine.) (That would mean having a variable for aObject in AppendRulesArrayPointer, and adding a blank line after the first 2 lines of DeferredFinalizeRulesArray.) >+ delete rulesArray; Perhaps the comments in DeferredFinalize.h should mention that you're supposed to do this? >+ uint32_t newOuterLen = rulesArray->Length(); rulesArray is an nsTArray, whose Length() returns size_t. I think you should thus use size_t for newOuterLen. >+ aSliceBudget -= currSlice; In theory DeferredFinalize.h says this should be protected by if (aSliceBudget != UINT32_MAX). r=dbaron with that

Comment on attachment 8622568 [details] [diff] [review] Incremental css::Rule destroyer. Carrying forward dbaron's r+. (In reply to David Baron [:dbaron] ⏰UTC-7 (mostly busy, back June 29) from comment #22) > Perhaps the comments in DeferredFinalize.h should mention that you're > supposed to do this? That's true. I filed bug 1174783 to fix up the comment. > In theory DeferredFinalize.h says this should be protected by > if (aSliceBudget != UINT32_MAX). Good catch! In all the time I've spent looking over this code, I never noticed that. We do in fact rely on that when calling the callback, but none of the existing implementors actually follow that convention, which could be bad. I filed bug 1174796 for addressing that somehow. I addressed the rest of the review comments (including the stuff about the void* casts). Thanks for the review.

new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3a5eb6b971d