Bug 1367214, part 3 - Avoid hashtable lookups in the undisplayed maps for elements that have never had display:none nor display:contents children. r=dholbert draft
authorJonathan Watt <jwatt@jwatt.org>
Tue, 15 Aug 2017 00:11:22 +0100
changeset 646205 c9c96a19e9bb3af78104513faa2383af600c9465
parent 646187 d3d8de877ce87e75834fe494bfe11312f25a468c
child 726152 406f6f45b05de336a00b343dfe90790543ce17e1
push id74022
push userjwatt@jwatt.org
push dateMon, 14 Aug 2017 23:11:51 +0000
reviewersdholbert
bugs1367214
milestone57.0a1
Bug 1367214, part 3 - Avoid hashtable lookups in the undisplayed maps for elements that have never had display:none nor display:contents children. r=dholbert MozReview-Commit-ID: Lk48ilsw4OA
dom/base/nsINode.h
layout/base/nsFrameManager.cpp
--- a/dom/base/nsINode.h
+++ b/dom/base/nsINode.h
@@ -1581,16 +1581,19 @@ private:
     ElementHasWeirdParserInsertionMode,
     // Parser sets this flag if it has notified about the node.
     ParserHasNotified,
     // Sets if the node is apz aware or we have apz aware listeners.
     MayBeApzAware,
     // Set if the element might have any kind of anonymous content children,
     // which would not be found through the element's children list.
     ElementMayHaveAnonymousChildren,
+    // Set if this node has at some point (and may still have)
+    // display:none or display:contents children.
+    NodeMayHaveChildrenWithLayoutBoxesDisabled,
     // Guard value
     BooleanFlagCount
   };
 
   void SetBoolFlag(BooleanFlag name, bool value) {
     static_assert(BooleanFlagCount <= 8*sizeof(mBoolFlags),
                   "Too many boolean flags");
     mBoolFlags = (mBoolFlags & ~(1 << name)) | (value << name);
@@ -1723,16 +1726,29 @@ public:
   bool NodeMayBeApzAware() const
   {
     return GetBoolFlag(MayBeApzAware);
   }
 
   void SetMayHaveAnonymousChildren() { SetBoolFlag(ElementMayHaveAnonymousChildren); }
   bool MayHaveAnonymousChildren() const { return GetBoolFlag(ElementMayHaveAnonymousChildren); }
 
+  void SetMayHaveChildrenWithLayoutBoxesDisabled()
+  {
+    SetBoolFlag(NodeMayHaveChildrenWithLayoutBoxesDisabled);
+  }
+  void UnsetMayHaveChildrenWithLayoutBoxesDisabled()
+  {
+    ClearBoolFlag(NodeMayHaveChildrenWithLayoutBoxesDisabled);
+  }
+  bool MayHaveChildrenWithLayoutBoxesDisabled() const
+  {
+    return GetBoolFlag(NodeMayHaveChildrenWithLayoutBoxesDisabled);
+  }
+
 protected:
   void SetParentIsContent(bool aValue) { SetBoolFlag(ParentIsContent, aValue); }
   void SetIsInDocument() { SetBoolFlag(IsInDocument); }
   void SetNodeIsContent() { SetBoolFlag(NodeIsContent); }
   void ClearInDocument() { ClearBoolFlag(IsInDocument); }
   void SetIsElement() { SetBoolFlag(NodeIsElement); }
   void SetHasID() { SetBoolFlag(ElementHasID); }
   void ClearHasID() { ClearBoolFlag(ElementHasID); }
--- a/layout/base/nsFrameManager.cpp
+++ b/layout/base/nsFrameManager.cpp
@@ -221,16 +221,26 @@ nsFrameManager::SetStyleContextInMap(Und
 
 #ifdef DEBUG
   nsIPresShell* shell = aStyleContext->PresContext()->PresShell();
   NS_ASSERTION(parent || (shell && shell->GetDocument() &&
                           shell->GetDocument()->GetRootElement() == aContent),
                "undisplayed content must have a parent, unless it's the root "
                "element");
 #endif
+
+  // We set this bit as an optimization so that we can can know when a content
+  // node may have |display:none| or |display:contents| children.  This allows
+  // other parts of the code to avoid checking for such children in
+  // mDisplayNoneMap and mDisplayContentsMap if the bit isn't present on a node
+  // that it's handling.
+  if (parent) {
+    parent->SetMayHaveChildrenWithLayoutBoxesDisabled();
+  }
+
   aMap->AddNodeFor(parent, aContent, aStyleContext);
 }
 
 void
 nsFrameManager::RegisterDisplayNoneStyleFor(nsIContent* aContent,
                                             nsStyleContext* aStyleContext)
 {
   if (!mDisplayNoneMap) {
@@ -280,27 +290,54 @@ nsFrameManager::UnregisterDisplayNoneSty
   if (!mDisplayNoneMap) {
     return;
   }
 
   // This function is an entry point into UndisplayedMap handling code, so we
   // must call GetApplicableParent so the parent we pass around is correct.
   aParentContent = UndisplayedMap::GetApplicableParent(aParentContent);
 
-  for (UndisplayedNode* node = mDisplayNoneMap->GetFirstNode(aParentContent);
-       node; node = node->getNext()) {
+  if (aParentContent &&
+      !aParentContent->MayHaveChildrenWithLayoutBoxesDisabled()) {
+    MOZ_ASSERT(!mDisplayNoneMap->GetFirstNode(aParentContent),
+               "MayHaveChildrenWithLayoutBoxesDisabled bit out of sync - "
+               "may fail to remove node from mDisplayNoneMap");
+    return;
+  }
+
+  UndisplayedNode* node = mDisplayNoneMap->GetFirstNode(aParentContent);
+
+  const bool haveOneDisplayNoneChild = node && !node->getNext();
+
+  for (; node; node = node->getNext()) {
     if (node->mContent == aContent) {
       mDisplayNoneMap->RemoveNodeFor(aParentContent, node);
 
 #ifdef DEBUG_UNDISPLAYED_MAP
       printf( "REMOVED!\n");
 #endif
       // make sure that there are no more entries for the same content
       MOZ_ASSERT(!GetDisplayNoneStyleFor(aContent),
                  "Found more undisplayed content data after removal");
+
+      if (haveOneDisplayNoneChild) {
+        // There are no more children of aParentContent in mDisplayNoneMap.
+        MOZ_ASSERT(!mDisplayNoneMap->GetFirstNode(aParentContent),
+                   "Bad UnsetMayHaveChildrenWithLayoutBoxesDisabled call");
+        // If we also know that none of its children are in mDisplayContentsMap
+        // then we can call UnsetMayHaveChildrenWithLayoutBoxesDisabled.  We
+        // don't want to check mDisplayContentsMap though since that involves a
+        // hash table lookup in relatively hot code.  Still, we know there are
+        // no children in mDisplayContentsMap if the map is empty, so we do
+        // check for that.
+        if (aParentContent && !mDisplayContentsMap) {
+          aParentContent->UnsetMayHaveChildrenWithLayoutBoxesDisabled();
+        }
+      }
+
       return;
     }
   }
 
 #ifdef DEBUG_UNDISPLAYED_MAP
   printf( "not found.\n");
 #endif
 }
@@ -308,29 +345,47 @@ nsFrameManager::UnregisterDisplayNoneSty
 void
 nsFrameManager::ClearAllMapsFor(nsIContent* aParentContent)
 {
 #if defined(DEBUG_UNDISPLAYED_MAP) || defined(DEBUG_DISPLAY_CONTENTS_MAP)
   static int i = 0;
   printf("ClearAllMapsFor(%d): parent=%p \n", i++, aParentContent);
 #endif
 
-  if (mDisplayNoneMap) {
-    mDisplayNoneMap->RemoveNodesFor(aParentContent);
-  }
-  if (mDisplayContentsMap) {
-    nsAutoPtr<LinkedList<UndisplayedNode>> list =
-      mDisplayContentsMap->UnlinkNodesFor(aParentContent);
-    if (list) {
-      while (UndisplayedNode* node = list->popFirst()) {
-        ClearAllMapsFor(node->mContent);
-        delete node;
+  if (!aParentContent ||
+      aParentContent->MayHaveChildrenWithLayoutBoxesDisabled()) {
+    if (mDisplayNoneMap) {
+      mDisplayNoneMap->RemoveNodesFor(aParentContent);
+    }
+    if (mDisplayContentsMap) {
+      nsAutoPtr<LinkedList<UndisplayedNode>> list =
+        mDisplayContentsMap->UnlinkNodesFor(aParentContent);
+      if (list) {
+        while (UndisplayedNode* node = list->popFirst()) {
+          ClearAllMapsFor(node->mContent);
+          delete node;
+        }
       }
     }
+    if (aParentContent) {
+      aParentContent->UnsetMayHaveChildrenWithLayoutBoxesDisabled();
+    }
   }
+#ifdef DEBUG
+  else {
+    if (mDisplayNoneMap) {
+      MOZ_ASSERT(!mDisplayNoneMap->GetFirstNode(aParentContent),
+                 "We failed to remove a node from mDisplayNoneMap");
+    }
+    if (mDisplayContentsMap) {
+      MOZ_ASSERT(!mDisplayContentsMap->GetFirstNode(aParentContent),
+                 "We failed to remove a node from mDisplayContentsMap");
+    }
+  }
+#endif
 
   // Need to look at aParentContent's content list due to XBL insertions.
   // Nodes in aParentContent's content list do not have aParentContent as a
   // parent, but are treated as children of aParentContent. We iterate over
   // the flattened content list and just ignore any nodes we don't care about.
   FlattenedChildIterator iter(aParentContent);
   for (nsIContent* child = iter.GetNextChild(); child; child = iter.GetNextChild()) {
     auto parent = child->GetParent();
@@ -371,28 +426,55 @@ nsFrameManager::UnregisterDisplayContent
   if (!mDisplayContentsMap) {
     return;
   }
 
   // This function is an entry point into UndisplayedMap handling code, so we
   // must call GetApplicableParent so the parent we pass around is correct.
   aParentContent = UndisplayedMap::GetApplicableParent(aParentContent);
 
-  for (UndisplayedNode* node = mDisplayContentsMap->GetFirstNode(aParentContent);
-       node; node = node->getNext()) {
+  if (aParentContent &&
+      !aParentContent->MayHaveChildrenWithLayoutBoxesDisabled()) {
+    MOZ_ASSERT(!mDisplayContentsMap->GetFirstNode(aParentContent),
+               "MayHaveChildrenWithLayoutBoxesDisabled bit out of sync - "
+               "may fail to remove node from mDisplayContentsMap");
+    return;
+  }
+
+  UndisplayedNode* node = mDisplayContentsMap->GetFirstNode(aParentContent);
+
+  const bool haveOneDisplayContentsChild = node && !node->getNext();
+
+  for (; node; node = node->getNext()) {
     if (node->mContent == aContent) {
       mDisplayContentsMap->RemoveNodeFor(aParentContent, node);
 
 #ifdef DEBUG_DISPLAY_CONTENTS_MAP
       printf( "REMOVED!\n");
 #endif
       // make sure that there are no more entries for the same content
       MOZ_ASSERT(!GetDisplayContentsStyleFor(aContent),
                  "Found more entries for aContent after removal");
       ClearAllMapsFor(aContent);
+
+      if (haveOneDisplayContentsChild) {
+        // There are no more children of aParentContent in mDisplayContentsMap.
+        MOZ_ASSERT(!mDisplayContentsMap->GetFirstNode(aParentContent),
+                   "Bad UnsetMayHaveChildrenWithLayoutBoxesDisabled call");
+        // If we also know that none of its children are in mDisplayNoneMap
+        // then we can call UnsetMayHaveChildrenWithLayoutBoxesDisabled.  We
+        // don't want to check mDisplayNoneMap though since that involves a
+        // hash table lookup in relatively hot code.  Still, we know there are
+        // no children in mDisplayNoneMap if the map is empty, so we do
+        // check for that.
+        if (aParentContent && !mDisplayNoneMap) {
+          aParentContent->UnsetMayHaveChildrenWithLayoutBoxesDisabled();
+        }
+      }
+
       return;
     }
   }
 #ifdef DEBUG_DISPLAY_CONTENTS_MAP
   printf( "not found.\n");
 #endif
 }