1034225 - Can we avoid purging during ForkJoinNursery::forwardFromTenured?

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm filing this based on https://bugzilla.mozilla.org/show_bug.cgi?id=1017165#c89.

Looking at the code at http://hg.mozilla.org/mozilla-central/annotate/ac6960197eb6/js/src/gc/ForkJoinNursery.cpp#l447 I don't understand why we need to purge. Usually in code like this, we do copyFreeListToArena and then clearFreeListInArena after we're done. Is that possible in this code?

The problem with purge is that we can't allocate into that arena until the next full GC.

Comment 1

[Lars T Hansen [:lth]]

I have to do a little archeology to figure that out, but I seem to remember that I had a lot of asserts if I did not purge.  But I don't know how many other approaches I tried - it's possible I was just hacking around and purge seemed to do the trick.  That said, I believe I did try copyFreeListToArena in some context.

If I have a little time today I'll look into using the pair of methods you mentioned.

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Avoid purging the free lists every iteration in PJS by checking if the current arena is in use.

Whoops, missed that you filed this bug. Description from bug 1017165:

"If I'm reading the code right, though, PJS probably doesn't have to purge on every iteration. In a non-evacuating GC, we'll just copy the marked objects into the ForkJoinNursery::newspace, which is infallible [1]. For evacuating GCs, we can probably just add a small function to ArenaLists to check if a particular ArenaHeader matches the one for the current free list, and break out of the loop before we try to apply ArenaCellIter to it. Then we could just move the purge() call outside the loop, I think."

Lars, how does this look to you? I haven't tested this in a debug build yet, so might yet trip those assertions you were seeing. I'll leave the reviewer as Terrence, but feel free to hop in.

Comment 3

[Lars T Hansen [:lth]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)
> Created attachment 8450818 [details] [diff] [review]
> Avoid purging the free lists every iteration in PJS by checking if the
> current arena is in use.
> 
> Whoops, missed that you filed this bug. Description from bug 1017165:
> 
> "If I'm reading the code right, though, PJS probably doesn't have to purge
> on every iteration. In a non-evacuating GC, we'll just copy the marked
> objects into the ForkJoinNursery::newspace, which is infallible [1]. For
> evacuating GCs, we can probably just add a small function to ArenaLists to
> check if a particular ArenaHeader matches the one for the current free list,
> and break out of the loop before we try to apply ArenaCellIter to it. Then
> we could just move the purge() call outside the loop, I think."
> 
> Lars, how does this look to you? I haven't tested this in a debug build yet,
> so might yet trip those assertions you were seeing. I'll leave the reviewer
> as Terrence, but feel free to hop in.

Dispensing with a minor concern first: One of the near(ish)-term plans for PJS GC is to get rid of the distinction between evacuating and non-evacuating collections, since there are advantages to tenuring objects that are linked from the result array the first time we see them (see the first item on the list in bug 1019859).  The proposed change works at cross purposes to that.  But I can cross that bridge when I get to it.

As to the patch, it seems OK to me: if I understand correctly, arenas that are purged will be marked as not-in-use and will be scanned, and we'll be allocating into fresh arenas during forwarding, and those new arenas do not need to be scanned here.

If you land this be sure to manually run the slow tests in jit-test/tests/parallel in both release and debug builds.

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Lars T Hansen [:lth] from comment #3)
> Dispensing with a minor concern first: One of the near(ish)-term plans for
> PJS GC is to get rid of the distinction between evacuating and
> non-evacuating collections, since there are advantages to tenuring objects
> that are linked from the result array the first time we see them (see the
> first item on the list in bug 1019859).  The proposed change works at cross
> purposes to that.  But I can cross that bridge when I get to it.

I don't think I understand the full extent of what this change implies, but if it's just a matter of not being able to distinguish evacuating and non-evacuating GCs in forwardFromTenured(), just removing the isEvacuating_ check and doing the lists.arenaIsInUse() check unconditionally should work fine (it will always return false if we're not using new FreeLists).

Actually, that does make me think of something - to be safe, we should probably assert that the ArenaList's cursor is at the end of its list. I'm pretty sure it always should be, in this case - but things would break if it isn't.

> If you land this be sure to manually run the slow tests in
> jit-test/tests/parallel in both release and debug builds.

Thanks for the heads-up, will do.

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v2 - Avoid purging the free lists every iteration in PJS by checking if the current arena is in use.

This just adds the assertion I was talking about. I had to add another helper function to ArenaLists to do it, but it's small and I imagine we'll need something like it (and a few more) for the JIT fastpath I'm proposing in bug 1017165 anyway.

I ran the jit-tests with --slow on opt and debug on loop for a few hours with no issue. A debug try run on the version without the extra assertion also came back green: https://tbpl.mozilla.org/?tree=Try&rev=1284437947a7

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

For further reference, this is my understanding of the situation:

(1) We really do want to purge here, since we're moving all the objects either from one half of the nursery to the other, or out of the nursery into the evacuating zone. We don't want to accidentally allocate into the region we're moving *from*, and we also don't want to use whatever free list we currently have again afterward, so purging is the correct action.

(2) When we evacuate the nursery and allocate arenas from the evacuating zone, these arenas are appended to our ArenaList, and the ArenaIter will happily start walking them too. That's mostly fine - they're already outside the fromSpace, so we won't move these objects again - but we can't apply the ArenaCellIter to an arena that's marked as full, but which we're actually allocating out of. We were purging on each iteration to ensure that the arena we're inspecting wasn't being allocated out of, until the lack of new allocations naturally brought us to the end of the list.

(3) With my patch, we instead simply *check* if an arena is in use, and break out early. This is only correct if the ArenaList's cursor is at the end of the list, since otherwise we'll start allocating from the non-free arenas first, potentially breaking out early while there are still objects left in fromSpace. In v2 I added an assertion to check this - since we're taking care of GC ourselves, the cursor should never be moved to an earlier arena, so this assert should never trip.

(4) Prior to my patch, I'm pretty sure we were allocating a new arena for every arena that we marked, even if that arena barely had any marked objects in it, because we purged the free list on every iteration. After my patch, evacuating GCs should now effectively be compacting GCs as well, with every arena being full of marked objects except the last one.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8450907 [details] [diff] [review]
v2 - Avoid purging the free lists every iteration in PJS by checking if the current arena is in use.

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

Great! If Lars is okay with it, then let's unblock bug 1017165.

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks! It should allow me to shave about 2 lines off the patch in bug 1017165 ;) But this seemed like a nice simplification and improvement anyway.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f81fe2b04fef

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f81fe2b04fef