1843477 - A lot of time is spent in AddRef / Release during button.click() / checkbox.click(), especially when clearing the event target chain

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Markus Stange [:mstange]]

The testcases in bug 1834003 spend a fair amount of time calling AddRef and Release on cycle collected objects. https://share.firefox.dev/3DaKDxI

The biggest offender is nsTArray<EventTargetChainItem>::ClearAndRetainStorage. This function is called four times in total per call to .click(), probably with identical event target chains: For click, DOMActivate, input and change. It spends all its time calling virtual Release methods which spend their time in nsCycleCollectingAutoRefCnt::decr.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Yes. We need to keep the event targets alive, so the chain must have strong references. And yes, in these particular test cases the chain might be the same but in general it isn't. The spec requires us to have separate event path (== chain) for each event.

But we should make AddRef/Release non-virtual in this case. Non-trivial change, since it requires moving refcnt to EventTarget, and some EventTargets use mRefCnt.incr/decr<NS_CycleCollectorSuspectUsingNursery>, and some mRefCt.incr/decr<NS_CycleCollectorSuspect3>.
The nursery is available on the main thread only currently, because accessing it was tried to be as fast as possible.

Another thing we could do in these nested events case is that there could be a flag (there should be space for one flag in nsWrapperCache's flags) whether the target is currently part of an event chain, and if so, we can use raw pointers in the nested chains. Not sure if that would help much though if we make AddRef/Release non-virtual.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

The preliminary results look quite good
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=2cfd6144f271283f6a6964bcf50efcd7770a3d11&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=17fc117158fff8225831b449e60c3308c5c69951&page=1&showOnlyConfident=1
Svelte test for example spends lots of time in event dispatch

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1843477, remove main thread only CC macros, r=mccr8

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1843477, make CycleCollector use nursery purple buffer always on the main thread, r=mccr8

Depends on D187146

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1843477, non-virtual AddRef/Release for EventTarget and common subclasses, r=mccr8

Depends on D187147

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f45f40beaf7
remove main thread only CC macros, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/e7bc0b63286a
add a nursery purple buffer for each cycle collector, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/4dd5d8ac600c
non-virtual AddRef/Release for EventTarget, r=mccr8

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Hopefully this doesn't regress anything too badly. Should help especially event dispatch. The tls access (which happens once per refcnt-changed-object between forgetSkippable/CC) is annoying. I tested keeping the old main thread only nursery and use a bit from mRefCnt to tell which Suspect() to use, but that did regress perf quite badly.

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a18a71ffe10b
Backed out 3 changesets for causing hazards failures on TelemetryFixture.cpp. CLOSED TREE

Comment 9

[Cosmin Sabou [:CosminS]]

Backed out for causing hazards failures on TelemetryFixture.cpp

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Crunning%2Cpending%2Crunnable&fromchange=32879fad16bcd6ce0c4bf1fcbb464968ffd0ec70&searchStr=linux%2Cx64%2Cdebug%2Chazard-linux64-haz%2Fdebug%2Ch&tochange=bde2db6f12ce1100b2bbb4b10916f7be1cf90f3a&selectedTaskRun=WTB9rAHCTmaRPwf4lJOcAg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=428449725&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/a18a71ffe10b102e0f00f92b1a8d442fabcf0583

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1843477, mark some classes being main thread only, r=mccr8

Depends on D187148

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1843477, make nsCOMPtr use RefPtrTraits, r=nika

This could be done in various ways, the taken approach is just trying to avoid too many changes

Depends on D188958

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1843477, tweak AutoJSContextWithGlobal to avoid rooting hazard, r=mccr8

Depends on D188959

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48942aff586f
remove main thread only CC macros, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/6a0e19a60a5d
make CycleCollector use nursery purple buffer always on the main thread, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/1de814e5820d
non-virtual AddRef/Release for EventTarget and common subclasses, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/05c882dd0d9a
mark some classes being main thread only, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/bdcd84c938d1
make nsCOMPtr use RefPtrTraits, r=nika
https://hg.mozilla.org/integration/autoland/rev/64b093aa12a9
tweak AutoJSContextWithGlobal to avoid rooting hazard, r=mccr8

Comment 14

[Cosmin Sabou [:CosminS]]

Backed out for causing hazards failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel&revision=64b093aa12a927ed284928d5dd4f7c6f08368b42&searchStr=hazard&selectedTaskRun=UkQd0lu3STKZLD88Yv_sVA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=430299593&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/39dc607f61623c82e70f4982272959e0625d52ae

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1843477, avoid rooting hazards from AddRef, r=peterv

Depends on D188960

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1855201 to tweak the rooting hazard annotations.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10edcb4b807f
remove main thread only CC macros, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/5d9a36ae5068
make CycleCollector use nursery purple buffer always on the main thread, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/a122b7314f12
non-virtual AddRef/Release for EventTarget and common subclasses, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/7ad8d4a2d153
mark some classes being main thread only, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/3ed02194e51d
make nsCOMPtr use RefPtrTraits, r=nika
https://hg.mozilla.org/integration/autoland/rev/4ceeaf14d6cf
tweak AutoJSContextWithGlobal to avoid rooting hazard, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/90fecbcb1a40
avoid rooting hazards from AddRef, r=peterv

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/10edcb4b807f
https://hg.mozilla.org/mozilla-central/rev/5d9a36ae5068
https://hg.mozilla.org/mozilla-central/rev/a122b7314f12
https://hg.mozilla.org/mozilla-central/rev/7ad8d4a2d153
https://hg.mozilla.org/mozilla-central/rev/3ed02194e51d
https://hg.mozilla.org/mozilla-central/rev/4ceeaf14d6cf
https://hg.mozilla.org/mozilla-central/rev/90fecbcb1a40

Comment 19

[Alex Finder]

(In reply to Cosmin Sabou [:CosminS] from comment #14)

Backed out for causing hazards failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel&revision=64b093aa12a927ed284928d5dd4f7c6f08368b42&searchStr=hazard&selectedTaskRun=UkQd0lu3STKZLD88Yv_sVA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=430299593&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/39dc607f61623c82e70f4982272959e0625d52ae

== Change summary for alert #39683 (as of Thu, 28 Sep 2023 04:58:14 GMT) ==

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
5%	perf_reftest_singletons parent-basic-singleton.html	linux1804-64-shippable-qr	e10s fission stylo webrender	99.88 -> 105.25

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
6%	perf_reftest_singletons window-named-property-get.html	linux1804-64-shippable-qr	e10s fission stylo webrender	652.60 -> 612.93

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=39683