1391165 - Optimize TextEditRules::CollapseSelectionToTrailingBRIfNeeded()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1391165 - part1: EditorBase::GetChildOffset() and EditorBase::GetNodeLocation() shouldn't use nsINode::IndexOf() as far as possible

nsINode::IndexOf() is expensive especially when it's in a hot path, it's too
expensive.  So, EditorBase::GetChildOffset() and EditorBase::GetNodeLocation()
should check child's siblings first.  If some of them are nullptr, it means
that it's first child or last child of the parent.

Note that EditorBase::GetChildOffset() may return wrong index if it's called
while aChild is being removed from aParent or aParent isn't actual parent of
aChild.  However, they are check if result is -1 with MOZ_ASSERT().  Therefore,
it's safe to assume that aParent is always the parent of aChild and it won't
be called from ContentRemoved() of mutation observers.

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1391165 - part2: TextEditRules::CollapseSelectionToTrailingBRIfNeeded() should use nsINode::GetNextSibling() for retrieving next child of a node

nsINode::GetChildAt() is too expensive if it's used for next sibling of a node.

This patch makes it use nsINode::GetNextSibling() instead.

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1391165 - part3: TextEditRules::CollapseSelectionToTrailingBRIfNeeded() should use EditorBase::GetChildOffset() only when it's necessary

In most cases, TextEditRules::CollapseSelectionToTrailingBRIfNeeded() doesn't
use the index anymore.  Therefore, it should stop using
EditorBase::GetNodeLocation() and use EditorBase::GetChildOffset() only when
the offset is necessary.

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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1391165 - part4: nsIEditor::EndOfDocument() should be implemented with an internal method which takes pointer to Selection

EditorBase::GetSelection() sometimes appears in profile.  So, it shouldn't be
called in nsIEditor::EndOfDocument() if the callers of
nsIEditor::EndOfDocument() has a pointer to Selection.

This patch adds EditorBase::CollapseSelectionToEnd() for the internal method
and make all callers of nsIEditor::EndOfDocument() use it.

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

Comment 9

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

Comment on attachment 8898528 [details]
Bug 1391165 - part1: EditorBase::GetChildOffset() and EditorBase::GetNodeLocation() shouldn't use nsINode::IndexOf() as far as possible

https://reviewboard.mozilla.org/r/169594/#review175220

::: commit-message-15633:10
(Diff revision 1)
> +should check child's siblings first.  If some of them are nullptr, it means
> +that it's first child or last child of the parent.
> +
> +Note that EditorBase::GetChildOffset() may return wrong index if it's called
> +while aChild is being removed from aParent or aParent isn't actual parent of
> +aChild.  However, they are check if result is -1 with MOZ_ASSERT().  Therefore,

I don't understand "However, they are check if result is -1 with MOZ_ASSERT()."
Perhaps "There are MOZ_ASSERTs to ensure value isn't -1."

::: editor/libeditor/EditorBase.cpp:3186
(Diff revision 1)
> +
> +  // nsINode::IndexOf() is expensive.  So, if we can return index without
> +  // calling it, we should do that.
> +
> +  // If there is no previous siblings, it means that it's the first child.
> +  if (!aChild->GetPreviousSibling()) {

I'd prefer
if (aParent->GetFirstChild() == aChild)

::: editor/libeditor/EditorBase.cpp:3192
(Diff revision 1)
> +    MOZ_ASSERT(aParent->IndexOf(aChild) == 0);
> +    return 0;
> +  }
> +
> +  // If there is no next siblings, it means that it's the last child.
> +  if (!aChild->GetNextSibling()) {

and here I'd prefer if (aParent->GetLastChild() == child)

::: editor/libeditor/EditorBase.cpp:3193
(Diff revision 1)
> +    return 0;
> +  }
> +
> +  // If there is no next siblings, it means that it's the last child.
> +  if (!aChild->GetNextSibling()) {
> +    int32_t lastChildIndex = static_cast<int32_t>(aParent->Length() - 1);

Hopefully Length() won't become slow once bug 651120 lands.

Comment 10

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

Comment on attachment 8898529 [details]
Bug 1391165 - part2: TextEditRules::CollapseSelectionToTrailingBRIfNeeded() should use nsINode::GetNextSibling() for retrieving next child of a node

https://reviewboard.mozilla.org/r/169596/#review175228

Comment 11

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

Catalin, you may want to take a look at the patches too.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 13

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

Comment on attachment 8898530 [details]
Bug 1391165 - part3: TextEditRules::CollapseSelectionToTrailingBRIfNeeded() should use EditorBase::GetChildOffset() only when it's necessary

https://reviewboard.mozilla.org/r/169598/#review175234

Comment 14

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

Comment on attachment 8898531 [details]
Bug 1391165 - part4: nsIEditor::EndOfDocument() should be implemented with an internal method which takes pointer to Selection

https://reviewboard.mozilla.org/r/169600/#review175236

::: editor/libeditor/EditorBase.cpp:1161
(Diff revision 1)
> +  }
>  
> -  while (child && IsContainer(child->AsDOMNode())) {
> +  nsINode* child = node->GetLastChild();
> +  while (child && IsContainer(child)) {
>      node = child;
> +    // XXX nsINode::GetLastChild() is too expensive if the DOM tree is deep.

Why is GetLastChild expensive? I don't think so.

Comment 15

[Cătălin Badea (:catalinb)]

Just to clear this up, Length() and GetLastChild() won't be slower after bug 651120.

The changes here look good and don't interfere with what I'm doing for bug 651120. If I understand this correctly, GetChildOffset() is needed here to ultimately position ranges. If so, bug 1380367 might help by allowing to position the range on (ParentNode, child) boundaries and avoid some IndexOf calls.

Do you have a testcase/benchmark for this? I would like to test this with the patch from bug 1380367.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8898531 [details]
> Bug 1391165 - part4: nsIEditor::EndOfDocument() should be implemented with
> an internal method which takes pointer to Selection
> 
> https://reviewboard.mozilla.org/r/169600/#review175236
> 
> ::: editor/libeditor/EditorBase.cpp:1161
> (Diff revision 1)
> > +  }
> >  
> > -  while (child && IsContainer(child->AsDOMNode())) {
> > +  nsINode* child = node->GetLastChild();
> > +  while (child && IsContainer(child)) {
> >      node = child;
> > +    // XXX nsINode::GetLastChild() is too expensive if the DOM tree is deep.
> 
> Why is GetLastChild expensive? I don't think so.

(In reply to Cătălin Badea (:catalinb) from comment #15)
> Just to clear this up, Length() and GetLastChild() won't be slower after bug
> 651120.

Oh, really? It uses GetChildArray() and accesses like array to it. Then, why GetChildAt() will be slow down?

> The changes here look good and don't interfere with what I'm doing for bug
> 651120. If I understand this correctly, GetChildOffset() is needed here to
> ultimately position ranges.

Exactly.

> If so, bug 1380367 might help by allowing to
> position the range on (ParentNode, child) boundaries and avoid some IndexOf
> calls.

Oh, that's what I want!

> Do you have a testcase/benchmark for this? I would like to test this with
> the patch from bug 1380367.

Unfortunately, no testcases which exactly measure this code.

I found this in the profile of attachment 8848015 [details]. They will improve 1ms ~ 2ms of the score in my environment.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8898528 [details]
Bug 1391165 - part1: EditorBase::GetChildOffset() and EditorBase::GetNodeLocation() shouldn't use nsINode::IndexOf() as far as possible

https://reviewboard.mozilla.org/r/169594/#review175220

> I don't understand "However, they are check if result is -1 with MOZ_ASSERT()."
> Perhaps "There are MOZ_ASSERTs to ensure value isn't -1."

Oops, thanks!

> I'd prefer
> if (aParent->GetFirstChild() == aChild)

Okay, the cost shouldn't be changed in this case.

> and here I'd prefer if (aParent->GetLastChild() == child)

Oh, really? Okay, I'll change so.

> Hopefully Length() won't become slow once bug 651120 lands.

Yeah, Length() should just return child count...

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8898531 [details]
Bug 1391165 - part4: nsIEditor::EndOfDocument() should be implemented with an internal method which takes pointer to Selection

https://reviewboard.mozilla.org/r/169600/#review175236

> Why is GetLastChild expensive? I don't think so.

Just remove this comment.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8898528 [details]
Bug 1391165 - part1: EditorBase::GetChildOffset() and EditorBase::GetNodeLocation() shouldn't use nsINode::IndexOf() as far as possible

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

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8898529 [details]
Bug 1391165 - part2: TextEditRules::CollapseSelectionToTrailingBRIfNeeded() should use nsINode::GetNextSibling() for retrieving next child of a node

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

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8898530 [details]
Bug 1391165 - part3: TextEditRules::CollapseSelectionToTrailingBRIfNeeded() should use EditorBase::GetChildOffset() only when it's necessary

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

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8898531 [details]
Bug 1391165 - part4: nsIEditor::EndOfDocument() should be implemented with an internal method which takes pointer to Selection

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b8d070ae361
part1: EditorBase::GetChildOffset() and EditorBase::GetNodeLocation() shouldn't use nsINode::IndexOf() as far as possible r=smaug
https://hg.mozilla.org/integration/autoland/rev/b1348dd280c3
part2: TextEditRules::CollapseSelectionToTrailingBRIfNeeded() should use nsINode::GetNextSibling() for retrieving next child of a node r=smaug
https://hg.mozilla.org/integration/autoland/rev/1ec7f4653e3c
part3: TextEditRules::CollapseSelectionToTrailingBRIfNeeded() should use EditorBase::GetChildOffset() only when it's necessary r=smaug
https://hg.mozilla.org/integration/autoland/rev/79e7b5f9b3b6
part4: nsIEditor::EndOfDocument() should be implemented with an internal method which takes pointer to Selection r=smaug

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0b8d070ae361
https://hg.mozilla.org/mozilla-central/rev/b1348dd280c3
https://hg.mozilla.org/mozilla-central/rev/1ec7f4653e3c
https://hg.mozilla.org/mozilla-central/rev/79e7b5f9b3b6