GC: post-allocate slot data

RESOLVED FIXED in mozilla22

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Currently, we pre-allocate slot data on several of the less-used object-creation paths. This sometimes ends up making the nursery track an extra external slots pointer that we could have easily allocated directly in the nursery. This patch makes the slot allocation happen after object creation.

This is also a nice cleanup. Without this patch it is quite easy to pass a |slots| that does not match the object's shape. It also keeps us from having to track and manually js_free the slots pointer in the two places that use this feature. It also removes about 200 lines of code.
(Assignee)

Comment 1

5 years ago
Created attachment 720986 [details] [diff] [review]
v0: Bugzilla ate my patch.

This is my second IonMonkey patch, so you may want to pay extra attention to those changes.
Attachment #720986 - Flags: review?(bhackett1024)
Comment on attachment 720986 [details] [diff] [review]
v0: Bugzilla ate my patch.

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

::: js/src/jsobjinlines.h
@@ +952,5 @@
> +    size_t nDynamicSlots = dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan());
> +    if (nDynamicSlots) {
> +        slots = cx->pod_malloc<js::HeapSlot>(nDynamicSlots);
> +        if (!slots)
> +            return NULL;

The reason we preallocate dynamic slots for an object is that if this malloc fails, ::create will leave a mutant object in existence whose slot span does not match its dynamic slots.  The GC will crash if it tries to mark this object.  This isn't a concern with exact marking as the return NULL here will ensure the object is well and truly dead, but with a conservative GC this is not guaranteed.  As such I think this patch needs (a little?) more work to avoid this issue.
Attachment #720986 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

5 years ago
Created attachment 721446 [details] [diff] [review]
v1

Uhg, that makes sense. It's slightly worse really: we have the same problem even for exact rooting because of CellIter. After discussing this will Bill at lunch, neither of us was able to think of anything particularly clean to do here. For now lets just continue pre-allocating, but move it right before the js_NewGCObject so that we can keep all the nice aspects of this patch. For now I can just continue with my existing workarounds for GGC.
Attachment #720986 - Attachment is obsolete: true
Attachment #721446 - Flags: review?(bhackett1024)
(Assignee)

Comment 4

5 years ago
Created attachment 721463 [details] [diff] [review]
v2

This time remembering that we once again need to not to leak the slots pointer.
Attachment #721446 - Attachment is obsolete: true
Attachment #721446 - Flags: review?(bhackett1024)
Attachment #721463 - Flags: review?(bhackett1024)
Comment on attachment 721463 [details] [diff] [review]
v2

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

::: js/src/ion/CodeGenerator.cpp
@@ -2445,5 @@
>      masm.newGCThing(obj, templateObj, ool->entry());
>      masm.initGCThing(obj, templateObj);
>  
> -    if (lir->slots()->isRegister())
> -        masm.storePtr(ToRegister(lir->slots()), Address(obj, JSObject::offsetOfSlots()));

Sorry, I should have looked at the first patch more and noticed this then, but I don't think these Ion changes are valid.  By removing this call, the call object created by this MIR node will have a NULL dynamic slots pointer, even if the call object requires one, so that code will crash if it tries to access those properties stored in the dynamic slots.  All the Ion machinery being removed is in support of this write, so I think all the Ion changes need to be reverted.  The only change I see as being necessary is that NewCallObject in VMFunctions.cpp should free() the slots pointer passed to it.  (It should be very unlikely that the OOL call will be performed; just when exhausting the active arena or, with GGC, the nursery.)

Now, I think this machinery was added for earley-boyer back when its call objects required dynamic slots; this is (I think) no longer the case since only aliased vars end up in call objects now.  In that case this stuff could be removable, provided controls were in place so that call objects which do require dynamic slots are handled correctly, but that should be done in a separate patch (talk to dvander for more).

Besides this the patch looks good.
Attachment #721463 - Flags: review?(bhackett1024)
(Assignee)

Comment 6

5 years ago
(In reply to Brian Hackett (:bhackett) from comment #5)
> Comment on attachment 721463 [details] [diff] [review]
> v2
> 
> Review of attachment 721463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/CodeGenerator.cpp
> @@ -2445,5 @@
> >      masm.newGCThing(obj, templateObj, ool->entry());
> >      masm.initGCThing(obj, templateObj);
> >  
> > -    if (lir->slots()->isRegister())
> > -        masm.storePtr(ToRegister(lir->slots()), Address(obj, JSObject::offsetOfSlots()));
> 
> Sorry, I should have looked at the first patch more and noticed this then,
> but I don't think these Ion changes are valid.  By removing this call, the
> call object created by this MIR node will have a NULL dynamic slots pointer,
> even if the call object requires one, so that code will crash if it tries to
> access those properties stored in the dynamic slots.  All the Ion machinery
> being removed is in support of this write, so I think all the Ion changes
> need to be reverted.

Yeah, sorry, I /totally/ misunderstood the what the ool->rejoin was doing.

> The only change I see as being necessary is that
> NewCallObject in VMFunctions.cpp should free() the slots pointer passed to
> it.  (It should be very unlikely that the OOL call will be performed; just
> when exhausting the active arena or, with GGC, the nursery.)

Right, this case only hits with --ion-eager, and then only in 5 tests: this is why I missed the brokenness when testing initially. We could also make JSObject::create take a default NULL slots param. I'll see what looks better.
(Assignee)

Comment 7

5 years ago
Created attachment 722561 [details] [diff] [review]
v3: A more modest implementation.

This passes all tests with --ion-eager.
Attachment #721463 - Attachment is obsolete: true
Attachment #722561 - Flags: review?(bhackett1024)
Comment on attachment 722561 [details] [diff] [review]
v3: A more modest implementation.

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

::: js/src/jsobjinlines.h
@@ +939,5 @@
>      JS_ASSERT(cx->compartment == type->compartment());
>      JS_ASSERT_IF(type->clasp->flags & JSCLASS_BACKGROUND_FINALIZE, IsBackgroundFinalized(kind));
>      JS_ASSERT_IF(type->clasp->finalize, heap == js::gc::TenuredHeap);
> +    JS_ASSERT_IF(extantSlots,
> +                 !!dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan()) == !!extantSlots);

This can just be JS_ASSERT_IF(extantSlots, dynamicSlotsCount(...))

::: js/src/vm/ScopeObject.cpp
@@ +146,5 @@
>      JS_ASSERT(CanBeFinalizedInBackground(kind, &CallClass));
>      kind = gc::GetBackgroundAllocKind(kind);
>  
>      JSObject *obj = JSObject::create(cx, kind, GetInitialHeap(GenericObject, &CallClass),
> +                                     shape, type);

This function should pass in 'slots' here.
Attachment #722561 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 9

5 years ago
The whole point of the last change I made and I forgot to pass the slots through. *facepalm*

Thanks for the catch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d09c1509a43
https://hg.mozilla.org/mozilla-central/rev/0d09c1509a43
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.