Comment on attachment 595838 [details] [diff] [review] patch This relies on CanSkip to remove stuff from purple buffer and mark objects black. This should be useful in cases when DOM Node is in CCGeneration document, but not suspected.

Bah, I found a case when this is overkill. I need yet another CanSkip method, or change the existing nsGenericElement::CanSkip.

If we have a disconnected tree, make sure we know that is has been handled already. This wasn't an issue before, since we'd use CanSkip only during actual forgetSkippable call.

This is so effective, that if no regressions are found, we could perhaps think of taking this to FF12.

This isn't a huge problem, since object is usually removed from the purple buffer when forgetSkippable next time runs. But that needs possibly a new DOM tree traversing. (I actually thought the situation was worse until I noticed that I was testing a worse-case version of a testcase)

Do we think this patch will help mitigate recent GC/CC issues? What's the motivation for uplifting?

Comment on attachment 596502 [details] [diff] [review] additional patch 2 Review of attachment 596502 [details] [diff] [review]: ----------------------------------------------------------------- This is a little gross, but I guess we can leave untangling this unpurple stuff to a future patch. ::: xpcom/glue/nsCycleCollectionParticipant.h @@ +148,5 @@ > // If CanSkip returns true, p is removed from the purple buffer during > // a call to nsCycleCollector_forgetSkippable(). > // Note, calling CanSkip may remove objects from the purple buffer! > + // If aRemovingAllowed is true, p can be removed from the purple buffer. > + bool CanSkip(void *p, bool aRemovingAllowed = false) CanSkip is only called in two places, one with true and one with false, so having a default argument doesn't make sense.

(In reply to Alex Keybl [:akeybl] from comment #8) > Do we think this patch will help mitigate recent GC/CC issues? What's the > motivation for uplifting? This patch seems, at least locally cut down CC times significantly. Also, this shouldn't be horribly risky patch (I know, there have been 2 followups). This is not about mitigating recent CC issues, but actually part of fixing CC handling. Most of the CC handling fixes did land to FF12.

But before getting this to FF12, it is better to wait for few days to get some more Telemetry data.

We're now reconsidering for uplift to Aurora given the conclusion to our recent CC investigations, which made it clear that we were seeing significantly better performance on FF13. Tracking for FF12.

I'll prepare patches for FF12

[Approval Request Comment] Regression caused by (bug #): User impact if declined: Higher CC times Testing completed (on m-c, etc.): 2 weeks in m-c Risk to taking this patch (and alternatives if risky): Should be reasonable low risk, since it is just using other optimizations used also elsewhere. String changes made by this patch: N/A Hopefully it even compiles :) https://tbpl.mozilla.org/?tree=Try&rev=d8038c33230e

Comment on attachment 600083 [details] [diff] [review] all the patches for FF12 [Triage Comment] Approving for Aurora 12 to help defend against some of the pauses we've had internal reports about.