Bug 1462261 - Abandon the idea of HashTableEntry being "POD", make its constructor explicitly initialize HashTableEntry::keyHash, give it a destructor that destroys the stored T if the entry is live, and call both constructor and destructor in the necessary places. r=jandem
authorJeff Walden <jwalden@mit.edu>
Wed, 16 May 2018 14:18:11 -0700
changeset 418911 353d2ee29591
parent 418910 233871244d80
child 418912 7a053ded7f80
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
bugs1462261
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 1462261 - Abandon the idea of HashTableEntry being "POD", make its constructor explicitly initialize HashTableEntry::keyHash, give it a destructor that destroys the stored T if the entry is live, and call both constructor and destructor in the necessary places. r=jandem
js/public/HashTable.h
--- a/js/public/HashTable.h
+++ b/js/public/HashTable.h
@@ -767,19 +767,16 @@ class HashMapEntry
     HashMapEntry(const HashMapEntry&) = delete;
     void operator=(const HashMapEntry&) = delete;
 };
 
 } // namespace js
 
 namespace mozilla {
 
-template <typename T>
-struct IsPod<js::detail::HashTableEntry<T> > : IsPod<T> {};
-
 template <typename K, typename V>
 struct IsPod<js::HashMapEntry<K, V> >
   : IntegralConstant<bool, IsPod<K>::value && IsPod<V>::value>
 {};
 
 } // namespace mozilla
 
 namespace js {
@@ -790,55 +787,56 @@ template <class T, class HashPolicy, cla
 class HashTable;
 
 template <typename T>
 class HashTableEntry
 {
   private:
     using NonConstT = typename mozilla::RemoveConst<T>::Type;
 
-    HashNumber keyHash;
+    static const HashNumber sFreeKey = 0;
+    static const HashNumber sRemovedKey = 1;
+    static const HashNumber sCollisionBit = 1;
+
+    HashNumber keyHash = sFreeKey;
     alignas(NonConstT) unsigned char valueData_[sizeof(NonConstT)];
 
   private:
     template <class, class, class> friend class HashTable;
 
     // Some versions of GCC treat it as a -Wstrict-aliasing violation (ergo a
     // -Werror compile error) to reinterpret_cast<> |valueData_| to |T*|, even
     // through |void*|.  Placing the latter cast in these separate functions
     // breaks the chain such that affected GCC versions no longer warn/error.
     void* rawValuePtr() { return valueData_; }
 
-    static const HashNumber sFreeKey = 0;
-    static const HashNumber sRemovedKey = 1;
-    static const HashNumber sCollisionBit = 1;
-
     static bool isLiveHash(HashNumber hash)
     {
         return hash > sRemovedKey;
     }
 
     HashTableEntry(const HashTableEntry&) = delete;
     void operator=(const HashTableEntry&) = delete;
-    ~HashTableEntry() = delete;
 
     NonConstT* valuePtr() { return reinterpret_cast<NonConstT*>(rawValuePtr()); }
 
     void destroyStoredT() {
         NonConstT* ptr = valuePtr();
         ptr->~T();
         MOZ_MAKE_MEM_UNDEFINED(ptr, sizeof(*ptr));
     }
 
   public:
-    // NB: HashTableEntry is treated as a POD: no constructor or destructor calls.
+    HashTableEntry() = default;
 
-    void destroyIfLive() {
+    ~HashTableEntry() {
         if (isLive())
             destroyStoredT();
+
+        MOZ_MAKE_MEM_UNDEFINED(this, sizeof(*this));
     }
 
     void destroy() {
         MOZ_ASSERT(isLive());
         destroyStoredT();
     }
 
     void swap(HashTableEntry* other) {
@@ -1291,36 +1289,41 @@ class HashTable : private AllocPolicy
         return keyHash & ~sCollisionBit;
     }
 
     enum FailureBehavior { DontReportFailure = false, ReportFailure = true };
 
     static Entry* createTable(AllocPolicy& alloc, uint32_t capacity,
                               FailureBehavior reportFailure = ReportFailure)
     {
-        static_assert(sFreeKey == 0,
-                      "newly-calloc'd tables have to be considered empty");
-        if (reportFailure)
-            return alloc.template pod_calloc<Entry>(capacity);
-
-        return alloc.template maybe_pod_calloc<Entry>(capacity);
+        Entry* table = reportFailure
+                       ? alloc.template pod_malloc<Entry>(capacity)
+                       : alloc.template maybe_pod_malloc<Entry>(capacity);
+        if (table) {
+            for (uint32_t i = 0; i < capacity; i++)
+                new (&table[i]) Entry();
+        }
+        return table;
     }
 
     static Entry* maybeCreateTable(AllocPolicy& alloc, uint32_t capacity)
     {
-        static_assert(sFreeKey == 0,
-                      "newly-calloc'd tables have to be considered empty");
-        return alloc.template maybe_pod_calloc<Entry>(capacity);
+        Entry* table = alloc.template maybe_pod_malloc<Entry>(capacity);
+        if (table) {
+            for (uint32_t i = 0; i < capacity; i++)
+                new (&table[i]) Entry();
+        }
+        return table;
     }
 
     static void destroyTable(AllocPolicy& alloc, Entry* oldTable, uint32_t capacity)
     {
         Entry* end = oldTable + capacity;
         for (Entry* e = oldTable; e < end; ++e)
-            e->destroyIfLive();
+            e->~Entry();
         alloc.free_(oldTable);
     }
 
   public:
     explicit HashTable(AllocPolicy ap)
       : AllocPolicy(ap)
       , gen(0)
       , hashShift(sHashBits)
@@ -1573,18 +1576,19 @@ class HashTable : private AllocPolicy
 
         // Copy only live entries, leaving removed ones behind.
         Entry* end = oldTable + oldCap;
         for (Entry* src = oldTable; src < end; ++src) {
             if (src->isLive()) {
                 HashNumber hn = src->getKeyHash();
                 findFreeEntry(hn).setLive(
                     hn, mozilla::Move(const_cast<typename Entry::NonConstT&>(src->get())));
-                src->destroy();
             }
+
+            src->~Entry();
         }
 
         // All entries have been destroyed, no need to destroyTable.
         this->free_(oldTable);
         return Rehashed;
     }
 
     bool shouldCompressTable()