Revert "heap: Avoid eager destruction of heap collections allocated o…

…n stack" This reverts commit de61609. Reason for revert: Speculative revert for crashers, see bugs. Original change's description: > heap: Avoid eager destruction of heap collections allocated on stack > > Before this CL the rules for GarbageCollectedFinalized were diverging > from standard C++ std::is_trivially_destructible because heap > collections had a destructor that would be ignored because the classes > were not required to be GarbageCollectedFinalized. > > After the CL, std::is_trivially_destructible can be used as replacement > for GarbageCollectedFinalized. > > Benchmarks (blink_perf.dom, blink_perf.bindings, speedometer) show no > regressions. > > Bug: 990913 > Change-Id: Ie1722b5c7f97bc7197c34c7d26120e7147893fd5 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1742147 > Commit-Queue: Anton Bikineev <bikineev@chromium.org> > Reviewed-by: Michael Lippautz <mlippautz@chromium.org> > Reviewed-by: Kentaro Hara <haraken@chromium.org> > Cr-Commit-Position: refs/heads/master@{#685557} TBR=haraken@chromium.org,mlippautz@chromium.org,bikineev@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 990913,992794,992657 Change-Id: I95e350eb65a053a9846cab5b261194963f69da46 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1746261 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/master@{#685941}
chromium · Aug 12, 2019 · 0166be9 · 0166be9
1 parent 7b47b5b
commit 0166be9
Show file tree

Hide file tree

Showing 6 changed files with 129 additions and 66 deletions.
diff --git a/third_party/blink/renderer/platform/heap/heap_allocator.h b/third_party/blink/renderer/platform/heap/heap_allocator.h
@@ -473,8 +473,6 @@ class HeapHashMap : public HashMap<KeyArg,
   DISALLOW_NEW();
 
   static void CheckType() {
-    static_assert(std::is_trivially_destructible<HeapHashMap>::value,
-                  "HeapHashMap must be trivially destructible.");
     static_assert(
         IsAllowedInContainer<KeyArg>::value,
         "Not allowed to directly nest type. Use Member<> indirection instead.");
@@ -484,7 +482,7 @@ class HeapHashMap : public HashMap<KeyArg,
     static_assert(
         WTF::IsTraceable<KeyArg>::value || WTF::IsTraceable<MappedArg>::value,
         "For hash maps without traceable elements, use HashMap<> "
-        "instead of HeapHashMap<>.");
+        "instead of HeapHashMap<>");
   }
 
  public:
@@ -506,14 +504,12 @@ class HeapHashSet
   DISALLOW_NEW();
 
   static void CheckType() {
-    static_assert(std::is_trivially_destructible<HeapHashSet>::value,
-                  "HeapHashSet must be trivially destructible.");
     static_assert(
         IsAllowedInContainer<ValueArg>::value,
         "Not allowed to directly nest type. Use Member<> indirection instead.");
     static_assert(WTF::IsTraceable<ValueArg>::value,
                   "For hash sets without traceable elements, use HashSet<> "
-                  "instead of HeapHashSet<>.");
+                  "instead of HeapHashSet<>");
   }
 
  public:
@@ -542,7 +538,7 @@ class HeapLinkedHashSet
         "Not allowed to directly nest type. Use Member<> indirection instead.");
     static_assert(WTF::IsTraceable<ValueArg>::value,
                   "For sets without traceable elements, use LinkedHashSet<> "
-                  "instead of HeapLinkedHashSet<>.");
+                  "instead of HeapLinkedHashSet<>");
   }
 
  public:
@@ -568,14 +564,12 @@ class HeapListHashSet
   DISALLOW_NEW();
 
   static void CheckType() {
-    static_assert(std::is_trivially_destructible<HeapListHashSet>::value,
-                  "HeapListHashSet must be trivially destructible.");
     static_assert(
         IsAllowedInContainer<ValueArg>::value,
         "Not allowed to directly nest type. Use Member<> indirection instead.");
     static_assert(WTF::IsTraceable<ValueArg>::value,
                   "For sets without traceable elements, use ListHashSet<> "
-                  "instead of HeapListHashSet<>.");
+                  "instead of HeapListHashSet<>");
   }
 
  public:
@@ -596,14 +590,12 @@ class HeapHashCountedSet
   DISALLOW_NEW();
 
   static void CheckType() {
-    static_assert(std::is_trivially_destructible<HeapHashCountedSet>::value,
-                  "HeapHashCountedSet must be trivially destructible.");
     static_assert(
         IsAllowedInContainer<Value>::value,
         "Not allowed to directly nest type. Use Member<> indirection instead.");
     static_assert(WTF::IsTraceable<Value>::value,
                   "For counted sets without traceable elements, use "
-                  "HashCountedSet<> instead of HeapHashCountedSet<>.");
+                  "HashCountedSet<> instead of HeapHashCountedSet<>");
   }
 
  public:
@@ -621,15 +613,12 @@ class HeapVector : public Vector<T, inlineCapacity, HeapAllocator> {
   DISALLOW_NEW();
 
   static void CheckType() {
-    static_assert(
-        std::is_trivially_destructible<HeapVector>::value || inlineCapacity,
-        "HeapVector must be trivially destructible.");
     static_assert(
         IsAllowedInContainer<T>::value,
         "Not allowed to directly nest type. Use Member<> indirection instead.");
     static_assert(WTF::IsTraceable<T>::value,
                   "For vectors without traceable elements, use Vector<> "
-                  "instead of HeapVector<>.");
+                  "instead of HeapVector<>");
   }
 
  public:
@@ -672,9 +661,6 @@ class HeapDeque : public Deque<T, inlineCapacity, HeapAllocator> {
   DISALLOW_NEW();
 
   static void CheckType() {
-    static_assert(
-        std::is_trivially_destructible<HeapDeque>::value || inlineCapacity,
-        "HeapDeque must be trivially destructible.");
     static_assert(
         IsAllowedInContainer<T>::value,
         "Not allowed to directly nest type. Use Member<> indirection instead.");

diff --git a/third_party/blink/renderer/platform/heap/heap_test.cc b/third_party/blink/renderer/platform/heap/heap_test.cc
@@ -6013,6 +6013,86 @@ TEST_F(HeapTest, HeapHashMapCallsDestructor) {
   EXPECT_TRUE(string.Impl()->HasOneRef());
 }
 
+TEST_F(HeapTest, PromptlyFreeStackAllocatedHeapVector) {
+  NormalPageArena* normal_arena;
+  Address before;
+  {
+    HeapVector<Member<IntWrapper>> vector;
+    vector.push_back(MakeGarbageCollected<IntWrapper>(0));
+    NormalPage* normal_page =
+        static_cast<NormalPage*>(PageFromObject(vector.data()));
+    normal_arena = normal_page->ArenaForNormalPage();
+    CHECK(normal_arena);
+    before = normal_arena->CurrentAllocationPoint();
+  }
+  Address after = normal_arena->CurrentAllocationPoint();
+  // We check the allocation point to see if promptly freed
+  EXPECT_NE(after, before);
+}
+
+TEST_F(HeapTest, PromptlyFreeStackAllocatedHeapDeque) {
+  NormalPageArena* normal_arena;
+  Address before;
+  {
+    HeapDeque<Member<IntWrapper>> deque;
+    deque.push_back(MakeGarbageCollected<IntWrapper>(0));
+    NormalPage* normal_page =
+        static_cast<NormalPage*>(PageFromObject(&deque.front()));
+    normal_arena = normal_page->ArenaForNormalPage();
+    CHECK(normal_arena);
+    before = normal_arena->CurrentAllocationPoint();
+  }
+  Address after = normal_arena->CurrentAllocationPoint();
+  // We check the allocation point to see if promptly freed
+  EXPECT_NE(after, before);
+}
+
+TEST_F(HeapTest, PromptlyFreeStackAllocatedHeapHashSet) {
+  NormalPageArena* normal_arena = static_cast<NormalPageArena*>(
+      ThreadState::Current()->Heap().Arena(BlinkGC::kHashTableArenaIndex));
+  CHECK(normal_arena);
+  Address before;
+  {
+    HeapHashSet<Member<IntWrapper>> hash_set;
+    hash_set.insert(MakeGarbageCollected<IntWrapper>(0));
+    before = normal_arena->CurrentAllocationPoint();
+  }
+  Address after = normal_arena->CurrentAllocationPoint();
+  // We check the allocation point to see if promptly freed
+  EXPECT_NE(after, before);
+}
+
+TEST_F(HeapTest, PromptlyFreeStackAllocatedHeapListHashSet) {
+  ClearOutOldGarbage();
+  NormalPageArena* normal_arena = static_cast<NormalPageArena*>(
+      ThreadState::Current()->Heap().Arena(BlinkGC::kHashTableArenaIndex));
+  CHECK(normal_arena);
+  Address before;
+  {
+    HeapListHashSet<Member<IntWrapper>> list_hash_set;
+    list_hash_set.insert(MakeGarbageCollected<IntWrapper>(0));
+    before = normal_arena->CurrentAllocationPoint();
+  }
+  Address after = normal_arena->CurrentAllocationPoint();
+  // We check the allocation point to see if promptly freed
+  EXPECT_NE(after, before);
+}
+
+TEST_F(HeapTest, PromptlyFreeStackAllocatedHeapLinkedHashSet) {
+  NormalPageArena* normal_arena = static_cast<NormalPageArena*>(
+      ThreadState::Current()->Heap().Arena(BlinkGC::kHashTableArenaIndex));
+  CHECK(normal_arena);
+  Address before;
+  {
+    HeapLinkedHashSet<Member<IntWrapper>> linked_hash_set;
+    linked_hash_set.insert(MakeGarbageCollected<IntWrapper>(0));
+    before = normal_arena->CurrentAllocationPoint();
+  }
+  Address after = normal_arena->CurrentAllocationPoint();
+  // We check the allocation point to see if promptly freed
+  EXPECT_NE(after, before);
+}
+
 TEST_F(HeapTest, ShrinkVector) {
   // Regression test: https://crbug.com/823289
 

diff --git a/third_party/blink/renderer/platform/wtf/deque.h b/third_party/blink/renderer/platform/wtf/deque.h
@@ -52,9 +52,7 @@ class DequeConstIterator;
 template <typename T,
           wtf_size_t inlineCapacity = 0,
           typename Allocator = PartitionAllocator>
-class Deque : public ConditionalDestructor<Deque<T, INLINE_CAPACITY, Allocator>,
-                                           (INLINE_CAPACITY == 0) &&
-                                               Allocator::kIsGarbageCollected> {
+class Deque {
   USE_ALLOCATOR(Deque, Allocator);
 
  public:
@@ -68,8 +66,9 @@ class Deque : public ConditionalDestructor<Deque<T, INLINE_CAPACITY, Allocator>,
   Deque& operator=(const Deque&);
   Deque(Deque&&);
   Deque& operator=(Deque&&);
+  ~Deque();
 
-  void Finalize();
+  void FinalizeGarbageCollectedObject() { NOTREACHED(); }
 
   void Swap(Deque&);
 
@@ -375,18 +374,16 @@ inline void Deque<T, inlineCapacity, Allocator>::DestroyAll() {
 // For design of the destructor, please refer to
 // [here](https://docs.google.com/document/d/1AoGTvb3tNLx2tD1hNqAfLRLmyM59GM0O-7rCHTT_7_U/)
 template <typename T, wtf_size_t inlineCapacity, typename Allocator>
-inline void Deque<T, inlineCapacity, Allocator>::Finalize() {
-  static_assert(!Allocator::kIsGarbageCollected || INLINE_CAPACITY,
-                "GarbageCollected collections without inline capacity cannot "
-                "be finalized.");
+inline Deque<T, inlineCapacity, Allocator>::~Deque() {
   if ((!INLINE_CAPACITY && !buffer_.Buffer()))
     return;
   if (!IsEmpty() &&
       !(Allocator::kIsGarbageCollected && buffer_.HasOutOfLineBuffer()))
     DestroyAll();
 
-  // For garbage collected deque HeapAllocator::BackingFree() will bail out
-  // during sweeping.
+  // If this is called during sweeping, it must not touch the OutOfLineBuffer.
+  if (Allocator::IsSweepForbidden())
+    return;
   buffer_.Destruct();
 }
 

diff --git a/third_party/blink/renderer/platform/wtf/hash_table.h b/third_party/blink/renderer/platform/wtf/hash_table.h
@@ -29,7 +29,6 @@
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/partition_allocator.h"
 #include "third_party/blink/renderer/platform/wtf/assertions.h"
-#include "third_party/blink/renderer/platform/wtf/conditional_destructor.h"
 #include "third_party/blink/renderer/platform/wtf/construct_traits.h"
 #include "third_party/blink/renderer/platform/wtf/hash_traits.h"
 
@@ -695,15 +694,7 @@ template <typename Key,
           typename Traits,
           typename KeyTraits,
           typename Allocator>
-class HashTable final
-    : public ConditionalDestructor<HashTable<Key,
-                                             Value,
-                                             Extractor,
-                                             HashFunctions,
-                                             Traits,
-                                             KeyTraits,
-                                             Allocator>,
-                                   Allocator::kIsGarbageCollected> {
+class HashTable final {
   DISALLOW_NEW();
 
  public:
@@ -735,11 +726,15 @@ class HashTable final
 
   HashTable();
 
-  void Finalize() {
-    static_assert(!Allocator::kIsGarbageCollected,
-                  "GCed collections can't be finalized.");
+  // For design of the destructor, please refer to
+  // [here](https://docs.google.com/document/d/1AoGTvb3tNLx2tD1hNqAfLRLmyM59GM0O-7rCHTT_7_U/)
+  ~HashTable() {
     if (LIKELY(!table_))
       return;
+    // If this is called during sweeping, it must not touch other heap objects
+    // such as the backing.
+    if (Allocator::IsSweepForbidden())
+      return;
     EnterAccessForbiddenScope();
     DeleteAllBucketsAndDeallocate(table_, table_size_);
     LeaveAccessForbiddenScope();

diff --git a/third_party/blink/renderer/platform/wtf/list_hash_set.h b/third_party/blink/renderer/platform/wtf/list_hash_set.h
@@ -25,7 +25,6 @@
 
 #include <memory>
 #include "third_party/blink/renderer/platform/wtf/allocator/partition_allocator.h"
-#include "third_party/blink/renderer/platform/wtf/conditional_destructor.h"
 #include "third_party/blink/renderer/platform/wtf/hash_set.h"
 
 namespace WTF {
@@ -77,10 +76,7 @@ template <typename ValueArg,
           typename HashArg = typename DefaultHash<ValueArg>::Hash,
           typename AllocatorArg =
               ListHashSetAllocator<ValueArg, inlineCapacity>>
-class ListHashSet
-    : public ConditionalDestructor<
-          ListHashSet<ValueArg, inlineCapacity, HashArg, AllocatorArg>,
-          AllocatorArg::kIsGarbageCollected> {
+class ListHashSet {
   typedef AllocatorArg Allocator;
   USE_ALLOCATOR(ListHashSet, Allocator);
 
@@ -152,7 +148,7 @@ class ListHashSet
   ListHashSet(ListHashSet&&);
   ListHashSet& operator=(const ListHashSet&);
   ListHashSet& operator=(ListHashSet&&);
-  void Finalize();
+  ~ListHashSet();
 
   void Swap(ListHashSet&);
 
@@ -808,10 +804,14 @@ inline void ListHashSet<T, inlineCapacity, U, V>::Swap(ListHashSet& other) {
   allocator_provider_.Swap(other.allocator_provider_);
 }
 
+// For design of the destructor, please refer to
+// [here](https://docs.google.com/document/d/1AoGTvb3tNLx2tD1hNqAfLRLmyM59GM0O-7rCHTT_7_U/)
 template <typename T, size_t inlineCapacity, typename U, typename V>
-inline void ListHashSet<T, inlineCapacity, U, V>::Finalize() {
-  static_assert(!Allocator::kIsGarbageCollected,
-                "GCed collections can't be finalized");
+inline ListHashSet<T, inlineCapacity, U, V>::~ListHashSet() {
+  // If this is called during GC sweeping, it must not touch other heap objects
+  // such as the ListHashSetNodes that is touching in DeleteAllNodes().
+  if (Allocator::IsSweepForbidden())
+    return;
   DeleteAllNodes();
   allocator_provider_.ReleaseAllocator();
 }

diff --git a/third_party/blink/renderer/platform/wtf/vector.h b/third_party/blink/renderer/platform/wtf/vector.h
@@ -31,7 +31,6 @@
 #include "base/template_util.h"
 #include "build/build_config.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/partition_allocator.h"
-#include "third_party/blink/renderer/platform/wtf/conditional_destructor.h"
 #include "third_party/blink/renderer/platform/wtf/construct_traits.h"
 #include "third_party/blink/renderer/platform/wtf/container_annotations.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"  // For default Vector template parameters.
@@ -974,11 +973,7 @@ class VectorBuffer : protected VectorBufferBase<T, true, Allocator> {
 // store iterators in another heap object.
 
 template <typename T, wtf_size_t inlineCapacity, typename Allocator>
-class Vector
-    : private VectorBuffer<T, INLINE_CAPACITY, Allocator>,
-      public ConditionalDestructor<Vector<T, INLINE_CAPACITY, Allocator>,
-                                   (INLINE_CAPACITY == 0) &&
-                                       Allocator::kIsGarbageCollected> {
+class Vector : private VectorBuffer<T, INLINE_CAPACITY, Allocator> {
   USE_ALLOCATOR(Vector, Allocator);
   using Base = VectorBuffer<T, INLINE_CAPACITY, Allocator>;
   using TypeOperations = VectorTypeOperations<T, Allocator>;
@@ -1262,12 +1257,12 @@ class Vector
     return Allocator::template MaxElementCountInBackingStore<T>();
   }
 
-  void Finalize() {
-    static_assert(!Allocator::kIsGarbageCollected || INLINE_CAPACITY,
-                  "GarbageCollected collections without inline capacity cannot "
-                  "be finalized.");
-    if (!INLINE_CAPACITY && LIKELY(!Base::Buffer())) {
-      return;
+  // For design of the destructor, please refer to
+  // [here](https://docs.google.com/document/d/1AoGTvb3tNLx2tD1hNqAfLRLmyM59GM0O-7rCHTT_7_U/)
+  ~Vector() {
+    if (!INLINE_CAPACITY) {
+      if (LIKELY(!Base::Buffer()))
+        return;
     }
     ANNOTATE_DELETE_BUFFER(begin(), capacity(), size_);
     if (LIKELY(size_) &&
@@ -1276,11 +1271,21 @@ class Vector
       size_ = 0;  // Partial protection against use-after-free.
     }
 
-    // For garbage collected vector HeapAllocator::BackingFree() will bail out
-    // during sweeping.
+    // If this is called during sweeping, the backing should not be touched.
+    // Other collections have an early return here if IsSweepForbidden(), but
+    // adding that resulted in performance regression for shadow dom benchmarks
+    // (crbug.com/866084) because of the additional access to TLS. The check has
+    // been removed but the same check exists in HeapAllocator::BackingFree() so
+    // things should be fine as long as VectorBase does not touch the backing.
+
     Base::Destruct();
   }
 
+  // This method will be referenced when creating an on-heap HeapVector with
+  // inline capacity and elements requiring destruction. However usage of such a
+  // type is banned with a static assert.
+  void FinalizeGarbageCollectedObject() { NOTREACHED(); }
+
   template <typename VisitorDispatcher, typename A = Allocator>
   std::enable_if_t<A::kIsGarbageCollected> Trace(VisitorDispatcher);