Make GetFlattenedTreeParent common-case fast

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8782734 [details] [diff] [review]
Part 1 - Devirtualize and inline GetShadowRoot. v1
Attachment #8782734 - Flags: review?(wchen)
(Assignee)

Comment 2

2 years ago
Created attachment 8782735 [details] [diff] [review]
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.
Attachment #8782735 - Flags: review?(wchen)
(Assignee)

Comment 6

2 years ago
Created attachment 8782778 [details] [diff] [review]
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.
Attachment #8782778 - Flags: review?(wchen)
(Assignee)

Updated

2 years ago
Attachment #8782735 - Attachment is obsolete: true
Attachment #8782735 - Flags: review?(wchen)
(Assignee)

Comment 7

2 years ago
Created attachment 8782977 [details] [diff] [review]
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.
Attachment #8782977 - Flags: review?(wchen)
(Assignee)

Updated

2 years ago
Attachment #8782778 - Attachment is obsolete: true
Attachment #8782778 - Flags: review?(wchen)

Comment 9

2 years ago
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+
(Assignee)

Updated

2 years ago
Attachment #8782977 - Attachment description: Part 3 - Optimize GetFlattenedTreeParent. v3 → Part 1 - Optimize GetFlattenedTreeParent. v3
(Assignee)

Updated

2 years ago
Attachment #8782977 - Attachment description: Part 1 - Optimize GetFlattenedTreeParent. v3 → Part 2 - Optimize GetFlattenedTreeParent. v3

Comment 11

2 years ago
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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c95e0617d78
https://hg.mozilla.org/mozilla-central/rev/4687780af322
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.