A lot of time is spent in AddRef / Release during button.click() / checkbox.click(), especially when clearing the event target chain
Categories
(Core :: DOM: Events, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox120 | --- | fixed |
People
(Reporter: mstange, Assigned: smaug)
References
(Blocks 2 open bugs)
Details
(Keywords: perf-alert, Whiteboard: [sp3])
Attachments
(7 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
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
.
Updated•1 year ago
|
Assignee | ||
Comment 1•1 year ago
|
||
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.
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 2•1 year ago
|
||
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
Assignee | ||
Comment 3•1 year ago
|
||
Assignee | ||
Comment 4•1 year ago
|
||
Depends on D187146
Assignee | ||
Comment 5•1 year ago
|
||
Depends on D187147
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Pushed by opettay@mozilla.com: 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
Assignee | ||
Comment 7•1 year ago
|
||
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.
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a18a71ffe10b Backed out 3 changesets for causing hazards failures on TelemetryFixture.cpp. CLOSED TREE
Comment 9•1 year ago
|
||
Backed out for causing hazards failures on TelemetryFixture.cpp
Failure log: https://treeherder.mozilla.org/logviewer?job_id=428449725&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/a18a71ffe10b102e0f00f92b1a8d442fabcf0583
Assignee | ||
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Assignee | ||
Comment 10•1 year ago
|
||
Depends on D187148
Assignee | ||
Comment 11•1 year ago
|
||
This could be done in various ways, the taken approach is just trying to avoid too many changes
Depends on D188958
Assignee | ||
Comment 12•1 year ago
|
||
Depends on D188959
Updated•1 year ago
|
Comment 13•1 year ago
|
||
Pushed by opettay@mozilla.com: 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•1 year ago
|
||
Backed out for causing hazards failures.
Failure log: https://treeherder.mozilla.org/logviewer?job_id=430299593&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/39dc607f61623c82e70f4982272959e0625d52ae
Assignee | ||
Comment 15•1 year ago
|
||
Depends on D188960
Assignee | ||
Comment 16•1 year ago
|
||
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1855201 to tweak the rooting hazard annotations.
Comment 17•1 year ago
|
||
Pushed by opettay@mozilla.com: 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•1 year ago
|
||
bugherder |
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•1 year ago
|
||
(In reply to Cosmin Sabou [:CosminS] from comment #14)
Backed out for causing hazards failures.
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
Updated•1 year ago
|
Description
•