Bug 429454 - The reading of the icons may block the GUI
Summary: The reading of the icons may block the GUI
Status: CLOSED INTENTIONAL
Alias: None
Product: frameworks-kiconthemes
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: unspecified All
: NOR normal
Target Milestone: ---
Assignee: Christoph Feck
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-21 14:54 UTC by 21Naown
Modified: 2021-02-01 19:00 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description 21Naown 2020-11-21 14:54:29 UTC
For example in Kate:
Click to “File” may block the GUI.
Comment 1 Nate Graham 2020-11-23 18:25:19 UTC
Can you add a bit more information? What makes you think it's reading icons? And what exactly do you mean by "reading icons"?

Can you explain your process of investigation that led you to this conclusion?
Comment 2 Christoph Feck 2020-11-23 18:50:21 UTC
Since icons are created by name, they are loaded by the icon engine the moment they are needed (i.e. rendered). Loading all icons in advance will give us bug reports of slow application startup, so they are loaded as needed.
Comment 3 21Naown 2020-11-24 16:52:13 UTC
(In reply to Nate Graham from comment #1)
> Can you add a bit more information? What makes you think it's reading icons?
> And what exactly do you mean by "reading icons"?
> 
> Can you explain your process of investigation that led you to this
> conclusion?
Here is an example:
strace -e file kate &
echo 3 > /proc/sys/vm/drop_caches
hdparm -y /dev/sda

Then click to “File” in Kate and “strace” mentions the access to icons.


(In reply to Christoph Feck from comment #2)
> Since icons are created by name, they are loaded by the icon engine the
> moment they are needed (i.e. rendered). Loading all icons in advance will
> give us bug reports of slow application startup, so they are loaded as
> needed.
The size of “/usr/share/icons/breeze” is ~15MiB. But ~10000 files is problematic for a busy HDD, so a solution could be to have one file containing the icons. Then to load them in advance.
Comment 4 Christoph Feck 2020-11-24 23:43:32 UTC
Icons are already loaded from a single icon cache file in ~/.cache
Comment 5 21Naown 2020-11-25 01:38:02 UTC
“strace” mentions “~/.cache/icon-cache.kcache” at the start of Kate, but “/usr/share/icons” at the time of a click to “File”.
Comment 6 21Naown 2020-12-03 03:11:14 UTC
(In reply to Christoph Feck from comment #2)
> Loading all icons in advance will give us bug reports of slow application
> startup, so they are loaded as needed.
Here is a simulation with an idle HDD:
echo 3 > /proc/sys/vm/drop_caches
time cp --no-clobber --recursive /usr/share/icons/breeze/ ~/

“time” displays ~4 seconds, so there is no problem.
Comment 7 21Naown 2020-12-15 02:59:29 UTC
More precisely with a 7 mm HDD:
~1 second through SATA3
~4 seconds through USB2
Comment 8 21Naown 2020-12-18 02:34:52 UTC
Another example is to add the output of the following command into “kate-master/kate/main.cpp”:
find /usr/share/icons/breeze/ -type f | grep --fixed-strings 'svg' | sed --regexp-extended 's/.+\///g' | sed --regexp-extended 's/^/    QIcon::fromTheme\(QStringLiteral\("/g' | sed --regexp-extended 's/\.svg$/"\)\)\.isNull\(\)\;/g' | sed --regexp-extended '1 i \    QElapsedTimer timer\;\n    timer\.start\(\)\;' | sed --regexp-extended '$ a \    qDebug\(\) \<\< timer\.elapsed\(\)\;'

Build, execute “echo 3 > /proc/sys/vm/drop_caches” then “kate-master/build/kate/kate”. “qDebug()” displays less than 1 second. The results seems even better.

No user with a HDD will ever report a single bug about slow application startup for nearly 1 more second at the start of Plasma (which is an example of when to load the icons because all icons are loaded, not only those of Kate).
Comment 9 21Naown 2020-12-18 17:28:37 UTC
Kate is an example among others:
Dolphin when scrolling.
Kwin with Alt+Tab.
Plasma when moving the cursor over the icons in the system tray.

Also, during updates, each of these examples may block the GUI for more than 1 second.
Comment 10 Christoph Feck 2020-12-18 22:33:04 UTC
> No user with a HDD will ever report a single bug about slow application startup for nearly 1 more second at the start of Plasma (which is an example of when to load the icons because all icons are loaded, not only those of Kate).

Here is your misunderstanding. You cannot load all icons in advance for all applications in a separate process. Each process needs it's own QPixmap instance, and each process needs to load them from the icon cache either on each application start, or at the moment they are actually needed.

We opted for the latter, because loading all icons even those that are not needed yet increase both the memory pressure, as well delay the application startup.
Comment 11 21Naown 2020-12-19 02:38:09 UTC
Thank you for your message.

I verified the memory:
Breeze’s icons are loaded in less than 1 second for ~1 MiB more memory.

For example with Kate:
Since Kate does not load all Breeze’s icons, Kate loads the necessary icons in less than 0,5 second for ~0,1 MiB more memory.

Yes this increases the memory pressure and the application startup time but both are so imperceptible.
Comment 12 21Naown 2020-12-31 00:21:30 UTC
I verified the pixmaps with KSysGuard’s “Detailed Memory Information”:
– No pixmap allocates memory before a click to “File” whether the icons are preloaded at the start of Kate (for example “const QIcon icon = QIcon::fromTheme(QStringLiteral("kate"));”) or not.
– After a click to “File”, the pixmaps allocate ~2 MiB whether the icons are preloaded or not.
Comment 13 21Naown 2021-01-15 03:39:53 UTC
Is there a problem with the suggested solution?
Comment 14 21Naown 2021-02-01 04:34:53 UTC
The suggested solution is to read the icons from the HDD at the start of the software. Here is an example for an icon with Kate in “kate/main.cpp”:

QIcon icon = QIcon::fromTheme(QStringLiteral("new-document"));
// without “isNull()”, the icon is not read
icon.isNull();

The icon is read from the HDD at the start of the software. “strace -e file build/kate/kate” does not mention “new-document” at the time of a click to “File”, so the icon cannot block the GUI.
Read all icons requires less than 0,5 second and ~0,1 MiB more memory without any pixmap.


I set the status to “REPORTED” because there is at least one imperceptible solution.
Comment 15 Nate Graham 2021-02-01 16:18:05 UTC
Good investigation. However this isn't a feasible thing for us to add to every QIcon::fromTheme() function call in every app. I would recommend doing this in Qt itself. You can file a Qt bug at https://bugreports.qt.io/. Thanks!
Comment 16 Christoph Feck 2021-02-01 18:54:42 UTC
It's not an upstream issue; our KIconEngine just stores the icon name instead of already loading the pixmap. As indicated above, this is intended.