From 65cc03ef2aa56ce7875730ca9bb17851bbda99eb Mon Sep 17 00:00:00 2001 From: Ted Campbell Date: Mon, 29 Jan 2018 08:20:00 +0200 Subject: [PATCH] Bug 1433837 - Cleanup JSObject slots_ initialization r=jandem js::Allocate now only sets slots_ if nDynamicSlots is non-zero. This avoids spurious writes for other types and is now consistent with JIT code. MozReview-Commit-ID: 3spPMFj7Fxz --HG-- extra : histedit_source : f01b0e2c54c85ec6b21f42ed564e714d0366cae0 --- js/src/gc/Allocator.cpp | 8 +++++--- js/src/gc/Allocator.h | 3 ++- js/src/gc/Nursery.cpp | 7 +++++-- js/src/jit/MacroAssembler.cpp | 2 ++ js/src/jsobj.h | 6 ------ js/src/jsobjinlines.h | 6 ------ js/src/vm/ArrayObject-inl.h | 4 +++- js/src/vm/NativeObject-inl.h | 4 +++- js/src/vm/NativeObject.h | 6 ++++++ 9 files changed, 26 insertions(+), 20 deletions(-) diff --git a/js/src/gc/Allocator.cpp b/js/src/gc/Allocator.cpp index 53c7297f5c9b..99c74b51db0d 100644 --- a/js/src/gc/Allocator.cpp +++ b/js/src/gc/Allocator.cpp @@ -119,10 +119,12 @@ GCRuntime::tryNewTenuredObject(JSContext* cx, AllocKind kind, size_t thingSize, JSObject* obj = tryNewTenuredThing(cx, kind, thingSize); - if (obj) - obj->setInitialSlotsMaybeNonNative(slots); - else + if (obj) { + if (nDynamicSlots) + static_cast(obj)->initSlots(slots); + } else { js_free(slots); + } return obj; } diff --git a/js/src/gc/Allocator.h b/js/src/gc/Allocator.h index a5f2a6877cc1..81afe6fd2b0f 100644 --- a/js/src/gc/Allocator.h +++ b/js/src/gc/Allocator.h @@ -21,7 +21,8 @@ struct Class; // // Note that JSObject allocation must use the longer signature below that // includes slot, heap, and finalizer information in support of various -// object-specific optimizations. +// object-specific optimizations. If dynamic slots are requested they will be +// allocated and the pointer stored directly in |NativeObject::slots_|. template T* Allocate(JSContext* cx); diff --git a/js/src/gc/Nursery.cpp b/js/src/gc/Nursery.cpp index d537d23dd7c2..da68d149aa82 100644 --- a/js/src/gc/Nursery.cpp +++ b/js/src/gc/Nursery.cpp @@ -293,8 +293,11 @@ js::Nursery::allocateObject(JSContext* cx, size_t size, size_t nDynamicSlots, co } } - /* Always initialize the slots field to match the JIT behavior. */ - obj->setInitialSlotsMaybeNonNative(slots); + /* Store slots pointer directly in new object. If no dynamic slots were + * requested, caller must initialize slots_ field itself as needed. We + * don't know if the caller was a native object or not. */ + if (nDynamicSlots) + static_cast(obj)->initSlots(slots); TraceNurseryAlloc(obj, size); return obj; diff --git a/js/src/jit/MacroAssembler.cpp b/js/src/jit/MacroAssembler.cpp index d9fb6058dda2..9f6da111f29a 100644 --- a/js/src/jit/MacroAssembler.cpp +++ b/js/src/jit/MacroAssembler.cpp @@ -875,6 +875,8 @@ MacroAssembler::allocateObject(Register result, Register temp, gc::AllocKind all if (!nDynamicSlots) return freeListAllocate(result, temp, allocKind, fail); + // Only NativeObject can have nDynamicSlots > 0 and reach here. + callMallocStub(nDynamicSlots * sizeof(GCPtrValue), temp, fail); Label failAlloc; diff --git a/js/src/jsobj.h b/js/src/jsobj.h index 4bb3d618ba92..62e36b6ee823 100644 --- a/js/src/jsobj.h +++ b/js/src/jsobj.h @@ -154,12 +154,6 @@ class JSObject : public js::gc::Cell inline js::Shape* maybeShape() const; inline js::Shape* ensureShape(JSContext* cx); - // Set the initial slots and elements of an object. These pointers are only - // valid for native objects, but during initialization are set for all - // objects. For non-native objects, these must not be dynamically allocated - // pointers which leak when the non-native object finishes initialization. - inline void setInitialSlotsMaybeNonNative(js::HeapSlot* slots); - enum GenerateShape { GENERATE_NONE, GENERATE_SHAPE diff --git a/js/src/jsobjinlines.h b/js/src/jsobjinlines.h index 8882e046ef64..93f6f61375c7 100644 --- a/js/src/jsobjinlines.h +++ b/js/src/jsobjinlines.h @@ -397,12 +397,6 @@ SetNewObjectMetadata(JSContext* cx, T* obj) } // namespace js -inline void -JSObject::setInitialSlotsMaybeNonNative(js::HeapSlot* slots) -{ - static_cast(this)->slots_ = slots; -} - inline js::GlobalObject& JSObject::global() const { diff --git a/js/src/vm/ArrayObject-inl.h b/js/src/vm/ArrayObject-inl.h index 07e9e45aed2a..fd2fd5cf0d05 100644 --- a/js/src/vm/ArrayObject-inl.h +++ b/js/src/vm/ArrayObject-inl.h @@ -57,7 +57,9 @@ ArrayObject::createArrayInternal(JSContext* cx, gc::AllocKind kind, gc::InitialH ArrayObject* aobj = static_cast(obj); aobj->initGroup(group); aobj->initShape(shape); - // NOTE: Slots are created and assigned internally by Allocate. + // NOTE: Dynamic slots are created internally by Allocate. + if (!nDynamicSlots) + aobj->initSlots(nullptr); MOZ_ASSERT(clasp->shouldDelayMetadataBuilder()); cx->compartment()->setObjectPendingMetadata(cx, aobj); diff --git a/js/src/vm/NativeObject-inl.h b/js/src/vm/NativeObject-inl.h index 43e29eaaa474..b1f54d3e2a17 100644 --- a/js/src/vm/NativeObject-inl.h +++ b/js/src/vm/NativeObject-inl.h @@ -544,7 +544,9 @@ NativeObject::create(JSContext* cx, js::gc::AllocKind kind, js::gc::InitialHeap NativeObject* nobj = static_cast(obj); nobj->initGroup(group); nobj->initShape(shape); - // NOTE: Slots are created and assigned internally by Allocate. + // NOTE: Dynamic slots are created internally by Allocate. + if (!nDynamicSlots) + nobj->initSlots(nullptr); nobj->setEmptyElements(); if (clasp->hasPrivate()) diff --git a/js/src/vm/NativeObject.h b/js/src/vm/NativeObject.h index 8ac309cfde5a..1671ab1b7bc5 100644 --- a/js/src/vm/NativeObject.h +++ b/js/src/vm/NativeObject.h @@ -680,6 +680,12 @@ class NativeObject : public ShapedObject } public: + + /* Object allocation may directly initialize slots so this is public. */ + void initSlots(HeapSlot* slots) { + slots_ = slots; + } + static MOZ_MUST_USE bool generateOwnShape(JSContext* cx, HandleNativeObject obj, Shape* newShape = nullptr) {