Closed Bug 1343986 Opened 3 years ago Closed 3 years ago

Full GCs don't set gray bits valid flag if helper thread zones are present

Categories

(Core :: JavaScript: GC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

The CC can run a GC beforehand to make sure that the gray bits are set correctly.  We're supposed to set a flag when this happens, but this flag is not set if helper thread zones are present because the GC sees that we are not collecting all zones and concludes that we haven't performed a full GC.

In reality this probably fine.  I don't think anything in a helper thread zone is ever reachable from the main heap so the CC doesn't need gray bits to be correct in these zones.
Here's a patch to set the 'gray bits are valid' flag even if we don't perform a full GC.

As described in the comments we ignore helper thread zones, empty zones and the atoms zone.

The purpose of this is so that when the CC requests a full GC because the gray marking state is invalid, we do end up with the flag set at the end.
Assignee: nobody → jcoppeard
Attachment #8843353 - Flags: review?(sphink)
Comment on attachment 8843353 [details] [diff] [review]
bug1343986-gray-bits-valid-flag

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

Makes sense to me. If there were gray cycles going through a helper zone, I think it would mean we're messing up our thread safety anyway. :-)

::: js/src/jsgc.cpp
@@ +5440,5 @@
>      }
>  }
>  
> +bool
> +GCRuntime::grayBitsHaveBecomeValid() const

I wonder if this should be allGrayableZonesWereCollected (if you'll forgive the mangling of the English language)?

@@ +5461,5 @@
> +    for (ZonesIter zone(rt, WithAtoms); !zone.done(); zone.next()) {
> +        if (!zone->isCollecting() &&
> +            !zone->usedByHelperThread() &&
> +            !zone->isAtomsZone() &&
> +            !zone->arenas.arenaListsAreEmpty())

Do you prefer it this way, or SkipAtoms and a shorter list of conditionals? (I'm fine either way.)
Attachment #8843353 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> I wonder if this should be allGrayableZonesWereCollected (if you'll forgive
> the mangling of the English language)?

I'm going to go with 'allCCVisibleZonesWereCollected'.

> Do you prefer it this way, or SkipAtoms and a shorter list of conditionals?
> (I'm fine either way.)

Ugh, yeah it's a bit strange to include the atoms zone only to explicitly disallow it.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2caae037c26
Fix setting of gray bits valid flag after GC r=sfink
https://hg.mozilla.org/mozilla-central/rev/b2caae037c26
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Is this worth backporting?
Flags: needinfo?(jcoppeard)
This might mean that we do some extra GCs before CCing that aren't strictly necessary.  I don't think it's worth backporting.
Flags: needinfo?(jcoppeard)
You need to log in before you can comment on or make changes to this bug.