852802 - Add incremental needsBarrier to the runtime and and check it first

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

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.

Comment 1

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

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.

Comment 2

[Terrence Cole [:terrence]]

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

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

Comment 3

[Terrence Cole [:terrence]]

Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b839ca9cf702

Comment 4

[Terrence Cole [:terrence]]

Bill spotted the bug. Relanded in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6a4d28e510

(Full) green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=b44adec02570

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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'

Comment 6

[Terrence Cole [:terrence]]

Bustage is from removal of Try assertion. Trivial fix. Re-re-pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a95fe57e30de

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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