nsCaret::NotifySelectionChanged() is too expensive due to timer thread lock grabbing

RESOLVED FIXED in Firefox 56

Status

()

Core
Layout
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 months ago
The selection listener notifications that we call under the input value setter in speedometer end up running this code: <http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/layout/base/nsCaret.cpp#639>

This code always cancels and resets the caret timer, but both of those operations need to grab the timer thread lock, which can be very expensive, and this shows up in profiles of speedometer.

We actually don't need to do this every time.  Technically we only need to do this once per caret, or when the rate is set to 0 (to turn the timer off) or when it changes.  A patch that does that seems to buy 2 points overall on Speedometer on my fast machine, it may be even better on slower hardware.  I'll post it after a try run.
(Assignee)

Comment 1

6 months ago
Sorry, I mentioned the wrong function name before!  I spent a bit too much time in the caret code today.  :-)
Summary: nsCaret::IsMenuPopupHidingCaret() is too expensive due to timer thread lock grabbing → nsCaret::NotifySelectionChanged() is too expensive due to timer thread lock grabbing
(Assignee)

Comment 2

6 months ago
Created attachment 8877376 [details] [diff] [review]
Avoid needlessly grabbing the timer thread lock when selections change in order to reset the caret timer
Attachment #8877376 - Flags: review?(mats)
(Assignee)

Comment 3

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95dbac4d2f7bcec27d7944135124977c825293b

Comment 4

6 months ago
Comment on attachment 8877376 [details] [diff] [review]
Avoid needlessly grabbing the timer thread lock when selections change in order to reset the caret timer

It looks like you've changed the semantics of blinkRate == 0.
Previously we did not call InitWithNamedFuncCallback in that case,
now we do.

I think we can do the change you want a lot simpler, just add:

uint32_t blinkRate = ...;
if (mBlinkRate == blinkRate) {
  return;
}

here:
http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/layout/base/nsCaret.cpp#638
and add "mBlinkRate = blinkRate" at the end.
Keep everything else the same (apart from the moved blinkRate assignment).

I think this would be easier to follow than what you propose.
Attachment #8877376 - Flags: review?(mats) → review-
(Assignee)

Comment 5

6 months ago
Oops, you're right.  New patch coming up...
(Assignee)

Comment 6

6 months ago
Created attachment 8877716 [details] [diff] [review]
Avoid needlessly grabbing the timer thread lock when selections change in order to reset the caret timer
Attachment #8877716 - Flags: review?(mats)
(Assignee)

Updated

6 months ago
Attachment #8877376 - Attachment is obsolete: true

Comment 7

6 months ago
Comment on attachment 8877716 [details] [diff] [review]
Avoid needlessly grabbing the timer thread lock when selections change in order to reset the caret timer

I think the "mBlinkRate = blinkRate" should be after the if-statement.
Otherwise, we'll do unnecessary Cancel() calls here since
"mBlinkRate == blinkRate" is still false.

Also, we might miss to re-enable the timer if we go back to the same value:
500ms -> 0 -> 500ms
The first transition will Cancel(), but mBlinkRate is still 500.
The second transition will hit "mBlinkRate == blinkRate" and
thus not re-enable the timer.

r=mats with that
Attachment #8877716 - Flags: review?(mats) → review+

Comment 8

6 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db8a48dae370
Avoid needlessly grabbing the timer thread lock when selections change in order to reset the caret timer; r=mats
https://hg.mozilla.org/mozilla-central/rev/db8a48dae370
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

6 months ago
Depends on: 1373788
You need to log in before you can comment on or make changes to this bug.