Optimize TextEditRules::CollapseSelectionToTrailingBRIfNeeded()

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments)

Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

2 years ago
Duplicate of this bug: 1391320

Comment 9

2 years ago
mozreview-review
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.
Attachment #8898528 - Flags: review?(bugs) → review+

Comment 10

2 years ago
mozreview-review
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
Attachment #8898529 - Flags: review?(bugs) → review+
Catalin, you may want to take a look at the patches too.
Flags: needinfo?(catalin.badea392)

Comment 13

2 years ago
mozreview-review
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
Attachment #8898530 - Flags: review?(bugs) → review+

Comment 14

2 years ago
mozreview-review
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.
Attachment #8898531 - Flags: review?(bugs) → review+
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.
Flags: needinfo?(catalin.badea392)
(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.
(Assignee)

Comment 17

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

Comment 18

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
Pushed by masayuki@d-toybox.com:
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

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.