1812034 - 13 elements are restyled every time the download progress is updated

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Florian Quèze [:florian]]

I noticed this in this profile: https://share.firefox.dev/3kAvjo6 from bug 1709175 comment 104.

I can reproduce locally: https://share.firefox.dev/3R0upNU

And making the JS code set the download progress on the specific element that has the progress display reduces the restyle to only one element: https://share.firefox.dev/3HszRpx

Comment 1

[Florian Quèze [:florian]]

Attachment: Bug 1812034 - set the download progress on the #downloads-indicator-progress-inner element directly instead of on the entire downloads button to limit restyled elements, r=Gijs

Comment 2

[Florian Quèze [:florian]]

Emilio, do you think this kind of change (see the attached patch) is worth doing?

Also, the thing I was really trying to understand is why we repaint so many images (https://share.firefox.dev/3iUqeqp) every time the download progress is updated, and my patch reduced the number of elements styled shown on the Styles marker, but didn't affect the number of Image Paint markers. Do you know why we repaint them every time and if there is a way to avoid that?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Florian Quèze [:florian] from comment #2)

Emilio, do you think this kind of change (see the attached patch) is worth doing?

Sure, if it's not a lot of work, the less elements restyled the better.

Also, the thing I was really trying to understand is why we repaint so many images (https://share.firefox.dev/3iUqeqp) every time the download progress is updated, and my patch reduced the number of elements styled shown on the Styles marker, but didn't affect the number of Image Paint markers. Do you know why we repaint them every time and if there is a way to avoid that?

Those are these markers right? I don't think all those markers really represent a repaint looking at where they are, at a glance. A person with more knowledge about imagelib like aosmond/tnikkel might be able to provide more context.

Comment 4

[Florian Quèze [:florian]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Those are these markers right? I don't think all those markers really represent a repaint looking at where they are, at a glance. A person with more knowledge about imagelib like aosmond/tnikkel might be able to provide more context.

There are 6 places in the code where we insert Image Paint markers. Are they correct? https://searchfox.org/mozilla-central/search?q=AutoProfilerImagePaintMarker+PROFILER_RAII&path=&case=false&regexp=false

Comment 5

[Florian Quèze [:florian]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Those are these markers right? I don't think all those markers really represent a repaint looking at where they are, at a glance.

Here's another profile where I made these Image Paint markers capture a stack: https://share.firefox.dev/3XA4EGF
Can the stack tell you if it's a real paint?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I don't think we're rasterizing the image on those markers, we most likely hit the surface cache here.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

So depends on what you call "paint" basically

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d22309f09ce5
set the download progress on the #downloads-indicator-progress-inner element directly instead of on the entire downloads button to limit restyled elements, r=Gijs,desktop-theme-reviewers

Comment 9

[Timothy Nikkel (:tnikkel)]

Yes, what Emilio said is correct. For raster images once we decode them we keep them in the surface cache and just pass that surface around. For vector images when we rasterize them we usually put them in the surface cache. If we were doing too much decoding or rasterizing that was unnecessary that would be something to look into but hitting those image paint markers does not indicate either of those things is happening.

(In reply to Florian Quèze [:florian] from comment #4)

There are 6 places in the code where we insert Image Paint markers. Are they correct? https://searchfox.org/mozilla-central/search?q=AutoProfilerImagePaintMarker+PROFILER_RAII&path=&case=false&regexp=false

I think so.

Comment 10

[Atila Butkovits]

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