Revert LRU cache a node's event nodes path order and re-use

After extensive implementation and performance analysis, we have assessed that implementing a cache for EventPath node path calculation is not an improvement on the existing code. Although it will improve speed for desktop users, it showed significant regressions for mobile users. The additional caching using hash maps also worsens the memory performance. See full study at [1]. [1] https://docs.google.com/document/d/1vDMQlT_Z73DLWaDsvao-HxPAnbH4Vynzpzxr1_IaVGs/edit?usp=sharing&resourcekey=0-DAkF61syXCMR8M62VesdJA Reverted patches: LRU Cache a node's event nodes path order to re-use https://chromium-review.googlesource.com/c/chromium/src/+/3877370 Avoid unneeded use of Member<> on the stack https://chromium-review.googlesource.com/c/chromium/src/+/3935185 Add Finch experiment config https://chromium-review.googlesource.com/c/chromium/src/+/3933509 Change EventPath caching of size 1 to not use hash sets https://chromium-review.googlesource.com/c/chromium/src/+/4126821 Change-Id: Iefd5a1f132accd909231dd9e3e3a0ffdccc7504b Bug: 329788, 1359407, 1394555 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189367 Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org> Reviewed-by: Mason Freed <masonf@chromium.org> Commit-Queue: Di Zhang <dizhangg@chromium.org> Cr-Commit-Position: refs/heads/main@{#1096983}
chromium · Jan 25, 2023 · fd74689 · fd74689
1 parent bfaec82
commit fd74689
Show file tree

Hide file tree

Showing 9 changed files with 0 additions and 233 deletions.
diff --git a/...oid_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java b/...oid_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java
@@ -315,8 +315,6 @@ private ProductionSupportedFlagList() {}
                     "Switches skia to use DMSAA instead of MSAA for tile raster"),
             Flag.baseFeature(
                     CcFeatures.AVOID_RASTER_DURING_ELASTIC_OVERSCROLL, "No effect on webview"),
-            Flag.baseFeature(BlinkFeatures.DOCUMENT_EVENT_NODE_PATH_CACHING,
-                    "Enables a performance optimization that caches event paths."),
             Flag.baseFeature(BlinkFeatures.WEB_RTC_METRONOME,
                     "Inject a metronome into webrtc to allow task coalescing, "
                             + " including synchronized decoding."),

diff --git a/testing/variations/fieldtrial_testing_config.json b/testing/variations/fieldtrial_testing_config.json
@@ -4421,28 +4421,6 @@
             ]
         }
     ],
-    "DocumentEventNodePathCaching": [
-        {
-            "platforms": [
-                "android",
-                "android_webview",
-                "chromeos",
-                "chromeos_lacros",
-                "ios",
-                "linux",
-                "mac",
-                "windows"
-            ],
-            "experiments": [
-                {
-                    "name": "Enabled",
-                    "enable_features": [
-                        "DocumentEventNodePathCaching"
-                    ]
-                }
-            ]
-        }
-    ],
     "DownloadLater": [
         {
             "platforms": [

diff --git a/third_party/blink/common/features.cc b/third_party/blink/common/features.cc
@@ -1562,10 +1562,6 @@ BASE_FEATURE(kThreadedBodyLoader,
              "ThreadedBodyLoader",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
-BASE_FEATURE(kDocumentEventNodePathCaching,
-             "DocumentEventNodePathCaching",
-             base::FEATURE_DISABLED_BY_DEFAULT);
-
 BASE_FEATURE(kNewBaseUrlInheritanceBehavior,
              "NewBaseUrlInheritanceBehavior",
              base::FEATURE_DISABLED_BY_DEFAULT);
@@ -1584,9 +1580,6 @@ bool IsNewBaseUrlInheritanceBehaviorEnabled() {
              switches::kDisableNewBaseUrlInheritanceBehavior);
 }
 
-const base::FeatureParam<int> kDocumentMaxEventNodePathCachedEntries{
-    &kDocumentEventNodePathCaching, "max-cache-entries", 10};
-
 BASE_FEATURE(
     kPostMessageFirstPartyToThirdPartyDifferentBucketSameOriginBlocked,
     "PostMessageFirstPartyToThirdPartyDifferentBucketSameOriginBlocked",

diff --git a/third_party/blink/perf_tests/events/EventsDispatchingDifferentEventsOnSameNode.html b/third_party/blink/perf_tests/events/EventsDispatchingDifferentEventsOnSameNode.html
diff --git a/third_party/blink/public/common/features.h b/third_party/blink/public/common/features.h
@@ -831,14 +831,6 @@ BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kFastPathPaintPropertyUpdates);
 // If enabled, reads and decodes navigation body data off the main thread.
 BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kThreadedBodyLoader);
 
-// If enabled, will cache for each node's EventPath::NodePath in document.
-BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kDocumentEventNodePathCaching);
-
-// Parameter for tuning max entries allowed in EventNodePathCache, which will be
-// used to do LRU eviction in document.
-BLINK_COMMON_EXPORT extern const base::FeatureParam<int>
-    kDocumentMaxEventNodePathCachedEntries;
-
 // Whether first-party to third-party different-bucket same-origin post messages
 // are blocked.
 BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(

diff --git a/third_party/blink/renderer/core/dom/document.cc b/third_party/blink/renderer/core/dom/document.cc
@@ -8560,10 +8560,6 @@ void Document::Trace(Visitor* visitor) const {
   visitor->Trace(anchor_element_interaction_tracker_);
   visitor->Trace(focused_element_change_observers_);
   visitor->Trace(pending_link_header_preloads_);
-  visitor->Trace(event_node_path_cache_);
-  visitor->Trace(event_node_path_cache_key_list_);
-  visitor->Trace(latest_cached_event_node_);
-  visitor->Trace(latest_cached_event_node_path_);
   Supplementable<Document>::Trace(visitor);
   TreeScope::Trace(visitor);
   ContainerNode::Trace(visitor);
@@ -8941,76 +8937,6 @@ Document::PendingJavascriptUrl::PendingJavascriptUrl(
 
 Document::PendingJavascriptUrl::~PendingJavascriptUrl() = default;
 
-static wtf_size_t MaxEventNodePathCachedEntriesValue() {
-  // The cache stores N entries/nodes that are receiving events simultaneously
-  // in a document. The size of this cache will be O(kN) where k is the average
-  // tree depth of the stored nodes.
-  static const wtf_size_t kMaxEventNodePathCachedEntriesValue =
-      features::kDocumentMaxEventNodePathCachedEntries.Get();
-  return kMaxEventNodePathCachedEntriesValue;
-}
-
-static bool EventNodePathCachingEnabled() {
-  // Cache the feature value since checking for each event path regresses
-  // performance.
-  static const bool kEnabled =
-      base::FeatureList::IsEnabled(features::kDocumentEventNodePathCaching);
-  return kEnabled;
-}
-
-const EventPath::NodePath& Document::GetOrCalculateEventNodePath(Node& node) {
-  DCHECK(EventNodePathCachingEnabled());
-  wtf_size_t max_entries = MaxEventNodePathCachedEntriesValue();
-  if (max_entries == 1) {
-    if (event_node_path_dom_tree_version_ == dom_tree_version_ &&
-        latest_cached_event_node_ == &node) {
-      return *latest_cached_event_node_path_;
-    } else {
-      EventPath::NodePath node_path = EventPath::CalculateNodePath(node);
-      event_node_path_dom_tree_version_ = dom_tree_version_;
-      latest_cached_event_node_ = &node;
-      latest_cached_event_node_path_ =
-          MakeGarbageCollected<EventPath::NodePath>(std::move(node_path));
-      return *latest_cached_event_node_path_;
-    }
-  }
-
-  if (event_node_path_dom_tree_version_ != dom_tree_version_) {
-    if (!event_node_path_cache_.empty()) {
-      event_node_path_cache_.clear();
-      event_node_path_cache_key_list_.clear();
-    } else {
-      DCHECK(event_node_path_cache_key_list_.empty());
-    }
-    event_node_path_dom_tree_version_ = dom_tree_version_;
-  }
-
-  EventPath::NodePath* event_node_path = nullptr;
-  {
-    // Keep `result` within own scope to avoid dangling references into cache,
-    // because it might get modified during pruning.
-    auto result = event_node_path_cache_.insert(&node, nullptr);
-    if (result.is_new_entry) {
-      EventPath::NodePath node_path = EventPath::CalculateNodePath(node);
-      result.stored_value->value =
-          MakeGarbageCollected<EventPath::NodePath>(std::move(node_path));
-    }
-    event_node_path = result.stored_value->value;
-  }
-  event_node_path_cache_key_list_.PrependOrMoveToFirst(&node);
-
-  // Prune oldest cached node if size is bigger than max.
-  if (event_node_path_cache_key_list_.size() > max_entries) {
-    DCHECK_EQ(event_node_path_cache_key_list_.size(), max_entries + 1);
-    event_node_path_cache_.erase(event_node_path_cache_key_list_.back());
-    event_node_path_cache_key_list_.pop_back();
-  }
-
-  DCHECK_EQ(event_node_path_cache_.size(),
-            event_node_path_cache_key_list_.size());
-  return *event_node_path;
-}
-
 void Document::ResetAgent(Agent& agent) {
   agent_ = agent;
 }

diff --git a/third_party/blink/renderer/core/dom/document.h b/third_party/blink/renderer/core/dom/document.h
@@ -1473,8 +1473,6 @@ class CORE_EXPORT Document : public ContainerNode,
   // https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-embed-element
   void DelayLoadEventUntilLayoutTreeUpdate();
 
-  const EventPath::NodePath& GetOrCalculateEventNodePath(Node& node);
-
   const DocumentTiming& GetTiming() const { return document_timing_; }
 
   bool ShouldMarkFontPerformance() const {
@@ -2474,20 +2472,6 @@ class CORE_EXPORT Document : public ContainerNode,
   // successful and not successful) by the page.
   std::unique_ptr<FontMatchingMetrics> font_matching_metrics_;
 
-  // For a given node, cache the vector of nodes that defines its EventPath so
-  // all events dispatched on this node won't get recalculated. This cache uses
-  // a LRU strategy and gets cleared when the DOM tree version changes.
-  uint64_t event_node_path_dom_tree_version_;
-  using EventNodePathCache =
-      HeapHashMap<Member<Node>, Member<EventPath::NodePath>>;
-  using EventNodePathCacheKeyList = HeapLinkedHashSet<Member<Node>>;
-  EventNodePathCache event_node_path_cache_;
-  EventNodePathCacheKeyList event_node_path_cache_key_list_;
-  // If we only want to cache one event path, we can avoid using the heap hash
-  // map and hash set.
-  Member<Node> latest_cached_event_node_;
-  Member<EventPath::NodePath> latest_cached_event_node_path_;
-
 #if DCHECK_IS_ON()
   unsigned slot_assignment_recalc_forbidden_recursion_depth_ = 0;
 #endif

diff --git a/third_party/blink/renderer/core/dom/events/event_path.cc b/third_party/blink/renderer/core/dom/events/event_path.cc
@@ -26,7 +26,6 @@
 
 #include "third_party/blink/renderer/core/dom/events/event_path.h"
 
-#include "third_party/blink/public/common/features.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/events/window_event_context.h"
 #include "third_party/blink/renderer/core/dom/shadow_root.h"
@@ -85,74 +84,10 @@ void EventPath::Initialize() {
   CalculateTreeOrderAndSetNearestAncestorClosedTree();
 }
 
-EventPath::NodePath EventPath::CalculateNodePath(Node& node) {
-  // Given a node, find all the nodes the event path might traverse.
-  NodePath node_path;
-  Node* current = &node;
-
-  node_path.push_back(current);
-  while (current) {
-    if (current->IsChildOfShadowHost() && !current->IsPseudoElement()) {
-      if (HTMLSlotElement* slot = current->AssignedSlot()) {
-        current = slot;
-        node_path.push_back(current);
-        continue;
-      }
-    }
-    if (auto* shadow_root = DynamicTo<ShadowRoot>(current)) {
-      current = current->OwnerShadowHost();
-      node_path.push_back(current);
-    } else {
-      current = current->parentNode();
-      if (current)
-        node_path.push_back(current);
-    }
-  }
-  return node_path;
-}
-
-static bool EventNodePathCachingEnabled() {
-  // Cache the feature value since checking for each event path regresses
-  // performance.
-  static const bool kEnabled =
-      base::FeatureList::IsEnabled(features::kDocumentEventNodePathCaching);
-  return kEnabled;
-}
-
 void EventPath::CalculatePath() {
   DCHECK(node_);
   DCHECK(node_event_contexts_.empty());
 
-  if (EventNodePathCachingEnabled())
-    CalculatePathCachingEnabled();
-  else
-    CalculatePathCachingDisabled();
-}
-
-void EventPath::CalculatePathCachingEnabled() {
-  // Find the cached CalculateNodePath result
-  const NodePath& node_path =
-      node_->GetDocument().GetOrCalculateEventNodePath(*node_);
-  // We need to do the KeepEventInNode and ShouldStopAtShadowRoot checks
-  // outside of CalculateNodePath as they depend on the dispatched event.
-  for (Node* node_in_path : node_path) {
-    DCHECK(node_in_path);
-    node_event_contexts_.push_back(NodeEventContext(
-        *node_in_path, EventTargetRespectingTargetRules(*node_in_path)));
-    if (!event_)
-      continue;
-    if (node_in_path->KeepEventInNode(*event_))
-      break;
-    if (auto* shadow_root = DynamicTo<ShadowRoot>(node_in_path)) {
-      if (ShouldStopAtShadowRoot(*event_, *shadow_root, *node_))
-        break;
-    }
-  }
-}
-
-// TODO(crbug.com/329788): This function should be removed once the
-// kDocumentEventNodePathCaching experiment is fully enabled.
-void EventPath::CalculatePathCachingDisabled() {
   // For performance and memory usage reasons we want to store the
   // path using as few bytes as possible and with as few allocations
   // as possible which is why we gather the data on the stack before

diff --git a/third_party/blink/renderer/core/dom/events/event_path.h b/third_party/blink/renderer/core/dom/events/event_path.h
@@ -46,8 +46,6 @@ class WindowEventContext;
 
 class CORE_EXPORT EventPath final : public GarbageCollected<EventPath> {
  public:
-  using NodePath = HeapVector<Member<Node>, 64>;
-
   explicit EventPath(Node&, Event* = nullptr);
   EventPath(const EventPath&) = delete;
   EventPath& operator=(const EventPath&) = delete;
@@ -90,7 +88,6 @@ class CORE_EXPORT EventPath final : public GarbageCollected<EventPath> {
   NodeEventContext& TopNodeEventContext();
 
   static EventTarget& EventTargetRespectingTargetRules(Node&);
-  static NodePath CalculateNodePath(Node&);
 
   void Trace(Visitor*) const;
   void Clear() {
@@ -103,8 +100,6 @@ class CORE_EXPORT EventPath final : public GarbageCollected<EventPath> {
 
   void Initialize();
   void CalculatePath();
-  void CalculatePathCachingEnabled();
-  void CalculatePathCachingDisabled();
   void CalculateAdjustedTargets();
   void CalculateTreeOrderAndSetNearestAncestorClosedTree();