To reduce over all memory usage during testing, GCs and CCs are forced all the time. This will keep us from doing much testing of actual incremental GC and CC.

Not all the time. every 10 page or so.

See also bug 939606.

If I understand correctly, implementing bug 939606 might mean we can revert bug 938945 and so call this bug fixed, right?

Yeah, something like that. I just wanted this bug on file that specifically addresses what I need for ICC.

I should measure how many incremental/regular CCs we get with bug 938945 reverted and ICC on, version with just ICC on, versus on trunk, on mochitests.

Make sure we really cancel and delete the GC timers when we call their timer fired callback.

This adds a new function pokeCollectors that is designed to accelerate the firing of the various collector timers. It runs down the list of possible timers that might be waiting to fire, and if it finds one, it fires that timer immediately. If there is no such pending timer, then it begins an incremental GC. In this way, we run the collectors more aggressively, but preserve their incremental nature. This is triggered whenever we finish a test. It backs out that other test that did a synch GC+CC every 10 tests. I still need to test a bit more, and make sure it doesn't fall over when ICC is disabled.

Do either of you have any thoughts on this approach? Whenever we finish a test we call this thing that runs down the list of collector timers. If we're in the middle of a lightweight timer, like an ICC or IGC timer, we just do a chunk of work. If our pending time is something that will do something more heavyweight, we increment a counter. If we are incrementing a counter, and it hits 5, then we start a CC, GC etc. One weird wrinkle is that the CCTimerFired gets called a number of times, to do the forget skippable, so once we hit the counter limit, that happens every time, but those pre-CC cleanup things are lightweight so I think that's okay.

Comment on attachment 8393855 [details] [diff] [review] part 2 - Add and use runNextCollectorTimer. Review of attachment 8393855 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSEnvironment.cpp @@ +2432,5 @@ > > +static bool > +ReadyToTriggerExpensiveCollectorTimer() > +{ > + return kPokesBetweenExpensiveCollectorTriggers < ++sExpensiveCollectorPokes; I like the approach, but some of the logic is kind of hard to follow. Can we just have ReadyToTriggerExpensiveCollectorTimer reset the counter whenever it returns true. Then you can get rid of ClearPokeExpensiveTimer. I think you'll be getting the same behavior, but it will be easier to understand. @@ +2449,5 @@ > + if (sShuttingDown) { > + return; > + } > + > + if (sGCTimer) { It would be nice to check sFullGCTimer as well here.

> Then you can get rid of ClearPokeExpensiveTimer. Well, the idea was that the collectors can fire for other reasons, such as being force from DOMWindowUtils, so we should clear the counter in those cases. Maybe it isn't a big deal. > It would be nice to check sFullGCTimer as well here. Well, since a regular GC timer starts up a full GC timer, my concern was starving the CC. Maybe I could do the sFullGCTimer check inside the sGCTimer check?

Also would it be a worry if we ended up mostly doing full GCs?

Comment on attachment 8393855 [details] [diff] [review] part 2 - Add and use runNextCollectorTimer. Somehow a bit hard to follow. I guess nsJSContext::RunNextCollectorTimer() could use some comments.

It looks like these patches are enough to make bug 957109 happen more frequently: https://tbpl.mozilla.org/?tree=Try&rev=7c3cd0142f16 https://tbpl.mozilla.org/?tree=Try&rev=31ae217c9902 It happened 1/5 times on L64 (maybe?0, and 2/23 times on WinXP.

with bug 957109 this patch doesn't seem to cause many leaks: https://tbpl.mozilla.org/?tree=Try&rev=453862228342 I'm going to go for a delayed rollout strategy: I'll wait a day or so between each of the various patches here to detect any fallout.

I'm not going to land this earlier than early next week, so no huge hurry on the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f42f8c4bfd26 https://hg.mozilla.org/integration/mozilla-inbound/rev/6ebfa2a0fbc8 Note to sheriffs: if bug 957109 starts happening a lot, this is probably to blame. Per comment 17, I think I fixed that but I may be wrong. This also slightly increases memory usage on plain Mochitests, but hopefully not enough to be a problem. If it is a problem, I can probably tweak things.

https://hg.mozilla.org/mozilla-central/rev/f42f8c4bfd26 https://hg.mozilla.org/mozilla-central/rev/6ebfa2a0fbc8