1296509 - Make GetFlattenedTreeParent common-case fast

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Devirtualize and inline GetShadowRoot. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Optimize GetFlattenedTreeParent. v1

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.

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03807d23699e

Comment 4

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a45314bfff4

Comment 5

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=252d70085597

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Optimize GetFlattenedTreeParent. v2

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.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Optimize GetFlattenedTreeParent. v3

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.

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c84af8e0fbae

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8782734 [details] [diff] [review]
Part 1 - Devirtualize and inline GetShadowRoot. v1

># HG changeset patch
># User Bobby Holley <[e-mail]>
>
>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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c95e0617d78
https://hg.mozilla.org/mozilla-central/rev/4687780af322