1037645 - Eliminate last traces of unnecessary purging from ForkJoinNursery::forwardFromTenured

Status: RESOLVED INVALID

(Core :: JavaScript: GC, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Only purge free lists that are inside the fromspace and don't iterate over arenas that aren't.

In bug 1034225 I moved the ArenaLists::purge() call out of the inner loop, but it turns out we can go a bit further: we only have to purge if the currently active free list is inside the fromspace.

As far as I can tell we also don't have to iterate over any arenas that aren't inside the fromspace in the inner loop - we don't need to (and won't) move any objects inside them anyway. This seems like a much more reasonable reason to end the inner loop early than the one I added in bug 1034225.

The attached patch makes these changes. Quick try push: https://tbpl.mozilla.org/?tree=Try&rev=dbbc65613b4c

Comment 1

[Lars T Hansen [:lth]]

Comment on attachment 8454682 [details] [diff] [review]
Only purge free lists that are inside the fromspace and don't iterate over arenas that aren't.

Review of attachment 8454682 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see how this can work; isInsideFromspace() is only ever true for any bump-allocated chunks that were in the newspace in the previous iteration, so isInsideFromspace(aheader) will always be false.

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Only purge free lists outside the evacuation zone and don't iterate over arenas inside it.

Oh, I see, guess I got pretty confused. This patch more-or-less does the inverse: we check the target arena's zone against the evacuation zone. If the free list's zone matches the evacuation zone, we don't need to purge it, and we shouldn't iterate over arenas that are inside the evacuation zone. If the evacuation zone is null, I would expect this test to always fail - all arenas should have an associated zone (right?).

Unfortunately I got a crash with this patch while running the parallel jit-tests in an opt-build with forwardFromTenured on the stack, and so far I haven't been able to reproduce it in a debug build (the crash pointed to the first line of ForkJoinNursery::getObjectAllocKind(), which makes no sense to me, but I didn't look too deep since it was an opt build). I'm still putting this up for review as I try to reproduce; let me know if something stands out to you as obviously wrong.

I do have a general question about forwardFromTenured(): I understand that during an evacuating GC, we want to move the objects in the tenured arenas to the evacuation zone. But why do we run this function at all outside evacuating GCs? Won't this move all the tenured objects back into the nursery?

Comment 3

[Lars T Hansen [:lth]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)

> I do have a general question about forwardFromTenured(): I understand that
> during an evacuating GC, we want to move the objects in the tenured arenas
> to the evacuation zone. But why do we run this function at all outside
> evacuating GCs? Won't this move all the tenured objects back into the
> nursery?

I think you misunderstand the meaning of the function (and probably the terminology).  "Forwarding" scans a space and looks for pointers in that space that reference objects that need to be moved.  The objects that are to be moved are always in the fromspace, ie always in the space that was the nursery, until the present GC commenced.  Specifically, forwardFromTenured scans the thread's private tenured area and looks for pointers from the tenured area into the fromspace, and moves objects that are in the fromspace to the tospace.  The tospace can be a new nursery, it can be the private tenured area, or it can (in principle) be a combination of the two, though I'm not currently using that option.

The terminology follows "forwardFromStack", which scans the stack looking for objects in the fromspace, and "forwardFromUpdatable", which scans the "updatable object", ie, the result array, looking for objects in the fromspace.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8466110 [details] [diff] [review]
Only purge free lists outside the evacuation zone and don't iterate over arenas inside it.

Review of attachment 8466110 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still not understanding what's being accomplished here (and I guess I have the crashes you report to back me up :)

I mildly believe that the number of purges could be reduced by guarding the call to lists.purge() but I don't understand the guard you're using.  To omit the purge the guard would have to prove that the list to be purged is never the list we're going to be allocating out of in the loop below, within the calls to traceObject, that is, to omit the purge the list's zone must not be the target zone.  I don't know if that's sufficient though.  Plus, the lists we're working on here belong to the private tenured area, so I don't understand how their zone could ever be anything other than the target zone.

Also I don't think changing the loop condition is right.  The point here is that we have to loop across the arena chunks that exist when we start iterating, that's what the current loop does.  The new loop looks like it could stop at some arbitrary place just because the zone of the chunk is different.   That might be ok if that's how the arena is organized but would require starting at the right place, and I don't see that it does that.

Is there any evidence that there is actually any "unnecessary" purging going on?

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Lars T Hansen [:lth] from comment #4)
> I'm still not understanding what's being accomplished here (and I guess I
> have the crashes you report to back me up :)

Well, I only saw it crash once and couldn't reproduce in a debug build :( Still, there's probably a real problem I guess.

> I mildly believe that the number of purges could be reduced by guarding the
> call to lists.purge() but I don't understand the guard you're using.  To
> omit the purge the guard would have to prove that the list to be purged is
> never the list we're going to be allocating out of in the loop below, within
> the calls to traceObject, that is, to omit the purge the list's zone must
> not be the target zone.  I don't know if that's sufficient though.  Plus,
> the lists we're working on here belong to the private tenured area, so I
> don't understand how their zone could ever be anything other than the target
> zone.

The lists don't have a zone pointer, but the arenas themselves do. In ForkJoinNursery::allocateInTospace we set new arenas' zone pointer to the evacuation zone during an evacuating GC.

> Also I don't think changing the loop condition is right.  The point here is
> that we have to loop across the arena chunks that exist when we start
> iterating, that's what the current loop does.  The new loop looks like it
> could stop at some arbitrary place just because the zone of the chunk is
> different.   That might be ok if that's how the arena is organized but would
> require starting at the right place, and I don't see that it does that.

At the start of forwardFromTenured we may be allocating from an arena in the evacuation zone. Do we need to check the objects in cells that have already been moved? If so then I agree it's not a valid reason to break out of the loop.

> Is there any evidence that there is actually any "unnecessary" purging going
> on?

Well, in bug 1017165 I saw multiple partially full arenas during calls to MergeCompartments originating from PJS. Ideally we'd end a collection with at most one partially full arena, although it ended up not mattering for that bug. The remaining purging may well be necessary though, my understanding of how this works was clearly wrong before.

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

PJS is being removed and this was probably based on a misunderstanding on my part anyway.