Systray: Move all icon resolution to dataengine
ClosedPublic

Authored by roberts on Oct 8 2016, 12:07 PM.

Details

Summary

Changes triggered by investigation into a long-running high CPU usage bug with system tray animations. The systray itself had icon name to icon resolution code, which was being triggered (twice) for every icon, every time any icon in the systray was updated. This code was spinning up a KIconLoader on each of these instances, and throwing it directly away. Each one triggered a large quantity of memory allocations and disk scans.

This patch moves the extra bit of "appName" logic from the native part of the system tray to the statusnotifieritem datasource, which already had a stored 'customIconLoader' to handle icon theme paths, and removes the special lookup from the sytemtray applet completely. It also prefers icons provided by the dataengine to doing another lookup (contentious?). This removes all the extra CPU usage outside of the QML scene graph and graphics drivers locally.

This is very much a looking for feedback item - there are things about the icon loading paths I almost certainly haven't appreciated yet, and perhaps preferring loading by icon name in the applet has a another purpose.

BUG: 356479

Test Plan

Have tested locally with kgpg and steam, the two apps I have that trigger the old code path. In neither case, however, did the appName logic produce a different result to the code with just the icon search path in statusnotifieritem.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Lint Skipped
Unit
Unit Tests Skipped
roberts updated this revision to Diff 7214.Oct 8 2016, 12:07 PM
roberts retitled this revision from to Systray: Move all icon resolution to dataengine.
roberts updated this object.
roberts edited the test plan for this revision. (Show Details)
roberts added reviewers: mart, Plasma.
roberts set the repository for this revision to R120 Plasma Workspace.
roberts added a project: Plasma.
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptOct 8 2016, 12:07 PM
roberts updated this revision to Diff 7215.Oct 8 2016, 12:12 PM

Absolutely +1 to the idea. been meaning to do this for a while.

However the diff seems broken, can you try reuploading.

roberts updated this revision to Diff 7230.Oct 9 2016, 1:30 PM

Recreated diff with arc.

davidedmundson accepted this revision.Oct 9 2016, 7:15 PM
davidedmundson added a reviewer: davidedmundson.

Thanks ever so much. Looks good.

Do you have commit access?

This revision is now accepted and ready to land.Oct 9 2016, 7:15 PM
mart accepted this revision.Oct 10 2016, 8:27 AM
mart edited edge metadata.
This revision was automatically updated to reflect the committed changes.

Should this be backported to the 5.8 LTS, if it proves to be stable?

mart added a comment.Oct 21 2016, 8:45 AM

Should this be backported to the 5.8 LTS, if it proves to be stable?

may make sense, a bit scared of that tough