heap: Avoid multiple retracing of weak containers

We found that erasing from the weak container worklist caused timeouts
in some blink_perf benchmarks. The erase operation was meant to avoid
retracing the same backing store multiple times (an iterator to a hash
table would result in at least 2 on-stack references to the backing
store). Instead of erasing, this CL pushes retraced backing stores to
a vector. Backing stores are retraced only if they are not yet in the
vector. Thus iterators should trigger only a single retracing of the
backing store.
The vector is limited in size to avoid making it uncontrollably large.
That means it is possible to retrace the same backing store multiple
times if it was already ejected from the backing store. However this is
very unlikely in practice.

Drive-by: re-enable skipped tests.

Bug: 1142315
Change-Id: I49bd30263db46e66293d046b85699826298aa033
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2516363Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824069}

third_party/blink/renderer/platform/heap/impl/heap.cc

@@ -59,24 +59,6 @@
 
 namespace blink {
 
-void WeakContainersWorklist::Push(const HeapObjectHeader* object) {
-  DCHECK(object);
-  WTF::MutexLocker locker(lock_);
-  objects_.insert(object);
-}
-
-void WeakContainersWorklist::Erase(const HeapObjectHeader* object) {
-  // This method is called only during atomic pause, so lock is not needed.
-  DCHECK(object);
-  objects_.erase(object);
-}
-
-bool WeakContainersWorklist::Contains(const HeapObjectHeader* object) {
-  // This method is called only during atomic pause, so lock is not needed.
-  DCHECK(object);
-  return objects_.find(object) != objects_.end();
-}
-
 HeapAllocHooks::AllocationHook* HeapAllocHooks::allocation_hook_ = nullptr;
 HeapAllocHooks::FreeHook* HeapAllocHooks::free_hook_ = nullptr;
 

third_party/blink/renderer/platform/heap/impl/heap.h

@@ -105,9 +105,17 @@ using NotSafeToConcurrentlyTraceWorklist =
 
 class WeakContainersWorklist {
  public:
-  void Push(const HeapObjectHeader*);
-  void Erase(const HeapObjectHeader*);
-  bool Contains(const HeapObjectHeader*);
+  inline void Push(const HeapObjectHeader* object) {
+    DCHECK(object);
+    WTF::MutexLocker locker(lock_);
+    objects_.insert(object);
+  }
+
+  inline bool Contains(const HeapObjectHeader* object) {
+    // This method is called only during atomic pause, so lock is not needed.
+    DCHECK(object);
+    return objects_.find(object) != objects_.end();
+  }
 
  private:
   WTF::Mutex lock_;

third_party/blink/renderer/platform/heap/impl/marking_visitor.cc

@@ -228,6 +228,24 @@ void MarkingVisitor::TraceMarkedBackingStoreSlow(const void* value) {
       .trace(thread_state->CurrentVisitor(), value);
 }
 
+constexpr size_t MarkingVisitor::RecentlyRetracedWeakContainers::kMaxCacheSize;
+
+bool MarkingVisitor::RecentlyRetracedWeakContainers::Contains(
+    const HeapObjectHeader* header) {
+  return std::find(recently_retraced_cache_.begin(),
+                   recently_retraced_cache_.end(),
+                   header) != recently_retraced_cache_.end();
+}
+
+void MarkingVisitor::RecentlyRetracedWeakContainers::Insert(
+    const HeapObjectHeader* header) {
+  last_used_index_ = (last_used_index_ + 1) % kMaxCacheSize;
+  if (recently_retraced_cache_.size() <= last_used_index_)
+    recently_retraced_cache_.push_back(header);
+  else
+    recently_retraced_cache_[last_used_index_] = header;
+}
+
 MarkingVisitor::MarkingVisitor(ThreadState* state, MarkingMode marking_mode)
     : MarkingVisitorBase(state, marking_mode, WorklistTaskId::MutatorThread) {
   DCHECK(state->InAtomicMarkingPause());
@@ -252,11 +270,11 @@ void MarkingVisitor::ConservativelyMarkAddress(BasePage* page,
     // retraced strongly now. Previously marked/traced weak containers are
     // marked using the |weak_containers_worklist_|. Other marked object can be
     // skipped.
-    if (weak_containers_worklist_->Contains(header)) {
+    if (weak_containers_worklist_->Contains(header) &&
+        !recently_retraced_weak_containers_.Contains(header)) {
       DCHECK(!header->IsInConstruction());
-      // Remove from the set so multiple iterators don't cause multiple
-      // retracings of the backing store.
-      weak_containers_worklist_->Erase(header);
+      // Record the weak container backing store to avoid retracing it again.
+      recently_retraced_weak_containers_.Insert(header);
       marking_worklist_.Push(
           {header->Payload(), GCInfo::From(header->GcInfoIndex()).trace});
     }

third_party/blink/renderer/platform/heap/impl/marking_visitor.h

@@ -174,6 +174,22 @@ class PLATFORM_EXPORT MarkingVisitor : public MarkingVisitorBase {
   static bool MarkValue(void*, BasePage*, ThreadState*);
   static void TraceMarkedBackingStoreSlow(const void*);
 
+  // Weak containers are strongly retraced during conservative stack scanning.
+  // Stack scanning happens once per GC at the start of the atomic pause.
+  // Because the visitor is not retained between GCs, there is no need to clear
+  // the set at the end of GC.
+  class RecentlyRetracedWeakContainers {
+    static constexpr size_t kMaxCacheSize = 8;
+
+   public:
+    bool Contains(const HeapObjectHeader*);
+    void Insert(const HeapObjectHeader*);
+
+   private:
+    std::vector<const HeapObjectHeader*> recently_retraced_cache_;
+    size_t last_used_index_ = -1;
+  } recently_retraced_weak_containers_;
+
   friend class HeapAllocator;
   template <typename T, TracenessMemberConfiguration tracenessConfiguration>
   friend class MemberBase;

tools/perf/expectations.config

@@ -38,11 +38,6 @@ crbug.com/1128019 [ android-nexus-5x ] blink_perf.bindings/worker-structured-clo
 
 # Benchmark: blink_perf.css
 crbug.com/891878 [ android-nexus-5x android-webview ] blink_perf.css/CustomPropertiesVarAlias.html [ Skip ]
-crbug.com/1142315 [ android-pixel-2 android-webview ] blink_perf.css/SelectorCountScaling.html [ Skip ]
-
-# Benchmark: blink_perf.dom
-crbug.com/1142315 [ android-pixel-2 android-webview ] blink_perf.dom/custom-element-default-style.html [ Skip ]
-crbug.com/1142315 [ android-pixel-2 android-webview ] blink_perf.dom/custom-element-default-style-with-shadow.html [ Skip ]
 
 # Benchmark: blink_perf.events
 crbug.com/963945 [ android-nexus-5x android-webview ] blink_perf.events/EventsDispatchingInDeeplyNestedV1ShadowTrees.html [ Skip ]
@@ -56,10 +51,6 @@ crbug.com/832686 [ android-nexus-5 ] blink_perf.layout/subtree-detaching.html [
 crbug.com/910207 [ android-nexus-5x ] blink_perf.layout/subtree-detaching.html [ Skip ]
 crbug.com/966921 [ android ] blink_perf.layout/line-layout-fit-content.html [ Skip ]
 crbug.com/966921 [ android ] blink_perf.layout/line-layout-fit-content-break-word.html [ Skip ]
-crbug.com/1142315 [ android-pixel-2 android-webview ] blink_perf.layout/ruby.html [ Skip ]
-crbug.com/1142315 [ android-pixel-2 android-webview ] blink_perf.layout/chapter-reflow-once.html [ Skip ]
-crbug.com/1142315 [ android-pixel-2 android-webview ] blink_perf.layout/flexbox-row-stretch-height-indefinite.html [ Skip ]
-crbug.com/1142315 [ android-pixel-2 android-webview ] blink_perf.layout/ArabicLineLayout.html [ Skip ]
 
 # Benchmark: blink_perf.owp_storage
 crbug.com/1128019 [ android-pixel-2 ] blink_perf.owp_storage/idb-load-docs.html [ Skip ]
@@ -84,7 +75,6 @@ crbug.com/966913 [ android-nexus-5x android-webview ] blink_perf.parser/query-se
 crbug.com/966613 [ android-pixel-2 ] blink_perf.parser/query-selector-all-class-deep.html [ Skip ]
 crbug.com/966613 [ android-pixel-2 ] blink_perf.parser/query-selector-all-deep.html [ Skip ]
 crbug.com/966613 [ android-pixel-2 ] blink_perf.parser/query-selector-all-id-deep.html [ Skip ]
-crbug.com/1142315 [ android-pixel-2 android-webview ] blink_perf.parser/innerHTML-setter-siblings.html [ Skip ]
 
 # Benchmark: blink_perf.shadow_dom
 crbug.com/702319 [ android-nexus-5x ] blink_perf.shadow_dom/* [ Skip ]