913130 - Make ShutdownCollect() call Collect()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight (out of office until 8/21) [:mccr8]]

The body of the ShutdownCollect() loop is almost the same as Collect(), so we should just make them the same.

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 1 - Modify nsCycleCollector::Collect to allow it to be used at shutdown.

This is the most complicated part.  Pass in white nodes as an argument, force a GC if the type is ShutdownCC, return if we collected anything.  No particular hurry on the reviews for this.

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 2 - Give Collect and ShutdownCollect more consistent ordering.

A few of the things in these two methods are different for no reason I can see, so make them consistent.  I figure it is better to do PrepareForCollection as early as possible.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 3 - Run {PrepareFor,CleanupAfter} every iteration of ShutdownCollect.

PrepareForCollection just initializes some variables and calls the runtime's PrepareForCollection, so doing it repeatedly doesn't seem like a big deal.  Doing CleanupAfterCollection on every iteration will fire telemetry repeatedly, but we don't run shutdown CCs in opt builds so it shouldn't matter.

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 4 - Replace body of ShutdownCollect loop with Collect.

The inner loop in ShutdownCollect is now the same as Collect, so just call Collect.

Comment 6

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

Comment on attachment 800830 [details] [diff] [review]
part 1 - Modify nsCycleCollector::Collect to allow it to be used at shutdown.


>-    void Collect(ccType aCCType,
>+    bool Collect(ccType aCCType,
>+                 nsTArray<PtrInfo*> *aWhiteNodes,
Could this be  nsTArray<PtrInfo*> &aWhiteNodes




Not sure I understand yet the setup... reading other patches.

Comment 7

[Andrew McCreight (out of office until 8/21) [:mccr8]]

> Could this be nsTArray<PtrInfo*> &aWhiteNodes

Sort of, but it just gets passed through a few functions and stuck into a field mWhiteNodes that has to be a pointer because it is NULL when the CC isn't running.  I'm also going to get rid of that argument for ICC anyways, so I'm just going to leave it as a * for now.

Comment 8

[Andrew McCreight (out of office until 8/21) [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9d23a05bd9d6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3d4d7d678c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0896b8c0f616
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5e861b43ebf1

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9d23a05bd9d6
https://hg.mozilla.org/mozilla-central/rev/ed3d4d7d678c
https://hg.mozilla.org/mozilla-central/rev/0896b8c0f616
https://hg.mozilla.org/mozilla-central/rev/5e861b43ebf1