Make nsCaret::NotifySelectionChanged() less expensive

RESOLVED FIXED in Firefox 57

Status

()

Core
Layout
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

11 months ago
Created attachment 8897256 [details] [diff] [review]
Part 1: Inline Selection::IsCollapsed()
Attachment #8897256 - Flags: review?(bugs)
(Assignee)

Comment 2

11 months ago
Created attachment 8897257 [details] [diff] [review]
Part 2: Inline nsCaret::IsVisible()
Attachment #8897257 - Flags: review?(dholbert)
(Assignee)

Comment 3

11 months ago
Created attachment 8897258 [details] [diff] [review]
Part 3: Avoid recomputing the selection in nsCaret::SchedulePaint() when the caller has the information available
Attachment #8897258 - Flags: review?(dholbert)
(Assignee)

Comment 4

11 months ago
Created attachment 8897259 [details] [diff] [review]
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().
Attachment #8897259 - Flags: review?(dholbert)
Attachment #8897256 - Flags: review?(bugs) → review+
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...
Attachment #8897257 - Flags: review?(dholbert) → review-
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
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 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 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?
Attachment #8897258 - Flags: review?(dholbert) → review+
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 :)).
Attachment #8897259 - Flags: review?(dholbert) → review+
(Assignee)

Comment 11

11 months ago
(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?
Flags: needinfo?(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).
Flags: needinfo?(dholbert)
(Assignee)

Comment 13

11 months ago
Created attachment 8897519 [details] [diff] [review]
Part 2: Avoid recomputing the selection in Selection::IsVisible() when the caller has the information available
Attachment #8897519 - Flags: review?(dholbert)
(Assignee)

Comment 14

11 months ago
Created attachment 8897520 [details] [diff] [review]
Part 3: Inline nsCaret::IsVisible()
Attachment #8897520 - Flags: review?(dholbert)
(Assignee)

Updated

11 months ago
Attachment #8897257 - Attachment is obsolete: true
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
Attachment #8897519 - Flags: review?(dholbert) → review+
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.)
Attachment #8897520 - Flags: review?(dholbert) → review+
(Assignee)

Comment 17

11 months ago
(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

11 months ago
Pushed by eakhgari@mozilla.com:
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
(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!
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.