Can we avoid purging during ForkJoinNursery::forwardFromTenured?

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript: GC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: billm, Assigned: ehoogeveen)

Tracking

unspecified
mozilla33
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
Flags: needinfo?(hansen)

Updated

4 years ago
Flags: needinfo?(hansen)

Comment 1

4 years ago
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.

Updated

4 years ago
Summary: Can we avoid purging during 419ForkJoinNursery::forwardFromTenured? → Can we avoid purging during ForkJoinNursery::forwardFromTenured?
(Assignee)

Comment 2

4 years ago
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.
Attachment #8450818 - Flags: review?(terrence)
Flags: needinfo?(lhansen)

Comment 3

4 years ago
(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.
Flags: needinfo?(lhansen)
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
Created attachment 8450907 [details] [diff] [review]
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
Assignee: nobody → emanuel.hoogeveen
Attachment #8450818 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8450818 - Flags: review?(terrence)
Attachment #8450907 - Flags: review?(terrence)
(Assignee)

Comment 6

4 years ago
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 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.
Attachment #8450907 - Flags: review?(terrence) → review+
(Assignee)

Comment 8

4 years ago
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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f81fe2b04fef
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f81fe2b04fef
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Updated

4 years ago
Blocks: 1037645
You need to log in before you can comment on or make changes to this bug.