1735745 - 4.58 - 2.87% tabswitch / tabswitch + 1 more (Windows) regression on Mon June 21 2021

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Perfherder has detected a talos performance regression from push 9daae850d8829872fb915161ef065c584aacaff5. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
5%	tabswitch	windows10-64-shippable-qr	e10s stylo webrender-sw	7.93 -> 8.30
4%	tabswitch	windows10-64-shippable-qr	e10s stylo webrender-sw	7.94 -> 8.23
3%	tabswitch	windows10-64-shippable-qr	e10s stylo webrender	7.95 -> 8.18

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[Kagami :saschanaz]

So this is probably what Masayuki was afraid about. https://phabricator.services.mozilla.com/D101245?id=450629#inline-653168

The ideal situation would be that eFormSelect won't affect the window-wise selection change listener. I'll need to dig about that, but Masayuki, do you perhaps have an updated idea?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, you touched very hot paths. There are some ideas:

• Make EventListenerManager and nsPIDOMWindow have separated members to fire each event (This can save the cost of creating AsyncEventDispatcher for events which are never handled.
• Check whether there is an event listener in SelectionChangeEventDispatcher::OnSelectionChange() before comparing selection ranges (anyway nees to update the cache, but multiple selection ranges in editor is rare, so it may be faster than computing startOffset and endOffset for comparing)
• EventListenerManager::GetInnerWindowForTarget() is expensive due to this QI, cannot we skip this call if mMayHaveSelectionChangeEventListener is true? (I'm not sure for this. It may be required for adopting nodes)

Comment 3

[Release mgmt bot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1677253

Comment 4

[Kagami :saschanaz]

Thanks, NI'ing myself.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Trying to omit to QI in bug 1455514.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #5)

Trying to omit to QI in bug 1455514.

According to the trysever's result, this fix may not affect.

Comment 7

[Hsin-Yi Tsai [:hsinyi]]

Masayuki is writing a patch for this. Should I re-assign to you?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, my patch can fix this bug.
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=26d3174ce6aebfd881c4bfcafec017cdb98957e8&newProject=try&newRevision=1dd3afa27aa08ef742a0560302329065c3bbaf23&framework=1&page=1&showOnlyImportant=1

I'll post the patch soon.

Comment 9

[Kagami :saschanaz]

Thank you Masayuki!

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

you're welcome!

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1735745 - Make `SelectionChangeEventDispatcher` dispatch `select` event and `selectchange` event only when there may be corresponding event listeners r=smaug!

select event on text controls now dispatched immediately before
selectionchange. However, it needs to create AsyncEventDispatcher for
each. This cost may not be expensive, but they are called really a lot even
if there is no corresponding event listener.

Therefore, this patch makes nsPIDOMWindow and EventListenerManager have
MayHave*EventListeners flag separately for each, and makes
SelectionChangeEventDispatcher does not try to do create
AsyncEventDispatcher when there is no corresponding event listener.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f88dcab731cc
Make `SelectionChangeEventDispatcher` dispatch `select` event and `selectchange` event only when there may be corresponding event listeners r=smaug

Comment 13

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/f88dcab731cc