869235 - Don't trigger the global's readbarrier when marking during MinorGC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

The read barrier asserts HeapIsIdle, so this crashes instantly. I have no idea why so few tests hit this in practice.

If you have any ideas for how to make this more elegant, I would love to hear them.

Comment 1

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

Comment on attachment 746128 [details] [diff] [review]
v0

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

I think the real problem is that the rt->needsBarrier() state gets out of sync with zone->needsBarrier() during incremental GC. This is fine in most cases because we check rt->needsBarrier() first. However, for the read barrier for objects, we don't. Therefore, we're still invoking the barrier during minor collection.

I think we should keep them in sync. Then this problem would go away. You can do that by adding a new field to the zone called savedNeedsBarrier or something. Then you can set zone->needsBarrier_ to false when starting a minor collection and restore it afterwards. We don't want to throw any code away, so pass Zone::DontUpdateIon for the second param to zone->setNeedsBarrier when setting and restoring.

The alternative would be to add a check for rt->needsBarrier in the read barrier. However, that seems really error-prone to me. Someone could just add another use that's broken.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v1

Comment 3

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

Comment on attachment 746692 [details] [diff] [review]
v1

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

::: js/src/gc/Nursery.cpp
@@ +180,5 @@
>      RelocationOverlay *head;
>      RelocationOverlay **tail;
>  
> +    /* Save and restore all of the runtime and zone state we use during MinorGC. */
> +    bool runtimePriorNeedsBarrier;

Please just call this "savedNeedsBarrier" like the other one. It should be clear enough that it's for the runtime.

@@ +249,5 @@
>      if (!t) {
>          zone->allocator.arenas.checkEmptyFreeList(thingKind);
>          t = zone->allocator.arenas.allocateFromArena(zone, thingKind);
>      }
> +    if (zone->savedNeedsBarrier())

Please add a comment here to say that, while we want write barriers to be disabled during minor collection, we do want objects to be allocated black if an incremental GC is in progress.

::: js/src/gc/Zone.h
@@ +113,5 @@
>  
>    private:
>      bool                         ionUsingBarriers_;
> +
> +    bool                         savedNeedsBarrier_;

Can you add a comment here? "This flag saves the value of needsBarrier_ during minor collection, since needsBarrier_ is always set to false during minor collection. Outside of minor collection, the value of savedNeedsBarrier_ is undefined."

Comment 4

[Terrence Cole [:terrence]]

Green try at:
https://tbpl.mozilla.org/?tree=Try&rev=ab2edeb50f5f

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf21050ee99d

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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