Bug 1476239 - Improve assertions around saved slots/elements ranges r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Thu, 26 Jul 2018 15:33:46 +0100
changeset 428595 246e495c481ee0fb320de0265027b2e18efd785d
parent 428594 d8b161307dedb285d72befa22f649c12f795d46f
child 428596 4545c5afeb75244bc89c8a7f6405f105b3c6ecf5
push id34337
push userncsoregi@mozilla.com
push dateThu, 26 Jul 2018 21:58:45 +0000
treeherdermozilla-central@8f2f847b2f9d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1476239
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1476239 - Improve assertions around saved slots/elements ranges r=sfink
js/src/gc/GCMarker.h
js/src/gc/Marking.cpp
--- a/js/src/gc/GCMarker.h
+++ b/js/src/gc/GCMarker.h
@@ -102,25 +102,27 @@ class MarkStack
         JSRope* asTempRope() const;
 
         void assertValid() const;
     };
 
     struct ValueArray
     {
         ValueArray(JSObject* obj, HeapSlot* start, HeapSlot* end);
+        void assertValid() const;
 
         HeapSlot* end;
         HeapSlot* start;
         TaggedPtr ptr;
     };
 
     struct SavedValueArray
     {
         SavedValueArray(JSObject* obj, size_t index, HeapSlot::Kind kind);
+        void assertValid() const;
 
         uintptr_t kind;
         uintptr_t index;
         TaggedPtr ptr;
     };
 
     explicit MarkStack(size_t maxCapacity = DefaultCapacity);
     ~MarkStack();
@@ -216,17 +218,17 @@ class MarkStackIter
     MarkStack::Tag peekTag() const;
     MarkStack::TaggedPtr peekPtr() const;
     MarkStack::ValueArray peekValueArray() const;
     void next();
     void nextPtr();
     void nextArray();
 
     // Mutate the current ValueArray to a SavedValueArray.
-    void saveValueArray(NativeObject* obj, uintptr_t index, HeapSlot::Kind kind);
+    void saveValueArray(const MarkStack::SavedValueArray& savedArray);
 
   private:
     size_t position() const;
 };
 
 } /* namespace gc */
 
 class GCMarker : public JSTracer
@@ -350,17 +352,21 @@ class GCMarker : public JSTracer
     inline void pushValueArray(JSObject* obj, HeapSlot* start, HeapSlot* end);
 
     bool isMarkStackEmpty() {
         return stack.isEmpty();
     }
 
     MOZ_MUST_USE bool restoreValueArray(const gc::MarkStack::SavedValueArray& array,
                                         HeapSlot** vpp, HeapSlot** endp);
+    gc::MarkStack::ValueArray restoreValueArray(const gc::MarkStack::SavedValueArray& savedArray);
+
     void saveValueRanges();
+    gc::MarkStack::SavedValueArray saveValueRange(const gc::MarkStack::ValueArray& array);
+
     inline void processMarkStackTop(SliceBudget& budget);
 
     /* The mark stack. Pointers in this stack are "gray" in the GC sense. */
     gc::MarkStack stack;
 
     /* The color is only applied to objects and functions. */
     MainThreadData<gc::MarkColor> color;
 
--- a/js/src/gc/Marking.cpp
+++ b/js/src/gc/Marking.cpp
@@ -1792,112 +1792,131 @@ GCMarker::processMarkStackTop(SliceBudge
 /*
  * During incremental GC, we return from drainMarkStack without having processed
  * the entire stack. At that point, JS code can run and reallocate slot arrays
  * that are stored on the stack. To prevent this from happening, we replace all
  * ValueArrayTag stack items with SavedValueArrayTag. In the latter, slots
  * pointers are replaced with slot indexes, and slot array end pointers are
  * replaced with the kind of index (properties vs. elements).
  */
+
 void
 GCMarker::saveValueRanges()
 {
     MarkStackIter iter(stack);
     while (!iter.done()) {
         auto tag = iter.peekTag();
         if (tag == MarkStack::ValueArrayTag) {
-            auto array = iter.peekValueArray();
-
-            NativeObject* obj = &array.ptr.asValueArrayObject()->as<NativeObject>();
-            MOZ_ASSERT(obj->isNative());
-
-            uintptr_t index;
-            HeapSlot::Kind kind;
-            HeapSlot* vp = obj->getDenseElementsAllowCopyOnWrite();
-            if (array.end == vp + obj->getDenseInitializedLength()) {
-                MOZ_ASSERT(array.start >= vp);
-                // Add the number of shifted elements here (and subtract in
-                // restoreValueArray) to ensure shift() calls on the array
-                // are handled correctly.
-                index = obj->unshiftedIndex(array.start - vp);
-                kind = HeapSlot::Element;
-            } else {
-                HeapSlot* vp = obj->fixedSlots();
-                unsigned nfixed = obj->numFixedSlots();
-                if (array.start == array.end) {
-                    index = obj->slotSpan();
-                } else if (array.start >= vp && array.start < vp + nfixed) {
-                    MOZ_ASSERT(array.end == vp + Min(nfixed, obj->slotSpan()));
-                    index = array.start - vp;
-                } else {
-                    MOZ_ASSERT(array.start >= obj->slots_ &&
-                               array.end == obj->slots_ + obj->slotSpan() - nfixed);
-                    index = (array.start - obj->slots_) + nfixed;
-                }
-                kind = HeapSlot::Slot;
-            }
-            iter.saveValueArray(obj, index, kind);
+            const auto& array = iter.peekValueArray();
+            auto savedArray = saveValueRange(array);
+            iter.saveValueArray(savedArray);
             iter.nextArray();
         } else if (tag == MarkStack::SavedValueArrayTag) {
             iter.nextArray();
         } else {
             iter.nextPtr();
         }
     }
 
     // This is also a convenient point to poison unused stack memory.
     stack.poisonUnused();
 }
 
 bool
-GCMarker::restoreValueArray(const MarkStack::SavedValueArray& array,
+GCMarker::restoreValueArray(const MarkStack::SavedValueArray& savedArray,
                             HeapSlot** vpp, HeapSlot** endp)
 {
-    JSObject* objArg = array.ptr.asSavedValueArrayObject();
+    JSObject* objArg = savedArray.ptr.asSavedValueArrayObject();
     if (!objArg->isNative())
         return false;
-    NativeObject* obj = &objArg->as<NativeObject>();
-
-    uintptr_t start = array.index;
-    if (array.kind == HeapSlot::Element) {
+
+    auto array = restoreValueArray(savedArray);
+    *vpp = array.start;
+    *endp = array.end;
+    return true;
+}
+
+MarkStack::SavedValueArray
+GCMarker::saveValueRange(const MarkStack::ValueArray& array)
+{
+    NativeObject* obj = &array.ptr.asValueArrayObject()->as<NativeObject>();
+    MOZ_ASSERT(obj->isNative());
+
+    uintptr_t index;
+    HeapSlot::Kind kind;
+    HeapSlot* vp = obj->getDenseElementsAllowCopyOnWrite();
+    if (array.end == vp + obj->getDenseInitializedLength()) {
+        MOZ_ASSERT(array.start >= vp);
+        // Add the number of shifted elements here (and subtract in
+        // restoreValueArray) to ensure shift() calls on the array
+        // are handled correctly.
+        index = obj->unshiftedIndex(array.start - vp);
+        kind = HeapSlot::Element;
+    } else {
+        HeapSlot* vp = obj->fixedSlots();
+        unsigned nfixed = obj->numFixedSlots();
+        if (array.start == array.end) {
+            index = obj->slotSpan();
+        } else if (array.start >= vp && array.start < vp + nfixed) {
+            MOZ_ASSERT(array.end == vp + Min(nfixed, obj->slotSpan()));
+            index = array.start - vp;
+        } else {
+            MOZ_ASSERT(array.start >= obj->slots_ &&
+                       array.end == obj->slots_ + obj->slotSpan() - nfixed);
+            index = (array.start - obj->slots_) + nfixed;
+        }
+        kind = HeapSlot::Slot;
+    }
+
+    return MarkStack::SavedValueArray(obj, index, kind);
+}
+
+MarkStack::ValueArray
+GCMarker::restoreValueArray(const MarkStack::SavedValueArray& savedArray)
+{
+    NativeObject* obj = &savedArray.ptr.asSavedValueArrayObject()->as<NativeObject>();
+    HeapSlot* start = nullptr;
+    HeapSlot* end = nullptr;
+
+    uintptr_t index = savedArray.index;
+    if (savedArray.kind == HeapSlot::Element) {
         uint32_t initlen = obj->getDenseInitializedLength();
 
         // Account for shifted elements.
         uint32_t numShifted = obj->getElementsHeader()->numShiftedElements();
-        start = (numShifted < start) ? start - numShifted : 0;
+        index = (numShifted < index) ? index - numShifted : 0;
 
         HeapSlot* vp = obj->getDenseElementsAllowCopyOnWrite();
-        if (start < initlen) {
-            *vpp = vp + start;
-            *endp = vp + initlen;
+        if (index < initlen) {
+            start = vp + index;
+            end = vp + initlen;
         } else {
             /* The object shrunk, in which case no scanning is needed. */
-            *vpp = *endp = vp;
+            start = end = vp;
         }
     } else {
-        MOZ_ASSERT(array.kind == HeapSlot::Slot);
+        MOZ_ASSERT(savedArray.kind == HeapSlot::Slot);
         HeapSlot* vp = obj->fixedSlots();
         unsigned nfixed = obj->numFixedSlots();
         unsigned nslots = obj->slotSpan();
-        if (start < nslots) {
-            if (start < nfixed) {
-                *vpp = vp + start;
-                *endp = vp + Min(nfixed, nslots);
+        if (index < nslots) {
+            if (index < nfixed) {
+                start = vp + index;
+                end = vp + Min(nfixed, nslots);
             } else {
-                *vpp = obj->slots_ + start - nfixed;
-                *endp = obj->slots_ + nslots - nfixed;
+                start = obj->slots_ + index - nfixed;
+                end = obj->slots_ + nslots - nfixed;
             }
         } else {
             /* The object shrunk, in which case no scanning is needed. */
-            *vpp = *endp = vp;
+            start = end = vp;
         }
     }
 
-    MOZ_ASSERT(*vpp <= *endp);
-    return true;
+    return MarkStack::ValueArray(obj, start, end);
 }
 
 
 /*** Mark Stack ***********************************************************************************/
 
 static_assert(sizeof(MarkStack::TaggedPtr) == sizeof(uintptr_t),
               "A TaggedPtr should be the same size as a pointer");
 static_assert(sizeof(MarkStack::ValueArray) == sizeof(MarkStack::SavedValueArray),
@@ -1919,33 +1938,16 @@ template <>
 struct MapTypeToMarkStackTag<JSScript*> { static const auto value = MarkStack::ScriptTag; };
 
 static inline bool
 TagIsArrayTag(MarkStack::Tag tag)
 {
     return tag == MarkStack::ValueArrayTag || tag == MarkStack::SavedValueArrayTag;
 }
 
-static inline void
-CheckValueArray(const MarkStack::ValueArray& array)
-{
-    array.ptr.assertValid();
-    MOZ_ASSERT(array.ptr.tag() == MarkStack::ValueArrayTag);
-    MOZ_ASSERT(uintptr_t(array.start) <= uintptr_t(array.end));
-    MOZ_ASSERT((uintptr_t(array.end) - uintptr_t(array.start)) % sizeof(Value) == 0);
-}
-
-static inline void
-CheckSavedValueArray(const MarkStack::SavedValueArray& array)
-{
-    array.ptr.assertValid();
-    MOZ_ASSERT(array.ptr.tag() == MarkStack::SavedValueArrayTag);
-    MOZ_ASSERT(array.kind == HeapSlot::Slot || array.kind == HeapSlot::Element);
-}
-
 inline
 MarkStack::TaggedPtr::TaggedPtr(Tag tag, Cell* ptr)
   : bits(tag | uintptr_t(ptr))
 {
     assertValid();
 }
 
 inline MarkStack::Tag
@@ -2003,22 +2005,45 @@ MarkStack::TaggedPtr::asTempRope() const
     MOZ_ASSERT(tag() == TempRopeTag);
     MOZ_ASSERT(ptr()->is<JSString>());
     return static_cast<JSRope*>(ptr());
 }
 
 inline
 MarkStack::ValueArray::ValueArray(JSObject* obj, HeapSlot* startArg, HeapSlot* endArg)
   : end(endArg), start(startArg), ptr(ValueArrayTag, obj)
-{}
+{
+    assertValid();
+}
+
+inline void
+MarkStack::ValueArray::assertValid() const
+{
+    ptr.assertValid();
+    MOZ_ASSERT(ptr.tag() == MarkStack::ValueArrayTag);
+    MOZ_ASSERT(start);
+    MOZ_ASSERT(end);
+    MOZ_ASSERT(uintptr_t(start) <= uintptr_t(end));
+    MOZ_ASSERT((uintptr_t(end) - uintptr_t(start)) % sizeof(Value) == 0);
+}
 
 inline
 MarkStack::SavedValueArray::SavedValueArray(JSObject* obj, size_t indexArg, HeapSlot::Kind kindArg)
   : kind(kindArg), index(indexArg), ptr(SavedValueArrayTag, obj)
-{}
+{
+    assertValid();
+}
+
+inline void
+MarkStack::SavedValueArray::assertValid() const
+{
+    ptr.assertValid();
+    MOZ_ASSERT(ptr.tag() == MarkStack::SavedValueArrayTag);
+    MOZ_ASSERT(kind == HeapSlot::Slot || kind == HeapSlot::Element);
+}
 
 MarkStack::MarkStack(size_t maxCapacity)
   : topIndex_(0)
   , maxCapacity_(maxCapacity)
 #ifdef DEBUG
   , iteratorCount_(0)
 #endif
 {}
@@ -2115,32 +2140,32 @@ inline bool
 MarkStack::push(JSObject* obj, HeapSlot* start, HeapSlot* end)
 {
     return push(ValueArray(obj, start, end));
 }
 
 inline bool
 MarkStack::push(const ValueArray& array)
 {
-    CheckValueArray(array);
+    array.assertValid();
 
     if (!ensureSpace(ValueArrayWords))
         return false;
 
     *reinterpret_cast<ValueArray*>(topPtr()) = array;
     topIndex_ += ValueArrayWords;
     MOZ_ASSERT(position() <= capacity());
     MOZ_ASSERT(peekTag() == ValueArrayTag);
     return true;
 }
 
 inline bool
 MarkStack::push(const SavedValueArray& array)
 {
-    CheckSavedValueArray(array);
+    array.assertValid();
 
     if (!ensureSpace(ValueArrayWords))
         return false;
 
     *reinterpret_cast<SavedValueArray*>(topPtr()) = array;
     topIndex_ += ValueArrayWords;
     MOZ_ASSERT(position() <= capacity());
     MOZ_ASSERT(peekTag() == SavedValueArrayTag);
@@ -2172,29 +2197,29 @@ MarkStack::popPtr()
 inline MarkStack::ValueArray
 MarkStack::popValueArray()
 {
     MOZ_ASSERT(peekTag() == ValueArrayTag);
     MOZ_ASSERT(position() >= ValueArrayWords);
 
     topIndex_ -= ValueArrayWords;
     const auto& array = *reinterpret_cast<ValueArray*>(topPtr());
-    CheckValueArray(array);
+    array.assertValid();
     return array;
 }
 
 inline MarkStack::SavedValueArray
 MarkStack::popSavedValueArray()
 {
     MOZ_ASSERT(peekTag() == SavedValueArrayTag);
     MOZ_ASSERT(position() >= ValueArrayWords);
 
     topIndex_ -= ValueArrayWords;
     const auto& array = *reinterpret_cast<SavedValueArray*>(topPtr());
-    CheckSavedValueArray(array);
+    array.assertValid();
     return array;
 }
 
 inline bool
 MarkStack::ensureSpace(size_t count)
 {
     if ((topIndex_ + count) <= capacity())
         return !js::oom::ShouldFailWithOOM();
@@ -2286,17 +2311,17 @@ MarkStackIter::peekTag() const
 inline MarkStack::ValueArray
 MarkStackIter::peekValueArray() const
 {
     MOZ_ASSERT(peekTag() == MarkStack::ValueArrayTag);
     MOZ_ASSERT(position() >= ValueArrayWords);
 
     const MarkStack::TaggedPtr* ptr = &stack_.stack()[pos_ - ValueArrayWords];
     const auto& array = *reinterpret_cast<const MarkStack::ValueArray*>(ptr);
-    CheckValueArray(array);
+    array.assertValid();
     return array;
 }
 
 inline void
 MarkStackIter::nextPtr()
 {
     MOZ_ASSERT(!done());
     MOZ_ASSERT(!TagIsArrayTag(peekTag()));
@@ -2316,26 +2341,25 @@ inline void
 MarkStackIter::nextArray()
 {
     MOZ_ASSERT(TagIsArrayTag(peekTag()));
     MOZ_ASSERT(position() >= ValueArrayWords);
     pos_ -= ValueArrayWords;
 }
 
 void
-MarkStackIter::saveValueArray(NativeObject* obj, uintptr_t index, HeapSlot::Kind kind)
+MarkStackIter::saveValueArray(const MarkStack::SavedValueArray& savedArray)
 {
     MOZ_ASSERT(peekTag() == MarkStack::ValueArrayTag);
-    MOZ_ASSERT(peekPtr().asValueArrayObject() == obj);
+    MOZ_ASSERT(peekPtr().asValueArrayObject() == savedArray.ptr.asSavedValueArrayObject());
     MOZ_ASSERT(position() >= ValueArrayWords);
 
     MarkStack::TaggedPtr* ptr = &stack_.stack()[pos_ - ValueArrayWords];
-    auto& array = *reinterpret_cast<MarkStack::SavedValueArray*>(ptr);
-    array = MarkStack::SavedValueArray(obj, index, kind);
-    CheckSavedValueArray(array);
+    auto dest = reinterpret_cast<MarkStack::SavedValueArray*>(ptr);
+    *dest = savedArray;
     MOZ_ASSERT(peekTag() == MarkStack::SavedValueArrayTag);
 }
 
 
 /*** GCMarker *************************************************************************************/
 
 /*
  * ExpandWeakMaps: the GC is recomputing the liveness of WeakMap entries by
@@ -2431,16 +2455,20 @@ GCMarker::pushTaggedPtr(T* ptr)
     if (!stack.push(ptr))
         delayMarkingChildren(ptr);
 }
 
 void
 GCMarker::pushValueArray(JSObject* obj, HeapSlot* start, HeapSlot* end)
 {
     checkZone(obj);
+
+    if (start == end)
+        return;
+
     if (!stack.push(obj, start, end))
         delayMarkingChildren(obj);
 }
 
 void
 GCMarker::repush(JSObject* obj)
 {
     MOZ_ASSERT_IF(markColor() == MarkColor::Gray, gc::TenuredCell::fromPointer(obj)->isMarkedGray());