865320 - Move logic for deciding when to do a merging cycle collection into the CC itself

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Currently, nsJSEnvironment decides when the CC should do a merging collection, but there's no reason it should be done there.  Moving this into the CC decouples CycleCollectNow and the cycle collector a bit, which is a little nicer for ICC.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: WIP

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: decide in the CC when to merge or not

Comment 3

[Andrew McCreight [:mccr8]]

Try run (for a slightly modified version): https://tbpl.mozilla.org/?tree=Try&rev=88a7945381c3

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 742722 [details] [diff] [review]
decide in the CC when to merge or not

This moves the logic for deciding whether to merge zones or not into XPConnect and the cycle collector.

Instead of CycleCollectNow telling the CC if it should merge or not, it tells the CC if the CC was forced.  Right now, that just means that we shouldn't merge, but with ICC, we can use this information to do things like synchronously finishing off the current CC and running a new one, or things like that.

The bulk of the changes in this patch are passing around the the "type" of the CC, which is either normal, forced, or shutdown.  As above, right now this only affects whether we merge or not, but will be used with ICC to alter the behavior more.  It isn't heavily used, but it seems like a nice general concept to have.  I just pass it around rather than add it as a field because we really only need it at the start of a CC.

Comment 5

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

Comment on attachment 742722 [details] [diff] [review]
decide in the CC when to merge or not

This is hard to review since it mixes words zone and compartment.
Could you fix comments.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: decide in the CC when to merge or not, fixed comments

Comment 7

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

Comment on attachment 743348 [details] [diff] [review]
decide in the CC when to merge or not, fixed comments

>+enum ccType {
>+    NormalCC,
>+    ForcedCC,
>+    ShutdownCC
>+};
Could you call these something like
AllowZoneMergesCC,
NormalCC,
ShutdownCC



>+nsCycleCollector::ShouldMergeZones(ccType aCCType)
>+{
>+    if (!mJSRuntime) {
>+        return false;
>+    }
>+
>+    MOZ_ASSERT(mUnmergedNeeded <= kMinConsecutiveUnmerged);
>+    MOZ_ASSERT(mMergedInARow <= kMaxConsecutiveMerged);
>+
>+    if (mMergedInARow == kMaxConsecutiveMerged) {
>+        MOZ_ASSERT(mUnmergedNeeded == 0);
>+        mUnmergedNeeded = kMinConsecutiveUnmerged;
>+    }
>+
>+    if (mUnmergedNeeded > 0) {
>+        mUnmergedNeeded--;
>+        mMergedInARow = 0;
>+        return false;
>+    }
>+
>+    if (aCCType == NormalCC && mJSRuntime->UsefulToMergeZones()) {
Then s/NormalCC/AllowZoneMergesCC/


>+void nsCycleCollector_collect(bool aForced,
>                               nsCycleCollectorResults *aResults,
>                               nsICycleCollectorListener *aListener);
aForced is really vague. aAllowZoneMergesCC? Need to then change how nsJSEnvironment calls this.

Comment 8

[Andrew McCreight [:mccr8]]

How about changing "aForced" to "aManuallyTriggered"?  Is that clearer?  Then I would change ForcedCC to ManualCC or something.

I could also change NormalCC to TimerCC, say, if you think that's a good idea.

Comment 9

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

aManuallyTriggered is clearer, yes. And TimerCC sounds good too, or should it be ScheduledCC or some such

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: decide in the CC when to merge or not

- renamed aForced to aManuallyTriggered
- rebased over some changes to the merging logic landed by bholley
- CC types are now ScheduledCC, ManualCC, ShutdownCC, and I added comments describing them.

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

Just waiting for inbound to reopen.

Comment 11

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a50323631c8

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a50323631c8