1427511 - Make GetFlattenedTreeParent more straight-forward.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

Now that a bunch of the stuff we need to implement it isn't virtual (bug 1427001), we can speed it up a fair bit in the rare case while keeping the normal case fast.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1427511: Make GetFlattenedTreeParent more straight-forward.

Now that accessing nsIContent slots is not a blob of virtual function calls, we
should be able to unify logic here, and speed up the not-so-rare case for
chrome, while keeping the usual case fast.

Review commit: https://reviewboard.mozilla.org/r/209710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209710/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8939278 [details]
Bug 1427511: Make GetFlattenedTreeParent more straight-forward.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209710/diff/1-2/

Comment 3

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

Comment on attachment 8939278 [details]
Bug 1427511: Make GetFlattenedTreeParent more straight-forward.

https://reviewboard.mozilla.org/r/209710/#review215254

Could I get some clarifications and I think IsShadowRoot() should live in nsINode

::: dom/base/FragmentOrElement.cpp:196
(Diff revision 2)
> -
> -  return parentSlot;
> -}
>  
> -nsINode*
> -nsIContent::GetFlattenedTreeParentNodeInternal(FlattenedParentType aType) const
> +  nsIContent* parent = GetParent();
> +  if (parent != OwnerDoc()->GetRootElement()) {

So MOZ_ASSERT(!GetParent()->GetParent()) but
then we still may have parent which isn't root element.
Could you explain this a bit more? Add some comment.

::: dom/base/nsIContent.h:191
(Diff revision 2)
>    {
>      return HasFlag(NODE_IS_NATIVE_ANONYMOUS_ROOT |
>                     NODE_IS_ROOT_OF_CHROME_ONLY_ACCESS);
>    }
>  
> +  bool IsShadowRoot() const

Why is this in nsIContent? Looks like the code would be a tad simpler if this lived in nsINode

::: dom/base/nsIContent.h:621
(Diff revision 2)
> -  inline nsIContent *GetFlattenedTreeParent() const;
> +  inline nsIContent* GetFlattenedTreeParent() const;
>  
> -  // Helper method, which we leave public so that it's accessible from nsINode.
> -  enum FlattenedParentType { eNotForStyle, eForStyle };
> -  nsINode* GetFlattenedTreeParentNodeInternal(FlattenedParentType aType) const;
> +  /**
> +   * Get the flattened tree parent for NAC holding from the root element.
> +   */
> +  nsINode* GetFlattenedTreeParentForRootElementNAC() const;

I have real trouble to understand what the method returns.
Root element of what?
Is this about the document element, top most element?

::: dom/base/nsNodeUtils.cpp:85
(Diff revision 2)
>        NS_OBSERVER_AUTO_ARRAY_NOTIFY_OBSERVERS(                              \
>          slots->mMutationObservers, nsIMutationObserver, 1,                  \
>          func_, params_);                                                    \
>      }                                                                       \
>      last = node;                                                            \
> -    if (ShadowRoot* shadow = ShadowRoot::FromNode(node)) {                  \
> +    if (node->IsContent() && node->AsContent()->IsShadowRoot()) {           \

Doesn't this make the code a tad slower.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8939278 [details]
Bug 1427511: Make GetFlattenedTreeParent more straight-forward.

https://reviewboard.mozilla.org/r/209710/#review215254

> Why is this in nsIContent? Looks like the code would be a tad simpler if this lived in nsINode

Sure

> I have real trouble to understand what the method returns.
> Root element of what?
> Is this about the document element, top most element?

It should be the root element, will add some comments.

> Doesn't this make the code a tad slower.

How? `IsContent()` is a bit check, and `IsShadowRoot` is cheap. Before this it used to be a function call that contained more. But yeah I guess the `IsContent` check is unnecessary if it lives in `nsINode`.

Comment 5

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

yeah, I was thinking that IsContent() check.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8939278 [details]
Bug 1427511: Make GetFlattenedTreeParent more straight-forward.

https://reviewboard.mozilla.org/r/209710/#review215254

> So MOZ_ASSERT(!GetParent()->GetParent()) but
> then we still may have parent which isn't root element.
> Could you explain this a bit more? Add some comment.

You can technically be in a detached subtree where the parent doesn't have a parent. Checking `!GetParent()->GetParent()` seemed faster than checking `OwnerDoc()->GetRootElement() == GetParent()`. But it's not much of a difference I guess.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8939278 [details]
Bug 1427511: Make GetFlattenedTreeParent more straight-forward.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209710/diff/2-3/

Comment 8

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

Comment on attachment 8939278 [details]
Bug 1427511: Make GetFlattenedTreeParent more straight-forward.

https://reviewboard.mozilla.org/r/209710/#review215290

::: dom/base/nsIContent.h:618
(Diff revision 3)
> +   *
> +   * Document-level anonymous content holds from the root element, even though
> +   * they should not be treated as such (they should be parented to the document
> +   * instead, and shouldn't inherit from the document element).
> +   */
> +  nsINode* GetFlattenedTreeParentForRootElementNAC() const;

I know it is longer, but could you call this
...DocumentElementNAC, since that would map to the spec terminology. Root of an nsIContent (Element for example) can be something else too than document.
And this method seems to deal with the case only when root is document.

::: dom/base/nsIContentInlines.h:67
(Diff revision 3)
> -template<nsIContent::FlattenedParentType Type>
> -static inline bool FlattenedTreeParentIsParent(const nsINode* aNode)
> +template<bool aForStyle>
> +static inline nsINode*
> +GetFlattenedTreeParentNode(const nsINode* aNode)
>  {
> -  // Try to short-circuit past the complicated and not-exactly-fast logic for
> -  // computing the flattened parent.
> +  nsINode* parent = aNode->GetParentNode();
> +  if (!aNode->IsContent()) {

What is this case trying to capture? When is aNode non-nsIContent, yet it has a parent node?

::: dom/base/nsIContentInlines.h:90
(Diff revision 3)
>  
> -  // Common case.
> -  return true;
> +  if (content->IsRootOfAnonymousSubtree()) {
> +    return parent;
> -}
> +  }
>  
> -template<nsIContent::FlattenedParentType Type>
> +  if (parentAsContent->GetShadowRoot()) {

This could use a comment that if parent has shadow root, yet this isn't assigned to any slot, we return null.

::: dom/base/nsIContentInlines.h:96
(Diff revision 3)
> -static inline nsINode*
> -GetFlattenedTreeParentNode(const nsINode* aNode)
> -{
> -  if (MOZ_LIKELY(FlattenedTreeParentIsParent<Type>(aNode))) {
> -    return aNode->GetParentNode();
> +    return content->GetAssignedSlot();
> +  }
> +
> +  if (parentAsContent->IsInShadowTree()) {
> +    if (auto* slot = mozilla::dom::HTMLSlotElement::FromContent(parentAsContent)) {
> +      return slot->AssignedNodes().IsEmpty()

And this could use a comment too

::: dom/base/nsIContentInlines.h:108
(Diff revision 3)
> +    }
> +  }
> +
> +  if (content->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) ||
> +      parent->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {
> +    if (auto* xblInsertionPoint = content->GetXBLInsertionPoint()) {

Don't use auto here. It isn't clear what the type is.

::: dom/base/nsIContentInlines.h:129
(Diff revision 3)
>  }
>  
>  inline nsINode*
>  nsINode::GetFlattenedTreeParentNode() const
>  {
> -  return ::GetFlattenedTreeParentNode<nsIContent::eNotForStyle>(this);
> +  return ::GetFlattenedTreeParentNode<false>(this);

It is unclear to me why you make the template take bool and not the existing easier to understand enum.
Please explain or keep using the enum.

::: dom/base/nsNodeUtils.cpp:121
(Diff revision 3)
>      if (slots && !slots->mMutationObservers.IsEmpty()) {          \
>        NS_OBSERVER_AUTO_ARRAY_NOTIFY_OBSERVERS_WITH_QI(            \
>          slots->mMutationObservers, nsIMutationObserver, 1,        \
>          nsIAnimationObserver, func_, params_);                    \
>      }                                                             \
> -    ShadowRoot* shadow = ShadowRoot::FromNode(node);              \
> +    if (ShadowRoot* shadow = ShadowRoot::FromNode(node)) {        \

Unrelated change, but fine.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8939278 [details]
Bug 1427511: Make GetFlattenedTreeParent more straight-forward.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209710/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8656e137157
Make GetFlattenedTreeParent more straight-forward. r=smaug

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b8656e137157