css/resolver: Refactor weaknesss in MatchedPropertiesCache

MatchedPropertiesCache was the single user of custom weakness using
HashTraits. Supporting custom weakness in hash tables is responsible for
a lot of complications in the tracing protocol.

MatchedPropertiesCache was using weakness to hold alive a cached entry as long
as all of its matched properties are alive. Once a single matched property dies
the entry was removed.

This can be modeled using a custom weakness callback that performs the same
logic. Cached entry don't have any outgoing edges except for their cached
matched properties, so ephemeron semantic is not required. Using a custom weak
callback is also faster because the callback is only executed once and not part
of the ephemeron fixed point protocol. The cached entry is unlinked using the weak callback and collected on next GC cylce.

After this conversion, custom weakness can be removed from hash traits.
Weakness should always go through already supported types (e.g.
WeakMember).

Bug: 1019191
Change-Id: I52b7b24dcba8a3aaca896f86a351da665667fcf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1886611
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710693}

third_party/blink/renderer/core/css/resolver/match_result.h

@@ -46,7 +46,7 @@ struct CORE_EXPORT MatchedProperties {
 
   Member<CSSPropertyValueSet> properties;
 
-  struct {
+  struct Data {
     unsigned link_match_type : 2;
     unsigned valid_property_filter : 2;
     // This is approximately equivalent to the 'shadow-including tree order'.
@@ -61,7 +61,8 @@ struct CORE_EXPORT MatchedProperties {
     //
     // https://drafts.csswg.org/css-scoping/#shadow-cascading
     uint16_t tree_order;
-  } types_;
+  };
+  Data types_;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/css/resolver/matched_properties_cache.cc

@@ -39,7 +39,10 @@ namespace blink {
 void CachedMatchedProperties::Set(const ComputedStyle& style,
                                   const ComputedStyle& parent_style,
                                   const MatchedPropertiesVector& properties) {
-  matched_properties.AppendVector(properties);
+  for (const auto& new_matched_properties : properties) {
+    matched_properties.push_back(new_matched_properties.properties);
+    matched_properties_types.push_back(new_matched_properties.types_);
+  }
 
   // Note that we don't cache the original ComputedStyle instance. It may be
   // further modified.  The ComputedStyle in the cache is really just a holder
@@ -68,20 +71,39 @@ const CachedMatchedProperties* MatchedPropertiesCache::Find(
   CachedMatchedProperties* cache_item = it->value.Get();
   if (!cache_item)
     return nullptr;
-
-  wtf_size_t size = properties.size();
-  if (size != cache_item->matched_properties.size())
+  if (*cache_item != properties)
     return nullptr;
   if (cache_item->computed_style->InsideLink() !=
       style_resolver_state.Style()->InsideLink())
     return nullptr;
-  for (wtf_size_t i = 0; i < size; ++i) {
-    if (properties[i] != cache_item->matched_properties[i])
-      return nullptr;
-  }
   return cache_item;
 }
 
+bool CachedMatchedProperties::operator==(
+    const MatchedPropertiesVector& properties) {
+  if (properties.size() != matched_properties.size())
+    return false;
+  for (wtf_size_t i = 0; i < properties.size(); ++i) {
+    if (properties[i].properties != matched_properties[i])
+      return false;
+    if (properties[i].types_.link_match_type !=
+        matched_properties_types[i].link_match_type)
+      return false;
+    if (properties[i].types_.tree_order !=
+        matched_properties_types[i].tree_order)
+      return false;
+    if (properties[i].types_.valid_property_filter !=
+        matched_properties_types[i].valid_property_filter)
+      return false;
+  }
+  return true;
+}
+
+bool CachedMatchedProperties::operator!=(
+    const MatchedPropertiesVector& properties) {
+  return !(*this == properties);
+}
+
 void MatchedPropertiesCache::Add(const ComputedStyle& style,
                                  const ComputedStyle& parent_style,
                                  unsigned hash,
@@ -158,6 +180,28 @@ bool MatchedPropertiesCache::IsCacheable(const StyleResolverState& state) {
 
 void MatchedPropertiesCache::Trace(blink::Visitor* visitor) {
   visitor->Trace(cache_);
+  visitor->RegisterWeakMembers<
+      MatchedPropertiesCache,
+      &MatchedPropertiesCache::RemoveCachedMatchedPropertiesWithDeadEntries>(
+      this);
+}
+
+void MatchedPropertiesCache::RemoveCachedMatchedPropertiesWithDeadEntries(
+    Visitor* visitor) {
+  Vector<unsigned> to_remove;
+  for (const auto& entry_pair : cache_) {
+    for (const auto& matched_properties :
+         entry_pair.value->matched_properties) {
+      if (!ThreadHeap::IsHeapObjectAlive(matched_properties)) {
+        to_remove.push_back(entry_pair.key);
+        break;
+      }
+    }
+  }
+  // Allocation is forbidden during executing weak callbacks, so the data
+  // structure will not be rehashed here. The next insertion/deletion from
+  // regular code will take care of shrinking accordingly.
+  cache_.RemoveAll(to_remove);
 }
 
 }  // namespace blink

third_party/blink/renderer/core/css/resolver/matched_properties_cache.h

@@ -39,7 +39,11 @@ class StyleResolverState;
 class CachedMatchedProperties final
     : public GarbageCollected<CachedMatchedProperties> {
  public:
-  HeapVector<MatchedProperties> matched_properties;
+  // Caches data of MachedProperties. See MatchedPropertiesCache::Cache for
+  // semantics.
+  Vector<UntracedMember<CSSPropertyValueSet>> matched_properties;
+  Vector<MatchedProperties::Data> matched_properties_types;
+
   scoped_refptr<ComputedStyle> computed_style;
   scoped_refptr<ComputedStyle> parent_computed_style;
 
@@ -47,55 +51,11 @@ class CachedMatchedProperties final
            const ComputedStyle& parent_style,
            const MatchedPropertiesVector&);
   void Clear();
-  void Trace(blink::Visitor* visitor) { visitor->Trace(matched_properties); }
-};
 
-// Specialize the HashTraits for CachedMatchedProperties to check for dead
-// entries in the MatchedPropertiesCache.
-struct CachedMatchedPropertiesHashTraits
-    : HashTraits<Member<CachedMatchedProperties>> {
-  static const WTF::WeakHandlingFlag kWeakHandlingFlag = WTF::kWeakHandling;
-
-  static bool IsAlive(Member<CachedMatchedProperties>& cached_properties) {
-    // Semantics see |CachedMatchedPropertiesHashTraits::TraceInCollection|.
-    if (cached_properties) {
-      for (const auto& matched_properties :
-           cached_properties->matched_properties) {
-        if (!ThreadHeap::IsHeapObjectAlive(matched_properties.properties)) {
-          return false;
-        }
-      }
-    }
-    return true;
-  }
-
-  template <typename VisitorDispatcher>
-  static bool TraceInCollection(
-      VisitorDispatcher visitor,
-      Member<CachedMatchedProperties>& cached_properties,
-      WTF::WeakHandlingFlag weakness) {
-    // Only honor the cache's weakness semantics if the collection is traced
-    // with |kWeakPointersActWeak|. Otherwise just trace the cachedProperties
-    // strongly, i.e., call trace on it.
-    if (cached_properties && weakness == WTF::kWeakHandling) {
-      // A given cache entry is only kept alive if none of the MatchedProperties
-      // in the CachedMatchedProperties value contain a dead "properties" field.
-      // If there is a dead field the entire cache entry is removed.
-      for (const auto& matched_properties :
-           cached_properties->matched_properties) {
-        if (!ThreadHeap::IsHeapObjectAlive(matched_properties.properties)) {
-          // For now report the cache entry as dead. This might not
-          // be the final result if in a subsequent call for this entry,
-          // the "properties" field has been marked via another path.
-          return true;
-        }
-      }
-    }
-    // At this point none of the entries in the matchedProperties vector
-    // had a dead "properties" field so trace CachedMatchedProperties strongly.
-    visitor->Trace(cached_properties);
-    return false;
-  }
+  void Trace(blink::Visitor*) {}
+
+  bool operator==(const MatchedPropertiesVector& properties);
+  bool operator!=(const MatchedPropertiesVector& properties);
 };
 
 class CORE_EXPORT MatchedPropertiesCache {
@@ -122,11 +82,17 @@ class CORE_EXPORT MatchedPropertiesCache {
   void Trace(blink::Visitor*);
 
  private:
+  // The cache is mapping a hash to a cached entry where the entry is kept as
+  // long as *all* properties referred to by the entry are alive. This requires
+  // custom weakness which is managed through
+  // |RemoveCachedMatchedPropertiesWithDeadEntries|.
   using Cache = HeapHashMap<unsigned,
                             Member<CachedMatchedProperties>,
                             DefaultHash<unsigned>::Hash,
-                            HashTraits<unsigned>,
-                            CachedMatchedPropertiesHashTraits>;
+                            HashTraits<unsigned>>;
+
+  void RemoveCachedMatchedPropertiesWithDeadEntries(Visitor*);
+
   Cache cache_;
   DISALLOW_COPY_AND_ASSIGN(MatchedPropertiesCache);
 };