888463 - GC: relocate hoisted elements/slots when their allocation moves during minor GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

If a minor GC happens from an operation callback inside a loop where the slot or element pointer has been hoisted above the loop-edge, we do not re-load the slot or element pointer after the GC. This leaves us pointing at freed slots/elements after the MinorGC.

Comment 1

[Terrence Cole [:terrence]]

Attachment: v0: implement slot/element relocation support in the Nursery

I think we don't actually need to worry about relocating zero-size slots or elements pointers at all. If we do hoist one of these, then any read or write to this pointer it is going to be invalid anyway. Thus, if we just skip writing to them we can avoid poisoning anything we don't want to. Conversely, if we read garbage and update the hoisted pointer with it, then presumably we'll never actually dereference it because the pointer has zero length.

The only awkward case is for ArrayBufferObject. In that case the size is in bytes, so if we get one with a size > 0, but < sizeof(HeapSlot), then we'll not store the forwarding pointer, but it will still be valid to read and write after hoisting. This patch doesn't handle that case, but Steve should be able to tell us where we need to intercept in the typedarray implementation to fix it.

Jan, I think you'll just need to update forwardMovedBuffers to walk whatever structure you implement.

Comment 2

[Steve Fink [:sfink] [:s:]]

So... this isn't pretty. You can get an ArrayBufferObject with an arbitrary number of elements by creating one with a byteLength of eg 3, which will produce an inlined ArrayBuffer, and then cause it to be un-inlined.

However, it seems like ObjectImpl has a notion of SLOT_CAPACITY_MIN, so one of these tiny external ArrayBuffers is already busted. It would return the wrong thing for dynamicSlotsCount(), for example.

The elements header stores both capacity and initializedLength values normally, so it seems like it'd be straightforward to overallocate and set capacity to the allocated size and initializedLength to the in-use size (ArrayBuffer.byteLength).

The bad news: ArrayBuffer uses the elements header for its own purposes, which means it only has an initializedLength field. Good news: it also has byteLength stored in a fixed slot, so it normally shouldn't need the initializedLength value. Bad news: "normally shouldn't" != "never does".

Yeuurgh.

I'll have to look further into this next week.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 769231 [details] [diff] [review]
v0: implement slot/element relocation support in the Nursery

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

Nice! Good thinking about the zero-size slots/elements case. The patch in bug 888872 is based on this and seems to work fine.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 769231 [details] [diff] [review]
v0: implement slot/element relocation support in the Nursery

With the patch in bug 888872 on top, this passes Kraken. Flagging for review.

Comment 5

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

Comment on attachment 769231 [details] [diff] [review]
v0: implement slot/element relocation support in the Nursery

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

::: js/src/gc/Nursery.cpp
@@ +303,5 @@
> +    /*
> +     * If the JIT has hoisted a zero length pointer, then we do not need to
> +     * relocate it because reads and writes to/from this pointer are invalid.
> +     */
> +    if (nslots < 1)

Please remove the comment and assert nslots > 0 here. We should be able to ensure that at least.

@@ +330,5 @@
> +js::Nursery::forwardBufferPointer(HeapSlot **pSlotsElems)
> +{
> +    HeapSlot *old = *pSlotsElems;
> +    /*
> +     * A if the slots/elements buffer is zero length, the "first" item could be

Please change slots/elements to just elements.

Comment 6

[Terrence Cole [:terrence]]

Attachment: v0

Adding the nslots > 0 assertion hits the weird case where we skip shrinkslots for call objects. The comment indicates this is so that we can hoist slots for call objects in contexts where there is an eval that deletes slots. After discussing with Luke and Bill, we think this is not true now that JM is gone. Since IonBuilder::jsop_getaliasedvar uses MSlots::new, the same as every other hoistable slots pointer, it seems normal slots/elements hoisting would hit the same issue if it were still a problem.

For what it's worth, jit-tests all pass a ggc build with this and the prior patch.

Comment 7

[Terrence Cole [:terrence]]

Attachment: v1: Updated with review feedback.

Comment 8

[Jan de Mooij [:jandem]]

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

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

Yeah, Ion should be fine with this.

Comment 9

[Jan de Mooij [:jandem]]

Pushed these patches and the one in bug 888872 to try:

https://tbpl.mozilla.org/?tree=Try&rev=bd432bd89925

Comment 10

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/193dc4224534
https://hg.mozilla.org/integration/mozilla-inbound/rev/f119f52bfa1e

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/193dc4224534
https://hg.mozilla.org/mozilla-central/rev/f119f52bfa1e