Closed Bug 869235 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v0 (obsolete) — Splinter Review
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.
Attachment #746128 - Flags: review?(wmccloskey)
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.
Attachment #746128 - Flags: review?(wmccloskey)
Attached patch v1Splinter Review
Attachment #746128 - Attachment is obsolete: true
Attachment #746692 - Flags: review?(wmccloskey)
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."
Attachment #746692 - Flags: review?(wmccloskey) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 883472
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: