Closed Bug 935721 Opened 6 years ago Closed 6 years ago

Reorganize top-level cycle collector stuff to prepare for ICC

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: [qa-])

Attachments

(10 files)

6.00 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.71 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.65 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.76 KB, patch
smaug
: review+
Details | Diff | Splinter Review
34.23 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.02 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.73 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.33 KB, patch
smaug
: review+
Details | Diff | Splinter Review
No description provided.
try run is green: https://tbpl.mozilla.org/?tree=Try&rev=bf274d8637e1

I'll upload patches tomorrow.
With ICC, the listener must persist across invocations of the CC, so store it on the CC.
Attachment #828780 - Flags: review?(bugs)
With ICC, the graph builder must persist across invocations of the CC, so store it on the heap.
Attachment #828781 - Flags: review?(bugs)
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.
Attachment #828782 - Flags: review?(bugs)
The white nodes array is only used in CollectWhite, so just allocate it there.
Attachment #828783 - Flags: review?(bugs)
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.
Attachment #828784 - Flags: review?(bugs)
This is done in a separate patch to reduce the size of the previous patch a bit.
Attachment #828785 - Flags: review?(bugs)
PrepareForCollection is trivial now, so just inline it.
Attachment #828786 - Flags: review?(bugs)
This moves towards letting the CC graph outlive the builder.
Attachment #828787 - Flags: review?(bugs)
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.
Attachment #828788 - Flags: review?(bugs)
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...
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.
Attachment #829390 - Flags: review?(bugs)
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.
Blocks: 937751
Attachment #828780 - Flags: review?(bugs) → review+
Attachment #828781 - Flags: review?(bugs) → review+
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.
Attachment #828782 - Flags: review?(bugs) → review+
or use AutoRestore
MOZ_ASSERT(!mScanInProgress);
AutoRestore<bool> ar(mScanInProgress);
mScanInProgress = true;
Attachment #828783 - Flags: review?(bugs) → review+
Attachment #828785 - Flags: review?(bugs) → review+
Attachment #828786 - Flags: review?(bugs) → review+
Attachment #828787 - Flags: review?(bugs) → review+
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.
Attachment #828784 - Flags: review?(bugs) → review+
Attachment #828788 - Flags: review?(bugs) → review+
Attachment #829390 - Flags: review?(bugs) → review+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.