Closed Bug 852802 Opened 7 years ago Closed 7 years ago

Add incremental needsBarrier to the runtime and and check it first

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

Attached patch v0Splinter Review
This lets us squelch pre-barriers before they access the zone pointer.

This fixes two hard problems:
(1) With generational GC, objects will get their Zone* by looking at Shape's arena header. During GC this shape may already have been finalized, so making this access is bad.
(2) When doing a minor collection, marking a key in a hash table may find a moved object. The ::remove function of all of our hash tables correctly calls the pre-barrier. Since the pre-barrier does not know about relocations, it will look through a broken Shape* for the arena header.

Currently we hack around these behaviors manually in all the places that happen to hit these cases by either doing a reinterpret_cast to change to an unbarriered type for the relevant accesses or calling a special method on the hash table.

My measurements show no change in performance from this patch.
Attachment #727002 - Flags: review?(wmccloskey)
Assignee: general → terrence
Blocks: 854051
Comment on attachment 727002 [details] [diff] [review]
v0

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

::: js/public/HeapAPI.h
@@ +101,5 @@
> +GetGCThingRuntime(const void *thing)
> +{
> +    uintptr_t addr = uintptr_t(thing);
> +    addr &= ~js::gc::ChunkMask;
> +    addr += js::gc::ChunkSize - sizeof(JS::shadow::Runtime *);

Could you instead create a ChunkRuntimeOffset (similar to ChunkMarkBitmapOffset) and use that here? Also, you should use |= instead of += in case the compiler fails to optimize it. Finally, please assert in gc/Heap.h that the runtime actually is at ChunkRuntimeOffset (similar to what we do for ChunkMarkBitmapOffset).

::: js/src/jsgc.cpp
@@ +3239,5 @@
>  #endif
>  }
>  
>  static void
> +AssertNeedsBarriersAreConsistent(JSRuntime *rt)

How about AssertNeedsBarrierFlagsConsistent?

::: js/src/jspubtd.h
@@ +312,2 @@
>      {
> +        bool needsBarrier_;

This isn't good. I think you should be able to move JS::shadow::Runtime to this file and include jspubtd.h from gc/HeapAPI.h. Then you can make RuntimeDummy inherit from JS::shadow::Runtime.
Attachment #727002 - Flags: review?(wmccloskey) → review+
Backed out for build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/46b8016553fa

https://tbpl.mozilla.org/php/getParsedLog.php?id=21313754&tree=Mozilla-Inbound

../../../js/src/jsgc.cpp:3202:52: error: 'void js::AssertNeedsBarrierFlagsConsistent(JSRuntime*)' should have been declared inside 'js'
Bustage is from removal of Try assertion. Trivial fix. Re-re-pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a95fe57e30de
https://hg.mozilla.org/mozilla-central/rev/a95fe57e30de
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.