935721 - Reorganize top-level cycle collector stuff to prepare for ICC

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Andrew McCreight [:mccr8]]

try run is green: https://tbpl.mozilla.org/?tree=Try&rev=bf274d8637e1

I'll upload patches tomorrow.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Store the listener pointer on the nsCycleCollector data structure. r=smaug

With ICC, the listener must persist across invocations of the CC, so store it on the CC.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Allocate GCGraphBuilder on the heap. r=smaug

With ICC, the graph builder must persist across invocations of the CC, so store it on the heap.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Use RAII to set mScanInProgress, hoist out MarkRoots and ScanRoots. r=smaug

With ICC, mScanInProgress gets set and cleared a bunch of times so add an RAII class to turn it on when we're doing stuff and clear it when we're not.

With that in place, we can easily move MarkRoots and ScanRoots out of BeginCollection
in preparation for making them separate phases in ICC.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Allocate white nodes array in CollectWhite. r=smaug

The white nodes array is only used in CollectWhite, so just allocate it there.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Invert the control flow of CycleCollectNow's pre- and post-collection work, add CCResults as a field on the CC. r=smaug

nsJSEnvironment::CycleCollectNow does work before and after a CC runs. With ICC, nsJSEnv won't
know where in the CC when a CC is about to begin or end, so this patch reorganizes that work
into two separate callback hooks.  This requires adding a new struct, CycleCollectorStats, to
hold data nsJSEnv needs between the two calls.

Rather than trying to pass around a pointer to a results structure, this patch just adds
it to the nsCycleCollector struct, and always stores them. The results are passed back
to the end CC callback.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 6 - Replace ccResults with aResults in EndCycleCollectionCallback. r=smaug

This is done in a separate patch to reduce the size of the previous patch a bit.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 7 - Inline PrepareForCollection into BeginCollection. r=smaug

PrepareForCollection is trivial now, so just inline it.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 8 - Add a reference to the graph to the graph builder. r=smaug

This moves towards letting the CC graph outlive the builder.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: part 9 - Move mPtrToNodeMap into GCGraph. r=smaug

With ICC, we may have to remove things from the graph after we have finished building
the graph, so move the mapping to graph addresses into the graph itself to create a
more self-contained structure.

Comment 11

[Andrew McCreight [:mccr8]]

Part 5 is the ugliest thing here, and is probably the ugliest patch in my entire stack of ICC patches.  There are three kinds of things we do in nsJSEnvironment: those we must do on every CC slice, those we do when we start a CC, and those we do when we end a CC.  This patch moves the latter two into callbacks.

We gather information about the per-slice information, but only record the max across all slices for a single CC, because it is easier that way, and max pause time is the more interesting thing anyways.  Eventually, we might want something fancy like IGC has that records all the information about every slice.

I should double check that this doesn't mess up the information we are reporting on the console...

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 10: Separate nsCycleCollector_collect and nsCycleCollector_scheduledCollect

With ICC, we will pass in a time slice budget to timer-scheduled CCs, but not manually triggered CCs, so it is handy to have two separate entry points. Plus it is just a little cleaner to reduce the pile of optional arguments and confusing booleans to these functions.

Comment 13

[Andrew McCreight [:mccr8]]

First, this patch defines a new function PrepareForCycleCollection that contains the stuff we need to run every time we enter the CC.  Then it defines a new ScheduledCycleCollectNow.  With the two call paths (scheduled and manual) separated, we can eliminate the boolean argument all over the place.

Comment 14

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

Comment on attachment 828782 [details] [diff] [review]
part 3 - Use RAII to set mScanInProgress, hoist out MarkRoots and ScanRoots. r=smaug


>+struct MOZ_STACK_CLASS AutoBool
>+{
>+    AutoBool(bool &aBool) : mBool(aBool)
>+    {
>+        MOZ_ASSERT(!mBool);
>+        mBool = true;
>+    }
>+
>+    ~AutoBool()
>+    {
>+        MOZ_ASSERT(mBool);
>+        mBool = false;
>+    }
>+private:
>+    bool &mBool;
>+};
Could you call this AutoTrue or some such.

Comment 15

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

or use AutoRestore

Comment 16

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

MOZ_ASSERT(!mScanInProgress);
AutoRestore<bool> ar(mScanInProgress);
mScanInProgress = true;

Comment 17

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

Comment on attachment 828784 [details] [diff] [review]
part 5 - Invert the control flow of CycleCollectNow's pre- and post-collection work, add CCResults as a field on the CC. r=smaug

A bit hard to follow in some places. I blame hg diff.


Could you document the member variables of CycleCollectorStats.

Comment 18

[Andrew McCreight [:mccr8]]

thanks for the reviews!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/067924b46192
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ceef597d36b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/50329b4cf0be
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f503f2359989
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9e354b8e06ce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eedc72464698
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ea0308d733
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/374376d6e85e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e48c498dbe18
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e597cdb674ea

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/067924b46192
https://hg.mozilla.org/mozilla-central/rev/5ceef597d36b
https://hg.mozilla.org/mozilla-central/rev/50329b4cf0be
https://hg.mozilla.org/mozilla-central/rev/f503f2359989
https://hg.mozilla.org/mozilla-central/rev/9e354b8e06ce
https://hg.mozilla.org/mozilla-central/rev/eedc72464698
https://hg.mozilla.org/mozilla-central/rev/c3ea0308d733
https://hg.mozilla.org/mozilla-central/rev/374376d6e85e
https://hg.mozilla.org/mozilla-central/rev/e48c498dbe18
https://hg.mozilla.org/mozilla-central/rev/e597cdb674ea