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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file, 1 obsolete file)
4.07 KB,
patch
|
billm
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 2•12 years ago
|
||
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+
Assignee | ||
Comment 4•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•