Massive jank on agezao.github.io/confetti-js/ (with max confetti = 800)
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox98 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
It seems we spend a lot of time on the main thread blocked on a mutex for an image: https://share.firefox.dev/3ATkmSK
|
Assignee | |
Comment 2•3 years ago
|
||
FWIW since I it took a bit until I saw the mutex, it is here. I think the assumption in the dom code is that these checks are fast, which they aren't, clearly.
However, I wonder if rather than calling GetImage() and so on we could just check the image progress and check STATUS_IS_ANIMATED. That's what the image frame checks, and should avoid the locking (code).
Happy to submit a patch if that seems reasonable.
|
|
Assignee | |
Comment 3•3 years ago
|
||
|
||
Updated•3 years ago
|
|
||
Updated•3 years ago
|
|
|
Assignee | |
Comment 4•3 years ago
|
||
With that patch the progress tracker lock still shows up in profiles...
|
||
Comment 5•3 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
With that patch the progress tracker lock still shows up in profiles...
Updated profile?
|
|
Assignee | |
Comment 6•3 years ago
|
||
The profile looks pretty much identical. I was going to send an updated patch ;)
|
|
Assignee | |
Comment 7•3 years ago
|
||
Updated profile with the patches that are incoming: https://share.firefox.dev/3roaLQD
The locking is gone but the OnUnlockedDraw notification still takes a lot of the time, mostly pointer-chasing and reference counting. Andrew, we only have one listener for this image notification which is https://searchfox.org/mozilla-central/rev/8b46752d1e583b2f817c451f93ba515fb865554d/dom/base/nsImageLoadingContent.cpp#291.
Tim, perhaps we can get away without the OnUnlockedDraw machinery? Per bug 1170680 comment 3 you proposed keeping animated images working like that.
|
|
Assignee | |
Comment 8•3 years ago
|
||
Drive-by cleanup.
Depends on D137762
|
|
Assignee | |
Comment 9•3 years ago
|
||
Depends on D137764
|
|
Assignee | |
Comment 10•3 years ago
|
||
We're checking animation consumers, not locked status. We don't care
about animation consumer count of non-animated images.
Depends on D137765
|
|
Assignee | |
Comment 11•3 years ago
|
||
Ok, comment 10 gives us the perf benefit without changing behavior for now. Perhaps we should consider doing comment 7, but I'm not familiar enough with our image tracking code to know if that's a good trade-off to make.
https://share.firefox.dev/3IXSAqU
We could still remove a bunch of the overhead from the image cache and so on I suspect, but it looks smooth as is now, it doesn't jank.
|
|
Assignee | |
Comment 12•3 years ago
|
||
10% is imgLoader::NotifyObserversForCachedImage. That is only for devtools, seems wrong.
|
|
Assignee | |
Comment 13•3 years ago
|
||
|
||
Comment 14•3 years ago
|
||
|
||
Comment 15•3 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/4414ddb4e286
https://hg.mozilla.org/mozilla-central/rev/703530db5516
https://hg.mozilla.org/mozilla-central/rev/27ae4d4205d1
https://hg.mozilla.org/mozilla-central/rev/9b229b696db4
|
||
Comment 16•3 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
Tim, perhaps we can get away without the
OnUnlockedDrawmachinery? Per bug 1170680 comment 3 you proposed keeping animated images working like that.
I think we probably still want it for animated images. We'd need some other solution to make sure images start animating asap if we didn't do it.
Description
•