1753471 - imgLoader::NotifyObserversForCachedImage is slow

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

On pages that frequently hit the network cache like bug 1753453, we spend a lot of time on this function, see bug 1753453 comment 11.

This is only for devtools and is supposed to run at most once per page per image, but instead we run it all the time and filter it on devtools' side (bug 1747494).

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 2

[Sean Feng [:sefeng]]

Agreed, yeah. We fixed it on the devtools side, but it's a shame that I didn't mention we should fix it on the platform side...

I don't know if Emilio is going to submit a patch, but I can definitely make one. So I'll leave my NI for now.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 4

[Sean Feng [:sefeng]]

Attachment: Bug 1753471 - Only notify http-on-image-cache-response once per cache entry r=tnikkel

I've also tested the behavior in Chrome and I think this is
the correct way of doing it. Chrome also only notifies the
cached entry once, it won't notify it even if I manually
trigger it after pageload.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52b536f08640
Only notify http-on-image-cache-response once per cache entry r=tnikkel

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/52b536f08640

Comment 7

[Timothy Nikkel (:tnikkel)]

Do you think this would be a good candidate to uplift?

Comment 8

[Sean Feng [:sefeng]]

Yeah, I noticed the profile in bug 1754908 has a lot of http-on-image-cache-response, so this patch has some potentials, I agree we should uplift.

Comment 9

[Sean Feng [:sefeng]]

Comment on attachment 9263141 [details]
Bug 1753471 - Only notify http-on-image-cache-response once per cache entry r=tnikkel

Beta/Release Uplift Approval Request

• User impact if declined: Users may experience degraded performance when they visit certain sites (If the sites reused a lot of cached images)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is not risky because the changes were only about preventing an notification from firing and the notification is only being used for devtools.
• String changes made/needed:

Comment 10

[Pascal Chevrel:pascalc]

Comment on attachment 9263141 [details]
Bug 1753471 - Only notify http-on-image-cache-response once per cache entry r=tnikkel

Safe fix in early betas for a perf regression that may also fix a webcompat P1 bug (bug 1754908), approved for 98 beta 5, thanks.

Comment 11

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c47e4081058