Closed Bug 1296509 Opened 3 years ago Closed 3 years ago

Make GetFlattenedTreeParent common-case fast

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files, 2 obsolete files)

GetFlattenedTreeParent does a whole bunch of slow, non-inline, and virtual stuff to figure out what the correct parent should be accounting for anonymous subtrees and insertion points. However, most of that work is unnecessary in the common case, and we call it in lots of places.

In particular, I want to call it in stylo during parent node traversal, which needs to be wicked fast. I've got patches to optimize it.
We need to call it on some hot paths in stylo, and this allows us to do
quick inline check before delegating to the slow path.
Attachment #8782735 - Flags: review?(wchen)
We need to call it on some hot paths in stylo, and this allows us to do
quick inline check before delegating to the slow path.
Attachment #8782778 - Flags: review?(wchen)
Attachment #8782735 - Attachment is obsolete: true
Attachment #8782735 - Flags: review?(wchen)
We need to call it on some hot paths in stylo, and this allows us to do
quick inline check before delegating to the slow path.
Attachment #8782977 - Flags: review?(wchen)
Attachment #8782778 - Attachment is obsolete: true
Attachment #8782778 - Flags: review?(wchen)
Comment on attachment 8782734 [details] [diff] [review]
Part 1 - Devirtualize and inline GetShadowRoot. v1

># HG changeset patch
># User Bobby Holley <bobbyholley@gmail.com>
>
>Bug 1296509 - Devirtualize and inline GetShadowRoot. v1
>
>diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp
>index 5f76be0..646891d 100644
>--- a/dom/base/FragmentOrElement.cpp
>+++ b/dom/base/FragmentOrElement.cpp
>@@ -1061,26 +1061,16 @@ FragmentOrElement::GetXBLInsertionParent() const
>       return slots->mXBLInsertionParent;
>     }
>   }
> 
>   return nullptr;
> }
> 
> ShadowRoot*
>-FragmentOrElement::GetShadowRoot() const
>-{
>-  nsDOMSlots *slots = GetExistingDOMSlots();
>-  if (slots) {
>-    return slots->mShadowRoot;
>-  }
>-  return nullptr;
>-}
>-
>-ShadowRoot*
> FragmentOrElement::GetContainingShadow() const
> {
>   nsDOMSlots *slots = GetExistingDOMSlots();
>   if (slots) {
>     return slots->mContainingShadow;
>   }
>   return nullptr;
> }
>diff --git a/dom/base/FragmentOrElement.h b/dom/base/FragmentOrElement.h
>index bf7f5ef..fd44b37 100644
>--- a/dom/base/FragmentOrElement.h
>+++ b/dom/base/FragmentOrElement.h
>@@ -143,17 +143,20 @@ public:
>   virtual bool HasTextForTranslation() override;
>   virtual void AppendTextTo(nsAString& aResult) override;
>   MOZ_MUST_USE
>   virtual bool AppendTextTo(nsAString& aResult, const mozilla::fallible_t&) override;
>   virtual nsIContent *GetBindingParent() const override;
>   virtual nsXBLBinding *GetXBLBinding() const override;
>   virtual void SetXBLBinding(nsXBLBinding* aBinding,
>                              nsBindingManager* aOldBindingManager = nullptr) override;
>-  virtual ShadowRoot *GetShadowRoot() const override;
>+  ShadowRoot *FastGetShadowRoot() const {
Nit, { goes to its own line.



>+inline mozilla::dom::ShadowRoot* nsIContent::GetShadowRoot() const
>+{
>+  uint16_t type = NodeType();
>+  if (type != nsIDOMNode::ELEMENT_NODE &&
>+      type != nsIDOMNode::DOCUMENT_FRAGMENT_NODE)
>+  {
Nit, { goes to the previous line :)


So, fragments shouldn't have shadowroot. Only Element calls SetShadowRoot on itself
http://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom17FragmentOrElement13SetShadowRootEPNS0_10ShadowRootE&redirect=false
So, the check could be
if (!IsElement()) {
  return nullptr;
}

And would be good to add MOZ_ASSERT(IsElement()); to FragmentOrElement::SetShadowRoot,
or even better, just move SetShadowRoot() to Element. Could you do that even in this bug.
Attachment #8782734 - Flags: review?(wchen) → review+
Comment on attachment 8782977 [details] [diff] [review]
Part 2 - Optimize GetFlattenedTreeParent. v3

>   /**
>-   * Returns the content node that is the parent of this node in the flattened
>-   * tree. For nodes that are not filtered into an insertion point, this
>-   * simply returns their DOM parent in the original DOM tree.
>-   *
>-   * @return the flattened tree parent
>+   * Same as GetFlattenedTreeParentNode, but returns null if the parent is
>+   * non-nsIContent.
>    */
>-  nsIContent *GetFlattenedTreeParent() const;
>+  inline nsIContent *GetFlattenedTreeParent() const;
>+
>+  /**
>+   * Helper method, which we leave public so that it's accessible from nsINode.
>+   */
>+  nsINode *GetFlattenedTreeParentNodeInternal() const;

Nit, nsIFoo*, not nsIFoo *



>+inline nsINode* nsINode::GetFlattenedTreeParentNode() const
>+{
>+  nsINode* parent = GetParentNode();
>+
>+  // Try to short-circuit past the complicated and not-exactly-fast logic for
>+  // computing the flattened parent.
>+  //
>+  // There are four cases where we need might something other than parentNode:
>+  //   (1) The node is in an anonymous XBL subtree.
>+  //   (2) The node is an explicit child of an XBL-bound element, re-bound
>+  //       to an XBL insertion point.
>+  //   (3) The node is in a shadow tree.
>+  //   (4) The node is an explicit child of an element with a shadow root,
>+  //       re-bound to an insertion point.
>+  //
>+  // If either (1) or (2) is true, NODE_MAY_BE_IN_BINDING_MNGR will be set.
(1) doesn't mean NODE_MAY_BE_IN_BINDING_MNGR. It means that binding manager somehow explicitly manages that node, but there
is anonymous content which isn't marked with NODE_MAY_BE_IN_BINDING_MNGR. But since GetParentNode() links work fine when crossing anonymous - non-anonymous boundary, but you you still
insertion point check, this should be fine.
But please remove comment (1)

>+  // We need to check (3) and (4) separately.
>+  bool needSlowCall = HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) ||
>+                      IsInShadowTree() ||
>+                      (parent && parent->IsContent() &&
>+                       parent->AsContent()->GetShadowRoot());
ok, and we don't need check for native anonymous content since GetParentNode() crosses NAC-nonNAC boundary just fine.

>   /**
>+   * Returns the node that is the parent of this node in the flattened
>+   * tree. For nodes that are not filtered into an insertion point, this
>+   * simply returns their DOM parent in the original DOM tree.
I know this is copied but this isn't quite exact, since it may return binding parent when crossing anon boundaries.
Attachment #8782977 - Flags: review?(wchen) → review+
Attachment #8782977 - Attachment description: Part 3 - Optimize GetFlattenedTreeParent. v3 → Part 1 - Optimize GetFlattenedTreeParent. v3
Attachment #8782977 - Attachment description: Part 1 - Optimize GetFlattenedTreeParent. v3 → Part 2 - Optimize GetFlattenedTreeParent. v3
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c95e0617d78
Devirtualize and inline GetShadowRoot. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4687780af322
Optimize GetFlattenedTreeParent. r=smaug
https://hg.mozilla.org/mozilla-central/rev/6c95e0617d78
https://hg.mozilla.org/mozilla-central/rev/4687780af322
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.