[M116 merge] Revert "blink: avoid unnecessary processing when class a…

…ttribute changes" This reverts commit c628499. Reason for revert: See bug 1474382 for regression. Original change's description: > blink: avoid unnecessary processing when class attribute changes > > When the class attribute changes Element does the following: > > 1. Updates `class` on the ElementData. > 2. Calls to the StyleEngine. > > If the ElementData is shared, and the class data is already up > to date date, then 1 is unnecessary. > > Additionally, if in the inactive document, then 2 is unnecessary. > 2 is unnecessary in this case because of an early out in the > StyleEngine: > https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:third_party/blink/renderer/core/css/style_engine.cc;l=1155;drc=90b428dcab63b652cc91107b81d2758270e92ac0 > > This takes one of the bits in ElementData to track whether the > class is dirty. As the bitfield is 32 bits, I'm reducing the > number of attributes by half (2^28th to 2^27th). This also ensures > we only create ShareableElementData if the number of attributes > fits in the bitmask. > > Bug: 1449329 > Change-Id: Ia661bff5b1a7399be86c6fe0df1eb14fed63982b > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4559646 > Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> > Commit-Queue: Scott Violet <sky@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1149848} (cherry picked from commit c70e750) Bug: 1449329, 1474382 Change-Id: I88187e5972fb747f041ec253f66250065b4a5426 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4803704 Commit-Queue: Scott Violet <sky@chromium.org> Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1187041} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4807997 Owners-Override: Daniel Yip <danielyip@google.com> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Cr-Commit-Position: refs/branch-heads/5845@{#1609} Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
chromium · Aug 25, 2023 · 7d98a3c · 7d98a3c
1 parent 805d74e
commit 7d98a3c
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 78 deletions.
diff --git a/third_party/blink/renderer/core/dom/element.cc b/third_party/blink/renderer/core/dom/element.cc
@@ -2469,17 +2469,6 @@ static inline ClassStringContent ClassStringHasClassName(
 
 void Element::ClassAttributeChanged(const AtomicString& new_class_string) {
   DCHECK(HasElementData());
-  if (!element_data_->IsUnique()) {
-    ShareableElementData* shareable_element_data =
-        To<ShareableElementData>(element_data_.Get());
-    if (!shareable_element_data->class_is_dirty() && !InActiveDocument()) {
-      // `element_data` is shared, class related state is up to date, and
-      // this is not in the active document. No additional processing is
-      // necessary (because ClassChangedForElement() does nothing if not
-      // in an active document).
-      return;
-    }
-  }
   ClassStringContent class_string_content_type =
       ClassStringHasClassName(new_class_string);
   const bool should_fold_case = GetDocument().InQuirksMode();
@@ -2498,9 +2487,6 @@ void Element::ClassAttributeChanged(const AtomicString& new_class_string) {
       GetElementData()->ClearClass();
     }
   }
-  if (!element_data_->IsUnique()) {
-    To<ShareableElementData>(element_data_.Get())->SetClassIsDirty(false);
-  }
 }
 
 void Element::UpdateClassList(const AtomicString& old_class_string,
@@ -2563,14 +2549,14 @@ void Element::ParserSetAttributes(
   DCHECK(!element_data_);
 
   if (!attribute_vector.empty()) {
-    if (auto* cache = GetDocument().GetElementDataCache()) {
-      element_data_ = cache->ElementDataWithAttributes(attribute_vector);
-    } else if (attribute_vector.size() <=
-               ShareableElementData::kMaxNumberOfAttributes) {
+    if (GetDocument().GetElementDataCache()) {
       element_data_ =
-          ShareableElementData::CreateWithAttributes(attribute_vector);
+          GetDocument()
+              .GetElementDataCache()
+              ->CachedShareableElementDataWithAttributes(attribute_vector);
     } else {
-      element_data_ = MakeGarbageCollected<UniqueElementData>(attribute_vector);
+      element_data_ =
+          ShareableElementData::CreateWithAttributes(attribute_vector);
     }
   }
 

diff --git a/third_party/blink/renderer/core/dom/element_data.cc b/third_party/blink/renderer/core/dom/element_data.cc
@@ -52,31 +52,27 @@ static AdditionalBytes AdditionalBytesForShareableElementDataWithAttributeCount(
 }
 
 ElementData::ElementData()
-    : bit_field_(IsUniqueFlag::encode(true) | ShareableArraySize::encode(0) |
+    : bit_field_(IsUniqueFlag::encode(true) | ArraySize::encode(0) |
                  PresentationAttributeStyleIsDirty::encode(false) |
                  StyleAttributeIsDirty::encode(false) |
-                 SvgAttributesAreDirty::encode(false) |
-                 ClassIsDirty::encode(true)) {}
+                 SvgAttributesAreDirty::encode(false)) {}
 
 ElementData::ElementData(unsigned array_size)
-    : bit_field_(
-          IsUniqueFlag::encode(false) | ShareableArraySize::encode(array_size) |
-          PresentationAttributeStyleIsDirty::encode(false) |
-          StyleAttributeIsDirty::encode(false) |
-          SvgAttributesAreDirty::encode(false) | ClassIsDirty::encode(true)) {}
+    : bit_field_(IsUniqueFlag::encode(false) | ArraySize::encode(array_size) |
+                 PresentationAttributeStyleIsDirty::encode(false) |
+                 StyleAttributeIsDirty::encode(false) |
+                 SvgAttributesAreDirty::encode(false)) {}
 
 ElementData::ElementData(const ElementData& other, bool is_unique)
     : bit_field_(
           IsUniqueFlag::encode(is_unique) |
-          ShareableArraySize::encode(is_unique ? 0
-                                               : other.Attributes().size()) |
+          ArraySize::encode(is_unique ? 0 : other.Attributes().size()) |
           PresentationAttributeStyleIsDirty::encode(
               other.bit_field_.get<PresentationAttributeStyleIsDirty>()) |
           StyleAttributeIsDirty::encode(
               other.bit_field_.get<StyleAttributeIsDirty>()) |
           SvgAttributesAreDirty::encode(
-              other.bit_field_.get<SvgAttributesAreDirty>()) |
-          ClassIsDirty::encode(other.bit_field_.get<ClassIsDirty>())),
+              other.bit_field_.get<SvgAttributesAreDirty>())),
       class_names_(other.class_names_),
       id_for_style_resolution_(other.id_for_style_resolution_) {
   // NOTE: The inline style is copied by the subclass copy constructor since we
@@ -129,15 +125,13 @@ void ElementData::TraceAfterDispatch(blink::Visitor* visitor) const {
 ShareableElementData::ShareableElementData(
     const Vector<Attribute, kAttributePrealloc>& attributes)
     : ElementData(attributes.size()) {
-  for (unsigned i = 0; i < bit_field_.get<ShareableArraySize>(); ++i) {
+  for (unsigned i = 0; i < bit_field_.get<ArraySize>(); ++i)
     new (&attribute_array_[i]) Attribute(attributes[i]);
-  }
 }
 
 ShareableElementData::~ShareableElementData() {
-  for (unsigned i = 0; i < bit_field_.get<ShareableArraySize>(); ++i) {
+  for (unsigned i = 0; i < bit_field_.get<ArraySize>(); ++i)
     attribute_array_[i].~Attribute();
-  }
 }
 
 ShareableElementData::ShareableElementData(const UniqueElementData& other)
@@ -148,9 +142,8 @@ ShareableElementData::ShareableElementData(const UniqueElementData& other)
     inline_style_ = other.inline_style_->ImmutableCopyIfNeeded();
   }
 
-  for (unsigned i = 0; i < bit_field_.get<ShareableArraySize>(); ++i) {
+  for (unsigned i = 0; i < bit_field_.get<ArraySize>(); ++i)
     new (&attribute_array_[i]) Attribute(other.attribute_vector_.at(i));
-  }
 }
 
 ShareableElementData* ShareableElementData::CreateWithAttributes(
@@ -191,10 +184,6 @@ ShareableElementData* UniqueElementData::MakeShareableCopy() const {
       *this);
 }
 
-UniqueElementData::UniqueElementData(
-    const Vector<Attribute, kAttributePrealloc>& attrs)
-    : attribute_vector_(attrs) {}
-
 void UniqueElementData::TraceAfterDispatch(blink::Visitor* visitor) const {
   visitor->Trace(presentation_attribute_style_);
   ElementData::TraceAfterDispatch(visitor);

diff --git a/third_party/blink/renderer/core/dom/element_data.h b/third_party/blink/renderer/core/dom/element_data.h
@@ -89,24 +89,15 @@ class ElementData : public GarbageCollected<ElementData> {
   void Trace(Visitor*) const;
 
  protected:
-  static constexpr size_t kShareableArraySizeBits = 27;
-
   using BitField = WTF::ConcurrentlyReadBitField<uint32_t>;
   using IsUniqueFlag =
       BitField::DefineFirstValue<bool, 1, WTF::BitFieldValueConstness::kConst>;
-  // This is used solely by ShareableElementData. It gives the number of
-  // elements in the array.
-  using ShareableArraySize =
-      IsUniqueFlag::DefineNextValue<uint32_t,
-                                    kShareableArraySizeBits,
-                                    WTF::BitFieldValueConstness::kConst>;
-  using PresentationAttributeStyleIsDirty =
-      ShareableArraySize::DefineNextValue<bool, 1>;
+  using ArraySize = IsUniqueFlag::
+      DefineNextValue<uint32_t, 28, WTF::BitFieldValueConstness::kConst>;
+  using PresentationAttributeStyleIsDirty = ArraySize::DefineNextValue<bool, 1>;
   using StyleAttributeIsDirty =
       PresentationAttributeStyleIsDirty::DefineNextValue<bool, 1>;
   using SvgAttributesAreDirty = StyleAttributeIsDirty::DefineNextValue<bool, 1>;
-  // Used by ShareableElementData. Tracks if the `class` needs to be calculated.
-  using ClassIsDirty = SvgAttributesAreDirty::DefineNextValue<bool, 1>;
 
   ElementData();
   explicit ElementData(unsigned array_size);
@@ -121,7 +112,6 @@ class ElementData : public GarbageCollected<ElementData> {
   bool svg_attributes_are_dirty() const {
     return bit_field_.get<SvgAttributesAreDirty>();
   }
-  bool class_is_dirty() const { return bit_field_.get<ClassIsDirty>(); }
 
   // Following 3 fields are meant to be mutable and can change even when const.
   void SetPresentationAttributeStyleIsDirty(
@@ -138,9 +128,6 @@ class ElementData : public GarbageCollected<ElementData> {
     const_cast<BitField*>(&bit_field_)
         ->set<SvgAttributesAreDirty>(svg_attributes_are_dirty);
   }
-  void SetClassIsDirty(bool value) const {
-    const_cast<BitField*>(&bit_field_)->set<ClassIsDirty>(value);
-  }
 
   BitField bit_field_;
 
@@ -172,9 +159,6 @@ class ElementData : public GarbageCollected<ElementData> {
 // duplicate sets of attributes (ex. the same classes).
 class ShareableElementData final : public ElementData {
  public:
-  // Maximum number of attributes this can hold.
-  static constexpr size_t kMaxNumberOfAttributes = 2 ^ kShareableArraySizeBits;
-
   static ShareableElementData* CreateWithAttributes(
       const Vector<Attribute, kAttributePrealloc>&);
 
@@ -217,8 +201,7 @@ class UniqueElementData final : public ElementData {
 
   UniqueElementData();
   explicit UniqueElementData(const ShareableElementData&);
-  UniqueElementData(const UniqueElementData&);
-  explicit UniqueElementData(const Vector<Attribute, kAttributePrealloc>&);
+  explicit UniqueElementData(const UniqueElementData&);
 
   void TraceAfterDispatch(blink::Visitor*) const;
 
@@ -251,8 +234,7 @@ inline AttributeCollection ElementData::Attributes() const {
 }
 
 inline AttributeCollection ShareableElementData::Attributes() const {
-  return AttributeCollection(attribute_array_,
-                             bit_field_.get<ShareableArraySize>());
+  return AttributeCollection(attribute_array_, bit_field_.get<ArraySize>());
 }
 
 inline AttributeCollection UniqueElementData::Attributes() const {

diff --git a/third_party/blink/renderer/core/dom/element_data_cache.cc b/third_party/blink/renderer/core/dom/element_data_cache.cc
@@ -69,7 +69,8 @@ inline bool HasSameAttributes(
                          attributes.size() * sizeof(Attribute));
 }
 
-ElementData* ElementDataCache::ElementDataWithAttributes(
+ShareableElementData*
+ElementDataCache::CachedShareableElementDataWithAttributes(
     const Vector<Attribute, kAttributePrealloc>& attributes) {
   DCHECK(!attributes.empty());
 
@@ -81,13 +82,8 @@ ElementData* ElementDataCache::ElementDataWithAttributes(
   if (it->value && !HasSameAttributes(attributes, *it->value))
     return ShareableElementData::CreateWithAttributes(attributes);
 
-  if (!it->value) {
-    if (attributes.size() < ShareableElementData::kMaxNumberOfAttributes) {
-      it->value = ShareableElementData::CreateWithAttributes(attributes);
-    } else {
-      return MakeGarbageCollected<UniqueElementData>(attributes);
-    }
-  }
+  if (!it->value)
+    it->value = ShareableElementData::CreateWithAttributes(attributes);
 
   return it->value.Get();
 }

diff --git a/third_party/blink/renderer/core/dom/element_data_cache.h b/third_party/blink/renderer/core/dom/element_data_cache.h
@@ -34,17 +34,13 @@
 
 namespace blink {
 
-class ElementData;
 class ShareableElementData;
 
 class ElementDataCache final : public GarbageCollected<ElementDataCache> {
  public:
   ElementDataCache();
 
-  // Returns an ElementData representing the specified attributes. This prefers
-  // returning a ShareableElementData, but in some cases returns an
-  // ElementData.
-  ElementData* ElementDataWithAttributes(
+  ShareableElementData* CachedShareableElementDataWithAttributes(
       const Vector<Attribute, kAttributePrealloc>&);
 
   void Trace(Visitor*) const;