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

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 769231 [details] [diff] [review]
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.
Attachment #769231 - Flags: feedback?(jdemooij)
Flags: needinfo?(sphink)
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.

Updated

5 years ago
Blocks: 888872
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.
Attachment #769231 - Flags: feedback?(jdemooij) → feedback+
(Assignee)

Comment 4

5 years ago
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.
Attachment #769231 - Flags: review?(wmccloskey)
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.
Attachment #769231 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 770308 [details] [diff] [review]
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.
Attachment #770308 - Flags: review?(jdemooij)
(Assignee)

Comment 7

5 years ago
Created attachment 770312 [details] [diff] [review]
v1: Updated with review feedback.
Attachment #769231 - Attachment is obsolete: true
Attachment #770312 - Flags: review+
Comment on attachment 770308 [details] [diff] [review]
v0

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

Yeah, Ion should be fine with this.
Attachment #770308 - Flags: review?(jdemooij) → review+
Pushed these patches and the one in bug 888872 to try:

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

Updated

5 years ago
Depends on: 889986
https://hg.mozilla.org/mozilla-central/rev/193dc4224534
https://hg.mozilla.org/mozilla-central/rev/f119f52bfa1e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Updated

4 years ago
Flags: needinfo?(sphink)
You need to log in before you can comment on or make changes to this bug.