Closed Bug 1342197 Opened 7 years ago Closed 7 years ago

Speed up GetSelectionRange in text controls

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(4 files, 1 obsolete file)

It's doing all sorts of slow stuff.
MozReview-Commit-ID: BmTw3rAzCuc
Attachment #8840612 - Flags: review?(ehsan)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
At this point, all this method does is ensure editor initialization and then ask
the editor state for various information.  Let's cut out the middleman.

MozReview-Commit-ID: p491umScJO
Attachment #8840616 - Flags: review?(ehsan)
With those patches, we're about 4-5x faster than before, but still about 2x slower than WebKit.  The remaining sources of slowness are:

1)  Virtual GetSelection on the selection controller, plus refcounting.
2)  The various stuff around the frame (flushing, EnsureEditorInitialized, etc; is any of this needed?).
3)  GetSelectionInTextControl making calls on the dom::Selection, plus self time.

Just for scale, at this point self time in HTMLInputElement::GetSelectionRange is about 10% of total testcase time on the microbenchmark in bug 1341768.  So none of these things are _really_ that slow.  They're just slower than not doing them.
Summary: Speed up nsTextControlFrame::GetSelectionRange → Speed up GetSelectionRange in text controls
I would be interested in thoughts on speeding up the remaining bits from comment 5...  Most obviously by simplifying this stuff somehow.
Flags: needinfo?(ehsan)
Comment on attachment 8840616 [details] [diff] [review]
part 4.  Move GetSelectionRange from nsTextControlFrame to the editor state

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e655a6121fc7c5aa665114bd206e3e7a8c66a808&selectedJob=79770671 says I missed something.

Need to check.
Attachment #8840616 - Flags: review?(ehsan) → review-
At this point, all this method does is ensure editor initialization and then ask
the editor state for various information.  Let's cut out the middleman.

MozReview-Commit-ID: p491umScJO
Attachment #8840744 - Flags: review?(ehsan)
Attachment #8840616 - Attachment is obsolete: true
Attachment #8840612 - Flags: review?(ehsan) → review+
Attachment #8840613 - Flags: review?(ehsan) → review+
Comment on attachment 8840615 [details] [diff] [review]
part 3.  Speed up nsContentUtils::GetSelectionInTextControl a bit by removing some refcounting and virtual calls

Review of attachment 8840615 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentUtils.cpp
@@ +7071,5 @@
>  
>    // We have at most two children, consisting of an optional text node followed
>    // by an optional <br>.
>    NS_ASSERTION(aRoot->GetChildCount() <= 2, "Unexpected children");
> +  nsIContent* firstChild = aRoot->GetFirstChild();

Can you please add some comment here noting that these variables are raw pointers for performance reasons?
Attachment #8840615 - Flags: review?(ehsan) → review+
Comment on attachment 8840744 [details] [diff] [review]
part 4.  Move GetSelectionRange from nsTextControlFrame to the editor state

Review of attachment 8840744 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLInputElement.cpp
@@ +6510,5 @@
> +    // Can we return a selection range anyway here, now that it lives on our
> +    // state?  In fact, could we make this behave more like
> +    // GetSelectionDirection, in the sense of working even when we have no
> +    // frame, by just delegating entirely to mState?  And then, do we really
> +    // need the flush?

Ditto.

@@ +6567,5 @@
> +    DirectionToName(state->GetSelectionProperties().GetDirection(), aDirection);
> +    return;
> +  }
> +
> +  aRv.Throw(rv);

Same.

::: dom/html/HTMLTextAreaElement.cpp
@@ +852,5 @@
> +    // Can we return a selection range anyway here, now that it lives on our
> +    // state?  In fact, could we make this behave more like
> +    // GetSelectionDirection, in the sense of working even when we have no
> +    // frame, by just delegating entirely to mState?  And then, do we really
> +    // need the flush?

Doing that sounds like a great idea to me, and I think it should be possible.  Follow-up?

@@ +902,3 @@
>    }
> +
> +  aError.Throw(rv);

Maybe throw a MOZ_ASSERT(NS_FAILED(rv)) before this also?

::: dom/html/nsITextControlElement.h
@@ +201,5 @@
> +   * Get the selection range start and end points.
> +   */
> +  NS_IMETHOD GetSelectionRange(int32_t* aSelectionStart,
> +                               int32_t* aSelectionEnd) = 0;
> +   

Nit: trailing whitespace

::: layout/forms/nsTextControlFrame.cpp
@@ +939,5 @@
>  
>    int32_t selStart = 0, selEnd = 0;
>  
> +  nsCOMPtr<nsITextControlElement> txtCtrl = do_QueryInterface(GetContent());
> +  NS_ASSERTION(txtCtrl, "Content not a text control element");

These need to be MOZ_ASSERTs.
Attachment #8840744 - Flags: review?(ehsan) → review+
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #5)
> With those patches, we're about 4-5x faster than before, but still about 2x
> slower than WebKit.  The remaining sources of slowness are:
> 
> 1)  Virtual GetSelection on the selection controller, plus refcounting.

I can't imagine how we would avoid the former, but the latter should be easy to avoid and will save some indirect calls.

> 2)  The various stuff around the frame (flushing, EnsureEditorInitialized,
> etc; is any of this needed?).

The flushing business shouldn't be needed.  It'd be nice if you can try and take them out.

> 3)  GetSelectionInTextControl making calls on the dom::Selection, plus self
> time.

Anything there that stands out?  Do you have a link to the profile post these changes to share?

> Just for scale, at this point self time in
> HTMLInputElement::GetSelectionRange is about 10% of total testcase time on
> the microbenchmark in bug 1341768.  So none of these things are _really_
> that slow.  They're just slower than not doing them.

Fantastic.  Thanks for doing this.
Flags: needinfo?(ehsan)
> Can you please add some comment here noting that these variables are raw pointers for performance reasons?

Done.

> Doing that sounds like a great idea to me, and I think it should be possible.  Follow-up?

Bug 1343037.

> Maybe throw a MOZ_ASSERT(NS_FAILED(rv)) before this also?

ErrorResult::Throw does that already.

> Nit: trailing whitespace

Fixed.

> These need to be MOZ_ASSERTs.

Done.

> but the latter should be easy to avoid and will save some indirect calls

As in, just add a non-XPCOM getter for an nsISelection* on selection controller?

> The flushing business shouldn't be needed.  It'd be nice if you can try and take them out.

I expect I can do that in bug 1343037.

> Anything there that stands out?  Do you have a link to the profile post these changes to share?

I'll get one up; I want to give bug 1343037 a shot and see what it looks like after that.
Flags: needinfo?(ehsan)
> Anything there that stands out?  Do you have a link to the profile post these changes to share?

With bug 1343037 there are now two cases.  Profile when the editor is not initialized:

https://perf-html.io/public/b28b23739158029731052aa67ed637ebd6a53b1f/calltree/?thread=0

Profile when the editor is initialized:

https://perf-html.io/public/5cd804da2198a5f285632ebeb79b2fcc0866ff30/calltree/?thread=0

The former is at about performance parity with WebKit.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #12)
> > but the latter should be easy to avoid and will save some indirect calls
> 
> As in, just add a non-XPCOM getter for an nsISelection* on selection
> controller?

Yeah.

Thanks for addressing the rest of the comments.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> > Anything there that stands out?  Do you have a link to the profile post these changes to share?
> 
> With bug 1343037 there are now two cases.  Profile when the editor is not
> initialized:
> 
> https://perf-html.io/public/b28b23739158029731052aa67ed637ebd6a53b1f/
> calltree/?thread=0
> 
> Profile when the editor is initialized:
> 
> https://perf-html.io/public/5cd804da2198a5f285632ebeb79b2fcc0866ff30/
> calltree/?thread=0
> 
> The former is at about performance parity with WebKit.

Sigh, so with the editor being around GetSelectionInTextControl is still pretty costly (I assume these profiles were using the same number of iterations in the test case?)

Perhaps after we took care of GetSelection, we should consider inlining these methods <http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/layout/generic/Selection.h#164> and then remeasure again?
Flags: needinfo?(ehsan)
> I assume these profiles were using the same number of iterations in the test case?

No, they were not.  The "profile when the editor is initialized" used 3x fewer.  That's why its overall time is a bit lower, instead of 2x higher, than the other profile.  I just didn't want to wait 15 seconds...

I'll do the non-xpcom getter and look at the selection getters to see what makes the most sense, then remeasure.
> I'll do the non-xpcom getter and look at the selection getters 

I filed bug 1343275 to track this.  With the patches there, I have the selection controller codepath pretty close to parity with the cached selection codepath.
Blocks: 1343037
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e7d6509b02a
part 1.  Change nsITextControlElement::GetRootEditorNode to return Element*.  r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/c52653ed3dd7
part 2.  Use nsITextControlElement::GetRootEditorNode to get the root editor node in nsTextControlFrame::GetSelectionRange.  r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/31bc78fa4886
part 3.  Speed up nsContentUtils::GetSelectionInTextControl a bit by removing some refcounting and virtual calls.  r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/17a2b5d9827b
part 4.  Move GetSelectionRange from nsTextControlFrame to the editor state.  r=ehsan
Depends on: 1343886
You need to log in before you can comment on or make changes to this bug.