979047 - Misc. ICC scheduling fixups

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

There are a few things that are a little off here.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Only clear mBeginSliceTime in EndCycleCollectionCallback.

Run FinishCycleCollectionSlice only once per slice, and only if we've previously run PrepareForCycleCollectionSlice.

https://tbpl.mozilla.org/?tree=Try&rev=c7e6a3a562d7

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Remove unused endGCTime assignments.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Make sure we trigger a GC after a current ICC when we poke the GC.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Run ICCSliceTime() after PrepareForCollectionSlice.

This way we take into account time spent finishing off a GC, or something.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Make sure we have at least one early forgetSkippable timer fire.

With ICC, ccDelay can get close to NS_CC_SKIPPABLE_DELAY, but we want to always want to run at least one forgetSkippable before the actual CC.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Only clear mBeginSliceTime in EndCycleCollectionCallback.

Run FinishCycleCollectionSlice only once per slice, and only if we've previously run PrepareForCycleCollectionSlice.

Olli pointed out that we should probably initialize mBeginSliceTime, and I think it is a nice idea to ensure it is cleared when we're not running the CC, so I added back the clearing of it to Clear().

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8385507 [details] [diff] [review]
part 1 - Only clear mBeginSliceTime in EndCycleCollectionCallback.

>+  if (!gCCStats.mBeginSliceTime.IsNull()) {
>+    gCCStats.FinishCycleCollectionSlice();
>+  }

FinishCycleCollectionSlice returns early if mBeginSliceTime is null, so you shouldn't need this.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8385422 [details] [diff] [review]
part 3 - Make sure we trigger a GC after a current ICC when we poke the GC.

Makes sense.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8385424 [details] [diff] [review]
part 5 - Make sure we have at least one early forgetSkippable timer fire.

I don't see how numEarlyTimerFires can be currently < 1.
Couldn't you just MOZ_ASSERT that numEarlyTimerFires >= 1

Either explain how numEarlyTimerFires can be < 1 (perhaps I missed something),
or just use assert.

Comment 10

[Andrew McCreight [:mccr8]]

> I don't see how numEarlyTimerFires can be currently < 1.

I explained in IRC, but just for posterity, this isn't a problem right now, but I've been messing around with kMaxICCDuration, and when you make it larger, like 4000, then you can hit problems.

The problem is even worse right now on trunk when you increase that value, because numEarlyTimerFires is a uint which underflows, so you just keep running the early timer fires forever, and never actually run the CC.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #7)
> FinishCycleCollectionSlice returns early if mBeginSliceTime is null, so you
> shouldn't need this.

Good catch!  I fixed that.  As you can see, this patch had a lot of iterations.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0775be6f49
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f05c51c43275
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aed1ab91fd8a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd36571c4d92
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/331738fa38a5

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b0775be6f49
https://hg.mozilla.org/mozilla-central/rev/f05c51c43275
https://hg.mozilla.org/mozilla-central/rev/aed1ab91fd8a
https://hg.mozilla.org/mozilla-central/rev/cd36571c4d92
https://hg.mozilla.org/mozilla-central/rev/331738fa38a5