Closed Bug 1735745 Opened 8 months ago Closed 6 months ago

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

Categories

(Core :: DOM: Selection, defect)

Firefox 91
defect

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- wontfix
firefox93 --- wontfix
firefox94 --- wontfix
firefox95 --- wontfix
firefox96 --- fixed

People

(Reporter: alexandrui, Assigned: masayuki)

References

(Regression)

Details

(4 keywords)

Attachments

(1 file)

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.

Flags: needinfo?(krosylight)

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?

Flags: needinfo?(krosylight) → needinfo?(masayuki)

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)
Flags: needinfo?(masayuki)

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

Thanks, NI'ing myself.

Assignee: nobody → krosylight
Flags: needinfo?(krosylight)

Trying to omit to QI in bug 1455514.

Depends on: 1455514

(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.

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

Flags: needinfo?(masayuki)
Assignee: krosylight → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Flags: needinfo?(krosylight)

Thank you Masayuki!

you're welcome!

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.

Pushed by masayuki@d-toybox.com:
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.