Closed Bug 1503809 Opened 6 years ago Closed 1 day ago

FaviconHelper::ObtainCachedIconFile does main thread IO and causes hangs

Categories

(Core :: Widget: Win32, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
129 Branch
Performance Impact medium
Tracking Status
firefox129 --- fixed

People

(Reporter: johannh, Assigned: mconley, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: perf:responsiveness, Whiteboard: [bhr:ObtainCachedIconFile])

Attachments

(3 files)

FaviconHelper::ObtainCachedIconFile has been showing up in BHR for causing hangs due to its main thread IO:

https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/widget/windows/WinUtils.cpp#1528

Now, the file.exists call in there is probably completely unnecessary, but I'm not sure if it's possible to get rid of the GetLastModified time.
Whiteboard: [fxperf] → [fxperf:p3]
Priority: -- → P3

This is a specific subset of the main thread I/O done by jumplists (bug 1529276).

Blocks: 1529276
Whiteboard: [fxperf:p3] → [fxperf:p3][bhr:ObtainCachedIconFile]
Severity: normal → S3
Performance Impact: --- → medium
Whiteboard: [fxperf:p3][bhr:ObtainCachedIconFile] → [bhr:ObtainCachedIconFile]
No longer blocks: 1529276
Depends on: 1529276
See Also: → 1867344

I think what we can do here is that we can introduce a new method, in FaviconHelper - FaviconHelper::ObtainCachedIconFileAsync. We can then transition the new Jump List backend and this usage here (which already ignores the synchronous return value) to that new method.

The new method can then do the existence testing / stat'ing on the IO thread rather than the main thread, and call into a runnable with either the path to the cached favicon or an nsresult reporting what has gone wrong.

And then we can update nsIJumpListBuilder.obtainAndCacheFavicon to return a Promise instead and use our new backend.

Assignee: nobody → mconley
Attachment #9389682 - Attachment description: WIP: Bug 1503809 - [WIP] Add an async version of FaviconHelper::ObtainCachedIconFile → Bug 1503809 - Add an async version of FaviconHelper::ObtainCachedIconFile. r?rkraesig
Blocks: 1895760
Attachment #9399653 - Attachment description: Bug 1503809 - Add tests for nsIJumpListBuilder.obtainAndCacheFaviconAsync. → WIP: Bug 1503809 - Add tests for nsIJumpListBuilder.obtainAndCacheFaviconAsync.
Attachment #9399653 - Attachment description: WIP: Bug 1503809 - Add tests for nsIJumpListBuilder.obtainAndCacheFaviconAsync. → Bug 1503809 - Add tests for nsIJumpListBuilder.obtainAndCacheFaviconAsync. r?Mossop
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78b1a5e7ec1e
Add an async version of FaviconHelper::ObtainCachedIconFile. r=rkraesig,win-reviewers
https://hg.mozilla.org/integration/autoland/rev/ce0ff7d06ed4
Update WindowsJumpList.sys.mjs to use the async favicon caching utility. r=rkraesig
https://hg.mozilla.org/integration/autoland/rev/4d04db1983e3
Add tests for nsIJumpListBuilder.obtainAndCacheFaviconAsync. r=mossop
https://hg.mozilla.org/integration/autoland/rev/22bf63387696
apply code formatting via Lando

Backed out for causing xpcshell failures on test_JumpListBuilder_obtainAndCacheFaviconAsync.js

Backout link

Push with failures

Failure log

Flags: needinfo?(mconley)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/947f25cfac26
Add an async version of FaviconHelper::ObtainCachedIconFile. r=rkraesig,win-reviewers
https://hg.mozilla.org/integration/autoland/rev/38b6ce46cca2
Update WindowsJumpList.sys.mjs to use the async favicon caching utility. r=rkraesig
https://hg.mozilla.org/integration/autoland/rev/e9c1549806bf
Add tests for nsIJumpListBuilder.obtainAndCacheFaviconAsync. r=mossop
https://hg.mozilla.org/integration/autoland/rev/c1a66ab56f9d
apply code formatting via Lando
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: