Consider not dispatching the selectionchange event for pages that don't have such a listener registered

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
5 days ago

People

(Reporter: Ehsan, Assigned: smaug)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This seems to show up inside EventListenerManager::HandleEventInternal() while we are trying to find an event listener for selectionchange that doesn't exist over and over again.  I think it should be fairly simple to avoid all of this work when no listener for this event has ever been registered.
(Assignee)

Comment 1

2 years ago
In e10s case we could easily store data in TabChildGlobal about events which there aren't listeners for.
Though, there is always the question of default handling too, since those aren't listeners, but just PostHandleEvent implementations.

Perhaps we could have a white list of events which could be skipped, and PostHandleEvent wouldn't ever be called for them.

(We do optimize out mutation events normally, since our chrome shouldn't be using those.)
(Reporter)

Comment 2

2 years ago
I'm taking a slightly different approach for this bug, since I think it can be done in a better way in this case, by moving SelectionChangeListener to be activated on demand at the nsISelectionController level when the selectionchange listener appears.
(Assignee)

Comment 3

2 years ago
ok, I'm trying separately figure out if comment 1 would help with other events too.
(Reporter)

Comment 5

2 years ago
Some explanation about the approach used here.  Right now three types of nodes can have selectionchange listeners, documents, inputs and textareas (the latter two are a feature that we haven't yet shipped).  These correspond to our selection controllers internally.  This patch makes each node tell its selection controller when its listeners are added or removed.  We manage the counter for the listeners centrally in nsFrameSelection, and create the SelectionChangeListener object dynamically when the first listener gets registered and remove it when the last one goes away.  We also take care of initializing SelectionChangeListener::mOldRanges according to the current selection, which is needed for the case where the first selectionchange listener is added with an already existing selection present.
(Assignee)

Comment 6

2 years ago
Comment on attachment 8879745 [details] [diff] [review]
Only register SelectionChangeListeners when there are event listeners for selectionchange events

This obviously can't work. There can be event listeners on any node (capture phase at least) or in Window added using addEventListener. And what if there is <input> element created, then selectionchange listener added, and then the element added to document?
Attachment #8879745 - Flags: review?(bugs) → review-
(Assignee)

Comment 7

2 years ago
Or what if a node is adopted/imported to a new document?
(Reporter)

Comment 8

2 years ago
Yeah, all true.  I can't think of how to do anything better, but this is *really* worth spending time on.

FWIW I measured the simulated impact of what fixing this would give us on the reference hardware by setting the dom.select_events.enabled pref to false and I could measure between 4 to 7 points improvements in the total benchmark points as a result!
Assignee: ehsan → nobody
(Reporter)

Updated

2 years ago
Attachment #8879745 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Did you rerun the test on reference hardware couple of time with and without the patch?
(Assignee)

Comment 10

2 years ago
Just want to ensure that isn't noise.

I think we could try something simpler. Add a flag to nsPIDOMWindow whether there might be selectionchange listeners, and if not, return early from SelectionChangeListener::NotifySelectionChanged.
That does cause us to create SelectionChangeListener still, but maybe that isn't too bad.
Flags: needinfo?(ehsan)
(Assignee)

Comment 11

2 years ago
...since locally on my linux machine dom.select_events.enabled=false gives zero improvement. Nothing.
Hmm, could there be something linux or windows specific in selection handling.
(Reporter)

Comment 12

2 years ago
(In reply to Olli Pettay [:smaug] from comment #9)
> Did you rerun the test on reference hardware couple of time with and without
> the patch?

I ran with and without the pref many times, yes.  But please feel free to try to replicate my results.  (Speedometer numbers have been really noisy for me in the last 2-3 weeks, I get variations of a few points across different runs, which makes it really hard to judge the impact of changes like this with full certainty.)
Flags: needinfo?(ehsan)
(Reporter)

Comment 13

2 years ago
(In reply to Olli Pettay [:smaug] from comment #11)
> ...since locally on my linux machine dom.select_events.enabled=false gives
> zero improvement. Nothing.
> Hmm, could there be something linux or windows specific in selection
> handling.

Yes, that is the exact same result that I also get when I test locally on my local Linux machine.  It could also be related to the machine being faster, it's hard to say for sure.  But I do see a difference when I test with the pref on and off on the Acer machine versus on my development Linux machine, so it's either a platform difference or caused by the speed difference in the machine...

I guess I can also try to run the try builds that I have from my patch here on the Acer machine and report some results back.
(Assignee)

Comment 14

2 years ago
I think we could just do this. This seems to optimize out several thousands of selectionchange events during Speedometer

The naming "MayHave" and Listener vs Listeners follows the exiting convention
(which doesn't make much sense)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=469191e611984dbd0988a9da780085dbd38dac69
Assignee: nobody → bugs
Attachment #8880179 - Flags: review?(michael)

Comment 15

2 years ago
Comment on attachment 8880179 [details] [diff] [review]
selectionchange_opt.diff

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

r=me if you just move the if (doc) statement down, I'd like to see it again if we take a different approach to fixing the event ordering issue.

::: dom/base/SelectionChangeListener.cpp
@@ +85,5 @@
>        !nsFrameSelection::sSelectionEventsEnabled) {
>      return NS_OK;
>    }
>  
> +  if (doc) {

I think we should do this check after the check if ranges have actually changed. For example, suppose we receive the following Notifies without actually changing the underlying ranges:

Notify | Add Event Listener | Notify

We would fire the event listener with this patch, if we moved the check until after we would not. I would rather not expose the fact we're doing this optimization to webpages.

The other option here would be to read & update the current set of ranges when the first selection change listener is added to the document. I'm not sure how much more difficult that would be to do.
Attachment #8880179 - Flags: review?(michael) → review+
(Assignee)

Comment 16

2 years ago
Do you not mean "after the check if ranges have actually changed", but after 
adding stuff to mOldRanges?

Comment 18

2 years ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d270f2a7bd
don't dispatch the selectionchange event for pages that don't have such a listener registered, r=mystor

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/15d270f2a7bd
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.