[mpc] Move kMaxDependencies to StyleResolverState

Instead of collecting all dependencies in a (potentially big) HashMap,
and then rejecting them cache-time, it's better to reject excessive
entries in the dependency map in the first place.

This way declarations like "all:inherit" won't (pointlessly) create a
huge HashMap internally.

Bug: 1057072
Change-Id: Ib88c964cdadad2469b9635d510647be8499c76df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2247765Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779708}

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

@@ -65,7 +65,7 @@ void CachedMatchedProperties::Set(
       RuntimeEnabledFeatures::CSSMatchedPropertiesCacheDependenciesEnabled() ||
       dependencies.IsEmpty());
   if (dependencies.size()) {
-    DCHECK(dependencies.size() <= MatchedPropertiesCache::kMaxDependencies);
+    DCHECK(dependencies.size() <= StyleResolverState::kMaxDependencies);
     // Plus one for g_null_atom.
     this->dependencies =
         std::make_unique<AtomicString[]>(dependencies.size() + 1);
@@ -257,8 +257,7 @@ bool MatchedPropertiesCache::IsCacheable(const StyleResolverState& state) {
     return true;
   }
 
-  return state.Dependencies().size() <= kMaxDependencies &&
-         !state.HasIncomparableDependency();
+  return state.HasValidDependencies() && !state.HasIncomparableDependency();
 }
 
 void MatchedPropertiesCache::Trace(Visitor* visitor) const {

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

@@ -79,13 +79,6 @@ class CORE_EXPORT MatchedPropertiesCache {
   MatchedPropertiesCache();
   ~MatchedPropertiesCache() { DCHECK(cache_.IsEmpty()); }
 
-  // Declarations such as all:inherit would effectively produce dependencies on
-  // all (400-500) properties, which is not practical to store in a cache.
-  // Hence we use a reasonable limit for how many dependencies a given entry
-  // may hold. If the limit is exceeded, the MatchResult/ComputedStyle is not
-  // eligible for entry into the cache.
-  static const size_t kMaxDependencies = 8;
-
   class CORE_EXPORT Key {
     STACK_ALLOCATED();
 

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

@@ -443,7 +443,7 @@ TEST_F(MatchedPropertiesCacheTest, MaxDependencies) {
   StyleResolverState state(GetDocument(), *GetDocument().body(), parent.get(),
                            parent.get());
   state.SetStyle(style);
-  for (size_t i = 0; i < MatchedPropertiesCache::kMaxDependencies; i++) {
+  for (size_t i = 0; i < StyleResolverState::kMaxDependencies; i++) {
     CustomProperty property(AtomicString(String::Format("--x%zu", i)),
                             GetDocument());
     state.MarkDependency(property);

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

@@ -242,6 +242,9 @@ const CSSValue& StyleResolverState::ResolveLightDarkPair(
 void StyleResolverState::MarkDependency(const CSSProperty& property) {
   if (!RuntimeEnabledFeatures::CSSMatchedPropertiesCacheDependenciesEnabled())
     return;
+  if (!HasValidDependencies())
+    return;
+
   has_incomparable_dependency_ |= !property.IsComputedValueComparable();
   dependencies_.insert(property.GetCSSPropertyName());
 }

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

@@ -227,6 +227,12 @@ class CORE_EXPORT StyleResolverState {
   // stored in the MatchedPropertiesCache.
   const CSSValue& ResolveLightDarkPair(const CSSProperty&, const CSSValue&);
 
+  // The dependencies we track here end up in an entry in the
+  // MatchedPropertiesCache. Declarations such as "all:inherit" incurs several
+  // hundred dependencies, which is too big to cache, hence the number of
+  // dependencies we can track is limited.
+  static const size_t kMaxDependencies = 8;
+
   // Mark the ComputedStyle as possibly dependent on the specified property.
   //
   // A "dependency" in this context means that one or more of the computed
@@ -239,6 +245,9 @@ class CORE_EXPORT StyleResolverState {
 
   // Returns the set of all properties seen by MarkDependency.
   //
+  // The caller must check if the dependencies are valid via
+  // HasValidDependencies() before calling this function.
+  //
   // Note that this set might be larger than the actual set of dependencies,
   // as we do some degree of over-marking to keep the implementation simple.
   //
@@ -252,13 +261,20 @@ class CORE_EXPORT StyleResolverState {
   //    margin: var(--x) (--y);
   //  }
   //
-  const HashSet<CSSPropertyName>& Dependencies() const { return dependencies_; }
+  const HashSet<CSSPropertyName>& Dependencies() const {
+    DCHECK(HasValidDependencies());
+    return dependencies_;
+  }
 
   // True if there's a dependency without the kComputedValueComparable flag.
   bool HasIncomparableDependency() const {
     return has_incomparable_dependency_;
   }
 
+  bool HasValidDependencies() const {
+    return dependencies_.size() <= kMaxDependencies;
+  }
+
  private:
   enum class AnimatingElementType { kElement, kPseudoElement };
 

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

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "third_party/blink/renderer/core/css/resolver/style_resolver_state.h"
+#include "third_party/blink/renderer/core/css/properties/longhands/custom_property.h"
 #include "third_party/blink/renderer/core/html/html_element.h"
 #include "third_party/blink/renderer/core/testing/page_test_base.h"
 #include "third_party/blink/renderer/platform/testing/runtime_enabled_features_test_helpers.h"
@@ -46,4 +47,20 @@ TEST_F(StyleResolverStateTest, Dependencies) {
   EXPECT_TRUE(state.HasIncomparableDependency());
 }
 
+TEST_F(StyleResolverStateTest, MaxDependencies) {
+  StyleResolverState state(GetDocument(), *GetDocument().body(), nullptr,
+                           nullptr);
+
+  EXPECT_TRUE(state.HasValidDependencies());
+
+  for (size_t i = 0; i < StyleResolverState::kMaxDependencies; ++i) {
+    auto name = AtomicString(String::Format("--v%zu", i));
+    state.MarkDependency(CustomProperty(name, GetDocument()));
+    EXPECT_TRUE(state.HasValidDependencies());
+  }
+
+  state.MarkDependency(CustomProperty("--exceeds-limit", GetDocument()));
+  EXPECT_FALSE(state.HasValidDependencies());
+}
+
 }  // namespace blink