1927173 - Since the last few days, favicons on address bar take some time to appear

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Mayank Bansal]

See attached video. Wasnt some recent work done on favicons? Maybe its a fallout from that?
See attached video.

Comment 1

[Mayank Bansal]

Attachment: bandicam 2024-10-25 21-33-29-655.mp4

Comment 2

[Mayank Bansal]

Attachment: about:support

Comment 3

[Marco Bonardo [:mak]]

it's possible one of the queries hit a slow path and now it takes longer to resolve.
We should investigate this on a large database.
If you go to about:telemetry and you have telemetry enabled, there should be a slow SQL section, and you could check for queries continaining "moz_icons", what is reported there?

Comment 4

[Marco Bonardo [:mak]]

Maybe related to the changes in Bug 1664001, this check would indeed be slow WHERE p.url BETWEEN :pageRoot AND :pageRoot || "X'FFFF' " because there is no index on url... we should use origin or rev_host to restrict the result set

Comment 5

[Mayank Bansal]

Attachment: slow sql.png

Comment 6

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: performance problem on retrieving icons

Comment 7

[Marco Bonardo [:mak]]

yeah, the posted Slow SQL telemetry confirms my suspect.

Comment 8

[Mayank Bansal]

I am happy to give a profile using the profiler. What preset should I use? Or what threads I should add in the profiler UI to capture slow SQL or placesstuff in general?

Comment 9

[Marco Bonardo [:mak]]

(In reply to Mayank Bansal from comment #8)

I am happy to give a profile using the profiler. What preset should I use? Or what threads I should add in the profiler UI to capture slow SQL or placesstuff in general?

Thank you, I think we have all we need to fix this already.
I must file a bug to improve our profiler story around queries execution, currently you can check the "ignore selected elements and register all threads" option in about:profiling, and that will also show sqldb threads, but they don't report the executed query.... we can really do better there (fwiw it looks like the StepStatement call shows the query in the stack)

Comment 10

[Mayank Bansal]

Here is a profile with all threads + IPC + I/O on all threads : https://share.firefox.dev/4f1NdY4

Comment 11

[Yazan Al Macki [:yazan]]

Attachment: Bug 1927173 - Make FetchMostFrecentSubPageIcon SQL query faster by filtering by host. r?mak

Comment 12

[Yazan Al Macki [:yazan]]

Submitted a patch that should fix this, thank you for reporting the issue.

Comment 13

[Mayank Bansal]

This try build does not fix the issue for me : https://share.firefox.dev/3YGvnEj

Comment 14

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: performance problem on retrieving icons

Comment 15

[Mayank Bansal]

Builds from this try push are much better: https://share.firefox.dev/48qdrB8

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32f052833473
Make FetchMostFrecentSubPageIcon SQL query faster by filtering by host. r=mak,places-reviewers

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/32f052833473

Comment 18

[Yazan Al Macki [:yazan]]

Attachment: Bug 1927173 - Make FetchMostFrecentSubPageIcon SQL query faster by filtering by host. r?mak

Original Revision: https://phabricator.services.mozilla.com/D227078

Comment 19

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Firefox hangs when opening and clicking into a new blank tab, favicons take time to load
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: This patch fixes a regression, and the fix has been verified in Nightly
• String changes made/needed: N/A
• Is Android affected?: no

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/f460b453ee98

Comment 21

[Mayank Bansal]

This is fixed for me on the latest Nightly. Thanks!