Closed
Bug 1342197
Opened 8 years ago
Closed 8 years ago
Speed up GetSelectionRange in text controls
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
It's doing all sorts of slow stuff.
Assignee | ||
Comment 1•8 years ago
|
||
MozReview-Commit-ID: BmTw3rAzCuc
Attachment #8840612 -
Flags: review?(ehsan)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
MozReview-Commit-ID: 7ncJVNVGF78
Attachment #8840613 -
Flags: review?(ehsan)
Assignee | ||
Comment 3•8 years ago
|
||
MozReview-Commit-ID: GuBVki5QFwz
Attachment #8840615 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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-
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8840616 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8840612 -
Flags: review?(ehsan) → review+
Updated•8 years ago
|
Attachment #8840613 -
Flags: review?(ehsan) → review+
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
(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)
Assignee | ||
Comment 12•8 years ago
|
||
> 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)
Assignee | ||
Comment 13•8 years ago
|
||
> 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.
Comment 14•8 years ago
|
||
(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)
Assignee | ||
Comment 15•8 years ago
|
||
> 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.
Assignee | ||
Comment 16•8 years ago
|
||
> 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.
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2e7d6509b02a
https://hg.mozilla.org/mozilla-central/rev/c52653ed3dd7
https://hg.mozilla.org/mozilla-central/rev/31bc78fa4886
https://hg.mozilla.org/mozilla-central/rev/17a2b5d9827b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•