Bug 1462540 - Remove NativeIterator::props_array (it's trivial to recalculate it when it's needed), and add a bunch of alignment assertions verifying the delicate memory layout of NativeIterator followed by the (only dynamically known number of) properties it iterates followed by the (only dynamically known number of) ReceiverGuards it uses. r=jandem
authorJeff Walden <jwalden@mit.edu>
Wed, 16 May 2018 23:24:13 -0700
changeset 418920 6ae525ee499f
parent 418919 7c45180cea08
child 418921 7d83c7de9404
push id103419
push userjwalden@mit.edu
push date2018-05-18 19:33 +0000
treeherdermozilla-inbound@7658d2d1e0d7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1462540
milestone62.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 1462540 - Remove NativeIterator::props_array (it's trivial to recalculate it when it's needed), and add a bunch of alignment assertions verifying the delicate memory layout of NativeIterator followed by the (only dynamically known number of) properties it iterates followed by the (only dynamically known number of) ReceiverGuards it uses. r=jandem
js/src/jit/CodeGenerator.cpp
js/src/vm/Iteration.cpp
js/src/vm/Iteration.h
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -9874,17 +9874,17 @@ CodeGenerator::visitIteratorMore(LIterat
     Address cursorEndAddr(outputScratch, offsetof(NativeIterator, props_end));
     masm.loadPtr(cursorAddr, temp);
     masm.branchPtr(Assembler::BelowOrEqual, cursorEndAddr, temp, &iterDone);
 
     // Get next string.
     masm.loadPtr(Address(temp, 0), temp);
 
     // Increase the cursor.
-    masm.addPtr(Imm32(sizeof(JSString*)), cursorAddr);
+    masm.addPtr(Imm32(sizeof(GCPtrFlatString)), cursorAddr);
 
     masm.tagValue(JSVAL_TYPE_STRING, temp, output);
     masm.jump(ool->rejoin());
 
     masm.bind(&iterDone);
     masm.moveValue(MagicValue(JS_NO_ITER_VALUE), output);
 
     masm.bind(ool->rejoin());
@@ -9918,18 +9918,19 @@ CodeGenerator::visitIteratorEnd(LIterato
     OutOfLineCode* ool = oolCallVM(CloseIteratorFromIonInfo, lir, ArgList(obj), StoreNothing());
 
     LoadNativeIterator(masm, obj, temp1, ool->entry());
 
     // Clear active bit.
     masm.and32(Imm32(~JSITER_ACTIVE), Address(temp1, offsetof(NativeIterator, flags)));
 
     // Reset property cursor.
-    masm.loadPtr(Address(temp1, offsetof(NativeIterator, props_array)), temp2);
-    masm.storePtr(temp2, Address(temp1, offsetof(NativeIterator, props_cursor)));
+    Address propCursor(temp1, offsetof(NativeIterator, props_cursor));
+    masm.computeEffectiveAddress(Address(temp1, sizeof(NativeIterator)), temp2);
+    masm.storePtr(temp2, propCursor);
 
     // Unlink from the iterator list.
     const Register next = temp2;
     const Register prev = temp3;
     masm.loadPtr(Address(temp1, NativeIterator::offsetOfNext()), next);
     masm.loadPtr(Address(temp1, NativeIterator::offsetOfPrev()), prev);
     masm.storePtr(prev, Address(next, NativeIterator::offsetOfPrev()));
     masm.storePtr(next, Address(prev, NativeIterator::offsetOfNext()));
--- a/js/src/vm/Iteration.cpp
+++ b/js/src/vm/Iteration.cpp
@@ -560,30 +560,46 @@ NewPropertyIteratorObject(JSContext* cx)
 
     MOZ_ASSERT(res->numFixedSlots() == JSObject::ITER_CLASS_NFIXED_SLOTS);
     return res;
 }
 
 NativeIterator*
 NativeIterator::allocateIterator(JSContext* cx, uint32_t numGuards, uint32_t plength)
 {
-    JS_STATIC_ASSERT(sizeof(ReceiverGuard) == 2 * sizeof(void*));
+    static_assert(sizeof(ReceiverGuard) == 2 * sizeof(GCPtrFlatString),
+                  "NativeIterators are allocated in space for 1) themselves, "
+                  "2) the properties a NativeIterator iterates (as "
+                  "GCPtrFlatStrings), and 3) |numGuards| ReceiverGuard "
+                  "objects; the additional-length calculation below assumes "
+                  "this size-relationship when determining the extra space to "
+                  "allocate");
+    static_assert(alignof(ReceiverGuard) == alignof(GCPtrFlatString),
+                  "the end of all properties must be exactly aligned adequate "
+                  "to begin storing ReceiverGuards, else the tacked-on memory "
+                  "below will be inadequate to store all properties/guards");
 
     size_t extraLength = plength + numGuards * 2;
-    NativeIterator* ni = cx->zone()->pod_malloc_with_extra<NativeIterator, void*>(extraLength);
+    NativeIterator* ni =
+        cx->zone()->pod_malloc_with_extra<NativeIterator, GCPtrFlatString>(extraLength);
     if (!ni) {
         ReportOutOfMemory(cx);
         return nullptr;
     }
 
-    void** extra = reinterpret_cast<void**>(ni + 1);
+    // Zero out the NativeIterator first.
     PodZero(ni);
+
+    // Zero out the remaining space for GCPtrFlatStrings for properties and
+    // ReceiverGuards for guards.
+    GCPtrFlatString* extra = ni->begin();
     PodZero(extra, extraLength);
-    ni->props_array = ni->props_cursor = reinterpret_cast<GCPtrFlatString*>(extra);
-    ni->props_end = ni->props_array + plength;
+
+    ni->props_cursor = extra;
+    ni->props_end = extra + plength;
     return ni;
 }
 
 NativeIterator*
 NativeIterator::allocateSentinel(JSContext* maybecx)
 {
     NativeIterator* ni = js_pod_malloc<NativeIterator>();
     if (!ni) {
@@ -616,21 +632,22 @@ NativeIterator::initProperties(JSContext
 {
     // The obj parameter is just so that we can ensure that this object will get
     // traced if we GC.
     MOZ_ASSERT(this == obj->getNativeIterator());
 
     size_t plength = props.length();
     MOZ_ASSERT(plength == size_t(end() - begin()));
 
+    GCPtrFlatString* propNames = begin();
     for (size_t i = 0; i < plength; i++) {
         JSFlatString* str = IdToString(cx, props[i]);
         if (!str)
             return false;
-        props_array[i].init(str);
+        propNames[i].init(str);
     }
 
     return true;
 }
 
 static inline void
 RegisterEnumerator(JSContext* cx, NativeIterator* ni)
 {
@@ -1131,17 +1148,17 @@ js::CloseIterator(JSObject* obj)
 
         MOZ_ASSERT(ni->flags & JSITER_ACTIVE);
         ni->flags &= ~JSITER_ACTIVE;
 
         /*
          * Reset the enumerator; it may still be in the cached iterators
          * for this thread, and can be reused.
          */
-        ni->props_cursor = ni->props_array;
+        ni->props_cursor = ni->begin();
     }
 }
 
 bool
 js::IteratorCloseForException(JSContext* cx, HandleObject obj)
 {
     MOZ_ASSERT(cx->isExceptionPending());
 
--- a/js/src/vm/Iteration.h
+++ b/js/src/vm/Iteration.h
@@ -28,35 +28,52 @@
 namespace js {
 
 class PropertyIteratorObject;
 
 struct NativeIterator
 {
     GCPtrObject obj;    // Object being iterated.
     JSObject* iterObj_; // Internal iterator object.
-    GCPtrFlatString* props_array;
     GCPtrFlatString* props_cursor;
     GCPtrFlatString* props_end;
     HeapReceiverGuard* guard_array;
     uint32_t guard_length;
     uint32_t guard_key;
     uint32_t flags;
 
   private:
     /* While in compartment->enumerators, these form a doubly linked list. */
     NativeIterator* next_;
     NativeIterator* prev_;
 
+    // No further fields appear after here *in NativeIterator*, but this class
+    // is always allocated with space tacked on immediately after |this| (see
+    // below) to store 1) a dynamically known number of iterated property names
+    // and 2) a dynamically known number of HeapReceiverGuards.  The limit of
+    // all such property names, and the start of HeapReceiverGuards, is
+    // |props_end|.  The next property name to iterate is |props_cursor| and
+    // equals |props_end| when this iterator is exhausted but not ready yet for
+    // possible reuse.
+
   public:
-    inline GCPtrFlatString* begin() const {
-        return props_array;
+    GCPtrFlatString* begin() const {
+        static_assert(alignof(NativeIterator) >= alignof(GCPtrFlatString),
+                      "GCPtrFlatStrings for properties must be able to appear "
+                      "directly after NativeIterator, with no padding space "
+                      "required for correct alignment");
+
+        // Note that JIT code inlines this computation to reset |props_cursor|
+        // when an iterator ends: see |CodeGenerator::visitIteratorEnd|.
+        const NativeIterator* immediatelyAfter = this + 1;
+        auto* afterNonConst = const_cast<NativeIterator*>(immediatelyAfter);
+        return reinterpret_cast<GCPtrFlatString*>(afterNonConst);
     }
 
-    inline GCPtrFlatString* end() const {
+    GCPtrFlatString* end() const {
         return props_end;
     }
 
     size_t numKeys() const {
         return end() - begin();
     }
 
     JSObject* iterObj() const {