1390382 - Make nsCaret::NotifySelectionChanged() less expensive

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[(no longer active)]

This function takes about 6% of the time under HTMLInputElement::SetValueInternal() in Speedometer.

Comment 1

[(no longer active)]

Attachment: Part 1: Inline Selection::IsCollapsed()

Comment 2

[(no longer active)]

Attachment: Part 2: Inline nsCaret::IsVisible()

Comment 3

[(no longer active)]

Attachment: Part 3: Avoid recomputing the selection in nsCaret::SchedulePaint() when the caller has the information available

Comment 4

[(no longer active)]

Attachment: Part 4 : Only call nsLayoutUtils::GetDisplayRootFrame() once per call to nsIFrame::SchedulePaint()

nsLayoutUtils::GetDisplayRootFrame() can be quite expensive to call.
By calling this function one level higher in callers and passing the return
value to callees, we can avoid calling it twice per call to SchedulePaint().

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8897257 [details] [diff] [review]
Part 2: Inline nsCaret::IsVisible()

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

::: layout/base/nsCaret.cpp
@@ +583,5 @@
>  NS_IMETHODIMP
>  nsCaret::NotifySelectionChanged(nsIDOMDocument *, nsISelection *aDomSel,
>                                  int16_t aReason)
>  {
> +  if ((aReason & nsISelectionListener::MOUSEUP_REASON) || !IsVisible(aDomSel))//this wont do

Two things:
 (1) This conversion -- passing in aDomSel here -- would be better to make in a separate commit, because it's not part of what this commit message says (nsCaret::IsVisible()).

 (2) I think this change (passing in aDomSel) is potentially a behavior change, right?  It looks like you're assuming aDomSel is the same object that IsVisible would've used anyway (i.e. it's the selection that IsVisible would get from GetSelectionInternal(), which is mDomSelectionWeak).  BUT, that's not guaranteed to be true -- there's code a few lines down from here which explicitly checks whether aDomSel is equal to mDomSelectionWeak (and handles the case where they're not equal).  So I don't think you can assume aDomSel is the same selection we would've been using here.

BUT, maybe it's the correct one to use anyway (and this is more correct than before)?  I'm not sure exactly what this code is trying to do (first time I've looked at it), so I don't know for sure at this point...

Comment 6

[Daniel Holbert [:dholbert]]

Sorry, splinter decided to not show the "-" line for that change above. Here's the full patch context for the chunk I discussed in comment 5:
> 
> NS_IMETHODIMP
> nsCaret::NotifySelectionChanged(nsIDOMDocument *, nsISelection *aDomSel,
>                                 int16_t aReason)
> {
>-  if ((aReason & nsISelectionListener::MOUSEUP_REASON) || !IsVisible())//this wont do
>+  if ((aReason & nsISelectionListener::MOUSEUP_REASON) || !IsVisible(aDomSel))//this wont do
>     return NS_OK;

And here's where we have existing code that treats aDomSel as possibly-distinct-from mDomSelectionWeak (the value that IsVisible() uses currently):
http://searchfox.org/mozilla-central/source/layout/base/nsCaret.cpp#615-626

Comment 7

[Daniel Holbert [:dholbert]]

Oh, reading the comment in more detail -- I guess if the selection is different, we bail anyway.  So in that case it doesn't matter what IsVisible returns, I suppose.

Maybe worth including a comment alongside the "IsVisible(aDomSel)" call, to refer to the comment further down, and to make this clearer?  (And still worth spinning that out into its own patch distinct from the inlining patch.)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8897257 [details] [diff] [review]
Part 2: Inline nsCaret::IsVisible()

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

One more thing on part 2:

::: layout/base/nsCaret.h
@@ +83,5 @@
> +
> +      if (!mShowDuringSelection) {
> +        mozilla::dom::Selection* selection;
> +        if (aSelection) {
> +          selection = static_cast<mozilla::dom::Selection*>(aSelection);

This static_cast should be replaced with
  aSelection->AsSelection()
...I think.  At least, that's what e.g. GetSelectionInternal does for this same type-conversion.

(AsSelection() is just a static_cast under the hood, so it's all the same -- but it's nice to constrain that hackery to the scope of that single function & be consistent about how we do it.)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8897258 [details] [diff] [review]
Part 3: Avoid recomputing the selection in nsCaret::SchedulePaint() when the caller has the information available

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

r=me on this SchedulePaint patch wit the following:

::: layout/base/nsCaret.cpp
@@ +443,4 @@
>  {
> +  Selection* selection;
> +  if (aSelection) {
> +    selection = static_cast<Selection*>(aSelection);

As above, this should be aSelection->AsSelection().

Also: I believe we're now assuming that aSelection is what GetSelectionInternal() would've returned, correct?  Maybe we should include an assertion to that effect here, to document & validate that assumption?

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8897259 [details] [diff] [review]
Part 4 : Only call nsLayoutUtils::GetDisplayRootFrame() once per call to nsIFrame::SchedulePaint()

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

Commit message nit:

> By calling this function one level higher in callers and passing the return
> value to callees

Maybe add parens around "(in callers...callees)".

Otherwise it kinda sounds like you're saying "one level higher in callers" -- i.e. it sounds like you're saying we *already* do this in callers and now we'll do it one level higher in those callers (whatever that would mean :)).

Comment 11

[(no longer active)]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Oh, reading the comment in more detail -- I guess if the selection is
> different, we bail anyway.  So in that case it doesn't matter what IsVisible
> returns, I suppose.

Yes, that was the idea.  I can also move that check to earlier in the function if you prefer.  What do you think?

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #11)
> Yes, that was the idea.  I can also move that check to earlier in the
> function if you prefer.  What do you think?

Let's leave the order as-is, so that we don't have to unconditionally call do_QueryReferent (since I think that's expensive, per IRC discussion just now).

Comment 13

[(no longer active)]

Attachment: Part 2: Avoid recomputing the selection in Selection::IsVisible() when the caller has the information available

Comment 14

[(no longer active)]

Attachment: Part 3: Inline nsCaret::IsVisible()

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8897519 [details] [diff] [review]
Part 2: Avoid recomputing the selection in Selection::IsVisible() when the caller has the information available

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

r=me

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8897520 [details] [diff] [review]
Part 3: Inline nsCaret::IsVisible()

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

r=me  (Do remember to renumber the old "Part 3" and "Part 4" in their commit messages before landing, too.)

Comment 17

[(no longer active)]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> r=me  (Do remember to renumber the old "Part 3" and "Part 4" in their commit
> messages before landing, too.)

Yep, already fixed locally.  :-)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1173745d96b
Part 1: Inline Selection::IsCollapsed(); r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c97e53e53c05
Part 2: Avoid recomputing the selection in Selection::IsVisible() when the caller has the information available; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae748be16f80
Part 3: Inline nsCaret::IsVisible(); r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/97347426c409
Part 4: Avoid recomputing the selection in nsCaret::SchedulePaint() when the caller has the information available; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8923ffe49206
Part 5 : Only call nsLayoutUtils::GetDisplayRootFrame() once per call to nsIFrame::SchedulePaint(); r=dholbert

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1173745d96b
https://hg.mozilla.org/mozilla-central/rev/c97e53e53c05
https://hg.mozilla.org/mozilla-central/rev/ae748be16f80
https://hg.mozilla.org/mozilla-central/rev/97347426c409
https://hg.mozilla.org/mozilla-central/rev/8923ffe49206

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away 8/23) from comment #0)
> This function takes about 6% of the time under
> HTMLInputElement::SetValueInternal() in Speedometer.

Sounds like this should be qf:p1 then. Hooray for a speedometer win!