Bug 1421054 - Remove performance cliff resulting from linear search through clips. r?mstange draft
authorKartikaya Gupta <kgupta@mozilla.com>
Tue, 28 Nov 2017 13:59:46 -0500
changeset 704697 8342bff5eb364097aa9a4c271c60c6839096b3ee
parent 704670 259c23e85d588192ab1246405f3467f4c45a9e8c
child 742125 6a4359582826d98f7802415d103ff52b33ecb9c3
push id91207
push userkgupta@mozilla.com
push dateTue, 28 Nov 2017 20:20:45 +0000
reviewersmstange
bugs1421054
milestone59.0a1
Bug 1421054 - Remove performance cliff resulting from linear search through clips. r?mstange MozReview-Commit-ID: 6XCgDSqzHgQ
gfx/layers/wr/ScrollingLayersHelper.cpp
gfx/layers/wr/ScrollingLayersHelper.h
--- a/gfx/layers/wr/ScrollingLayersHelper.cpp
+++ b/gfx/layers/wr/ScrollingLayersHelper.cpp
@@ -30,25 +30,27 @@ void
 ScrollingLayersHelper::BeginBuild(WebRenderLayerManager* aManager,
                                   wr::DisplayListBuilder& aBuilder)
 {
   MOZ_ASSERT(!mManager);
   mManager = aManager;
   MOZ_ASSERT(!mBuilder);
   mBuilder = &aBuilder;
   MOZ_ASSERT(mCache.empty());
+  MOZ_ASSERT(mScrollParents.empty());
   MOZ_ASSERT(mItemClipStack.empty());
 }
 
 void
 ScrollingLayersHelper::EndBuild()
 {
   mBuilder = nullptr;
   mManager = nullptr;
   mCache.clear();
+  mScrollParents.clear();
   MOZ_ASSERT(mItemClipStack.empty());
 }
 
 void
 ScrollingLayersHelper::BeginList()
 {
   mItemClipStack.emplace_back(nullptr, nullptr);
 }
@@ -347,42 +349,53 @@ ScrollingLayersHelper::RecurseAndDefineA
   FrameMetrics::ViewID scrollId = aAsr->GetViewId();
   if (mBuilder->IsScrollLayerDefined(scrollId)) {
     // If we've already defined this scroll layer before, we can early-exit
     ids.first = Some(scrollId);
     if (aChain) {
       if (mBuilder->HasExtraClip()) {
         ids.second = mBuilder->GetCacheOverride(aChain);
       } else {
-        auto it = mCache.find(aChain);
-        if (it == mCache.end()) {
-          // Degenerate case, where there are two clip chain items that are
-          // fundamentally the same but are different objects and so we can't
-          // find it in the cache via hashing. Linear search for it instead.
-          // XXX This shouldn't happen very often but it might still turn out
-          // to be a performance cliff, so we should figure out a better way to
-          // deal with this.
-          for (it = mCache.begin(); it != mCache.end(); it++) {
-            if (DisplayItemClipChain::Equal(aChain, it->first)) {
-              break;
-            }
+        // Since the scroll layer was already defined, find the clip (if any)
+        // that it was previously defined as a child of. If that clip is
+        // equivalent to |aChain|, then we should use that one in the mCache
+        // lookup as it is more likely to produce a result. This happens because
+        // of how we can have two DisplayItemClipChain items that are ::Equal
+        // but not ==, and mCache only does == checking. In the hunk below,
+        // |canonicalChain| can be thought of as the clip chain instance that is
+        // equivalent to |aChain| but has the best chance of being found in
+        // mCache.
+        const DisplayItemClipChain* canonicalChain = aChain;
+        auto it = mScrollParents.find(scrollId);
+        if (it != mScrollParents.end()) {
+          const DisplayItemClipChain* scrollParent = it->second;
+          if (DisplayItemClipChain::Equal(scrollParent, aChain)) {
+            canonicalChain = scrollParent;
           }
         }
+
+        auto it2 = mCache.find(canonicalChain);
         // If |it == mCache.end()| here then we have run into a case where the
-        // scroll layer was previously defined a specific parent clip, and
+        // scroll layer was previously defined with a specific parent clip, and
         // now here it has a different parent clip. Gecko can create display
         // lists like this because it treats the ASR chain and clipping chain
         // more independently, but we can't yet represent this in WR. This is
         // tracked by bug 1409442. For now we'll just leave ids.second as
         // Nothing() which will effectively ignore the clip |aChain|. Once WR
         // supports multiple ancestors on a scroll layer we can deal with this
         // better. The layout/reftests/text/wordwrap-08.html has a Text display
         // item that exercises this case.
-        if (it != mCache.end()) {
-          ids.second = Some(it->second);
+        if (it2 == mCache.end()) {
+          // leave ids.second as Nothing(). This should only happen if we didn't
+          // pick up a better canonicalChain above, either because it didn't
+          // exist, or because it was not ::Equal to aChain. Therefore
+          // canonicalChain must still be equal to aChain here.
+          MOZ_ASSERT(canonicalChain == aChain);
+        } else {
+          ids.second = Some(it2->second);
         }
       }
     }
     return ids;
   }
 
   // If not, recurse to ensure all the ancestors are defined
   auto ancestorIds = DefineClipChain(
@@ -418,16 +431,21 @@ ScrollingLayersHelper::RecurseAndDefineA
       ancestorIds.second = Nothing();
     }
   }
   // At most one of the ancestor pair should be defined here, and the one that
   // is defined will be the parent clip for the new scrollframe that we're
   // defining.
   MOZ_ASSERT(!(ancestorIds.first && ancestorIds.second));
 
+  if (ancestorIds.second) {
+    MOZ_ASSERT(aChain);
+    mScrollParents[scrollId] = aChain;
+  }
+
   LayoutDeviceRect contentRect =
       metrics.GetExpandedScrollableRect() * metrics.GetDevPixelsPerCSSPixel();
   // TODO: check coordinate systems are sane here
   LayoutDeviceRect clipBounds =
       LayoutDeviceRect::FromUnknownRect(metrics.GetCompositionBounds().ToUnknownRect());
   // The content rect that we hand to PushScrollLayer should be relative to
   // the same origin as the clipBounds that we hand to PushScrollLayer - that
   // is, both of them should be relative to the stacking context `aSc`.
--- a/gfx/layers/wr/ScrollingLayersHelper.h
+++ b/gfx/layers/wr/ScrollingLayersHelper.h
@@ -81,16 +81,19 @@ private:
   // end up creating multiple clips in WR that are effectively identical but
   // have separate clip ids. Hopefully this won't happen very often.
   typedef std::unordered_map<const DisplayItemClipChain*, wr::WrClipId> ClipIdMap;
 
   WebRenderLayerManager* MOZ_NON_OWNING_REF mManager;
   wr::DisplayListBuilder* mBuilder;
   ClipIdMap mCache;
 
+  typedef std::unordered_map<FrameMetrics::ViewID, const DisplayItemClipChain*> ScrollParentMap;
+  ScrollParentMap mScrollParents;
+
   struct ItemClips {
     ItemClips(const ActiveScrolledRoot* aAsr,
               const DisplayItemClipChain* aChain);
 
     const ActiveScrolledRoot* mAsr;
     const DisplayItemClipChain* mChain;
 
     Maybe<FrameMetrics::ViewID> mScrollId;