Differential D10512

try to preload certain applets in a smart way
ClosedPublic
Actions

Authored by mart on Feb 14 2018, 3:48 PM.

Details

Reviewers
None
Group Reviewers
Plasma
Commits
R242:6cbea20bf9f7: try to preload certain applets in a smart way
Summary

preload popups of some applets after init in the background
based on a value of X-Plasma-PreloadWeight in the desktop file
if present, otherwise some default values based on the applet
type (Provides)

Save the weight in the config, if an applet is never opened,
slowly decrease the weight, when it reaches 0 don't preload it
next start, increase every time it gets opened, so at the moment
it's quite aggressive about preloading, in order to not do it
a lot of plasma startups without touching the applet are needed

Applet with a very big weigth will be preloaded immediately,
therefore having an impact on the time it will take to have
a panel visible and usable, while lesser weigths will preload
after a random number of seconds between 2 and 10, so will load
in the background after everything is started

Test Plan

Plasma starts up correctly, applets load correctly and can be added
correctly both those expanded or collapsed.
plasmashell appears correctly usable without too big hiccups even
while it's loading things in the background

some numbers:
without preloading, plasma takes around 64 mb of memory after startup
when preloading everything about 94, so it's a cost of about 30 mb
which is not negligible.
don't have precise timing, but if everything gets preloaded immediately,
the time to get an usable desktop appears to be at least doubled,
while the delayed preloading (except just a couple of applets) doesn't
seem to have a big impact on the time needed to get an usable desktop

Diff Detail

Repository
R242 Plasma Framework (Library)
Branch
mart/preload
Lint
No Linters Available
Unit
No Unit Test Coverage
mart created this revision.Feb 14 2018, 3:48 PM
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptFeb 14 2018, 3:48 PM
Restricted Application added subscribers: Frameworks, plasma-devel. · View Herald Transcript
mart requested review of this revision.Feb 14 2018, 3:48 PM
mart updated this revision to Diff 27163.
  • silence warnings
mart updated this revision to Diff 27169.Feb 14 2018, 4:13 PM
  • correct typo
apol added a subscriber: apol.Feb 14 2018, 6:04 PM

It sounds like it should be the shell who decides what needs preloading, rather than the framework guessing it.

src/plasmaquick/appletquickitem.cpp
87 ↗(On Diff #27162)

Isn't X-Plasma-PreloadWeight for that?

broulik added a subscriber: broulik.Feb 14 2018, 8:09 PM

Cool!

Since this is all applet quick item stuff it will just work™ with applet within system tray?
Would be nice to have some qCInfo in there somewhere so we can better understand what it's doing, when it's increasing/decreasing the rating, when it's preloading it, etc.

src/plasmaquick/appletquickitem.cpp
47 ↗(On Diff #27162)

imho using an enum rather than a bunch of static ints is nicer

49 ↗(On Diff #27162)

Are you sure we want to preload the calendar right away? This thing takes forever to open, wouldn't want to have that slow down plasma startup massively.

95 ↗(On Diff #27162)

Could this be split into multiple lines, it's quite hard to follow

636 ↗(On Diff #27162)

This should go into a function, there's like three places where a "random" writeEntry is scattered around

642 ↗(On Diff #27162)

Add this as context or else it would blow up when the applet is destroyed before the timer fires:

QTimer::singleShot(qrand..., this, [this]() {
mart added inline comments.Feb 14 2018, 8:10 PM
src/plasmaquick/appletquickitem.cpp
87 ↗(On Diff #27162)

It's to have some default weights without having to populate every desktop file

ngraham added a comment.EditedFeb 14 2018, 8:13 PM

Let's keep in mind the PR aspect of this: if we end up 30 MB heavier and a few seconds slower on a cold boot for a new install, advanced users who frequent internet forums and write reviews will notice this, but they probably won't notice that some widgets open more quickly once Plasma is loaded.

Perhaps we could invert the logic: instead of preloading everything and then over time removing infrequently used widgets from the list, we could preload only a few fixed things like the menu, Task Manager, and plasma-nm, and for everything else, preload based on frequency of use.

davidedmundson added a subscriber: davidedmundson.Feb 14 2018, 8:18 PM
davidedmundson added inline comments.
src/plasmaquick/appletquickitem.cpp
789

So if you ever open the any applet slightly more than every alternate login (at any point during that session) over time it'll become blocking preloaded on boot?

Your login to desktop will just get slower and slower until it's ultimately loading everything.

I'm against doing any hacks without prior profiling.

If we find plasma-pa (for example) gains a lot at no cost, great! Add the entry.

If we find plasma-pa loaded in a nanosecond anyway, and takes up 10Mb of RAM to do so, then it's a very silly thing to do.

I'd like to see some numbers for every applet we add it to, I don't think the shell is in a position to do this automatically.

broulik added inline comments.Feb 14 2018, 9:52 PM
src/plasmaquick/appletquickitem.cpp
791 ↗(On Diff #27162)

I was also wondering about dbus-activated applet, from what I can tell whenever I start a media player, the score is reduced, so I'll most likely end up with 0 for media controller immediately.

mart added a comment.EditedFeb 15 2018, 12:51 PM

sooo, benchmarks:
i tracked the time that occurs between mouse click and the return of setExpanded, which is reasonably near to the popup actually appearing (numbers consistent doing things more than once)
this is done on a fairly fast machine, would be interesting to do the same on a really slow one, like the pinebook

Kickoff: no preload, before incubator removal: 279 msecs
Kickoff: no preload, after incubator removal: 211 msec (edit with kickoff from master this becomes 162 msecs)
Kickoff: with preload: 15 msecs (the first time still takes some tme to build all the nodes and texture, second time 0 msecs)

digital clock (with akonadi integration which makes the graphics a bit more complex)
digital clock: no preload, before incubator removal: 739 msecs
digital clock, no preload , after incubator removal: 234 msecs
digital clock, preload: 46 msecs (graphically quite complex, takes a more to create all textures, second time is 0 as usual)

media player: no preload, before incubator removal: 148 msecs
media player, no preload , after incubator removal: 89 msec
media player, preload: 0 msecs (graphics simple enough for texture creation to not register?)

Clipboard, no preload , before incubator removal: 191 msec
Clipboard, no preload , after incubator removal: 79 msec
Clipboard, preload: 0 msec

kicker: no preload, after incubator removal: 79 msecs
kicker: preload: 9 msec

mart updated this revision to Diff 27263.EditedFeb 15 2018, 2:30 PM
  • less aggressive preload policy
  • weigth decreases a bit quicker than it grows, takes some clicks to go over the preload treshold
  • no immediate load, but the delay depends from the weight.
  • use an enum for the magic numbers.
davidedmundson added inline comments.Feb 19 2018, 4:38 PM
src/plasmaquick/appletquickitem.cpp
50 ↗(On Diff #27162)

So by default we preload /everything/ then over time your session gets faster as you don't use stuff?

mart updated this revision to Diff 27554.Feb 19 2018, 4:46 PM

control with an env variable

can be either always off, always on or adaptive

broulik added inline comments.Feb 19 2018, 4:57 PM
src/plasmaquick/appletquickitem.cpp
63 ↗(On Diff #27162)

Cache the result in a static to read it only once?

65 ↗(On Diff #27162)

case insensitive?

648 ↗(On Diff #27162)

Still missing this context:

QTimer::singleShot(delay, this, [this, delay]() {
mart updated this revision to Diff 27557.Feb 19 2018, 5:16 PM
  • cache env var reading
ngraham added a comment.Feb 19 2018, 5:19 PM

If by default this slows down plasmashell's loading in favor of making widgets load faster, I'm against it. People will notice the drawbacks of the former more than the benefits of the latter.

mart added a comment.Feb 19 2018, 5:25 PM
In D10512#209679, @ngraham wrote:

If by default this slows down plasmashell's loading in favor of making widgets load faster, I'm against it. People will notice the drawbacks of the former more than the benefits of the latter.

with the trick of the QTimer, it shows an usable desktop actually before things are preloaded, then starting doing its things in the background.
thesting on the local systems, plasma seems to stay fluid even while it's actually performing the preload in the background, so shouldn't be noticeable there.
of course, needs to be tried by many people with systems more or less performant

src/plasmaquick/appletquickitem.cpp
63 ↗(On Diff #27162)

in a static?

mart updated this revision to Diff 27558.Feb 19 2018, 5:28 PM
  • always delay preloading
mart updated this revision to Diff 27559.Feb 19 2018, 5:29 PM
  • just PLASMA_PRELOAD_POLICY
This revision was not accepted when it landed; it landed in state Needs Review.Feb 19 2018, 5:59 PM
Closed by commit R242:6cbea20bf9f7: try to preload certain applets in a smart way (authored by mart). · Explain Why
This revision was automatically updated to reflect the committed changes.
anthonyfieroni added a subscriber: anthonyfieroni.Feb 19 2018, 6:15 PM
anthonyfieroni added inline comments.
src/plasmaquick/appletquickitem.cpp
63–74 ↗(On Diff #27162)

So Adaptive can a default even environment variable is not setted?
Furthermore you can make a static function to access value

AppletQuickItemPrivate::PreloadPolicy AppletQuickItemPrivate::appletPolicy()
{
    static const PreloadPolicy preloadPolicy = []() -> PreloadPolicy {
        if (qEnvironmentVariableIsSet("PLASMA_PRELOAD_POLICY")) {
            const QString policy = QString::fromUtf8(qgetenv("PLASMA_PRELOAD_POLICY")).toLower();
            if (policy == QStringLiteral("aggressive")) {
                return Aggressive;
            } else if (policy == QStringLiteral("none")) {
                return None;
            }
        }
        return Adaptive;
    }();
    return preloadPolicy;
}
mart added inline comments.Feb 20 2018, 10:02 AM
src/plasmaquick/appletquickitem.cpp
63–74 ↗(On Diff #27162)

So Adaptive can a default even environment variable is not setted?

yes, adaptive is the default

Furthermore you can make a static function to access value

it's done anyways only once as preloadPolicy is already a static?

anthonyfieroni added inline comments.Feb 21 2018, 7:29 AM
src/plasmaquick/appletquickitem.cpp
63–74 ↗(On Diff #27162)

But if PLASMA_PRELOAD_POLICY is not setted s_preloadPolicy stays Uninitialized, no? But since you check against >= Adaptive it's still ok :)

Revision Contents
Changeset List

Diff 27163

View Options

src/plasma/data/servicetypes/plasma-applet.desktop

Loading...
View Options

src/plasmaquick/appletquickitem.cpp

Loading...
View Options

src/plasmaquick/private/appletquickitem_p.h

Loading...