Cache the default KColorScheme configuration
ClosedPublic

Authored by mwolff on Jan 30 2019, 8:44 AM.

Details

Summary

KDevelop, Kate and probably other applications too, recreate
KColorScheme instances repeatedly. This was very costly since we
ended up reparsing the internal color scheme configuration file
every time - the shared configuration wasn't stored anywhere thus
it's refcount dropped to zero after once the KColorScheme was
fully constructed.

Optimize this apparently common scenario by caching the configuration
in a thread_local variable and only open a new configuration when the
user changed the application color scheme.

Diff Detail

Repository
R265 KConfigWidgets
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mwolff created this revision.Jan 30 2019, 8:44 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 30 2019, 8:44 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mwolff requested review of this revision.Jan 30 2019, 8:44 AM

I see that it's faster when I profile kate/kdev, but I cannot easily write a benchmark for this. It's only noticeable when the KDE_COLOR_SCHEME_PATH variable is set, otherwise the global application config will be used afte rall, which is going to be shared most probably. Any idea how I could construct a valid KDE_COLOR_SCHEME_PATH path to e.g. the breeze color scheme from within a kcolor scheme auto test?

broulik added a subscriber: broulik.Feb 1 2019, 8:05 AM

I also noticed that on application startup two default color schemes are created: One by plasma-integration set on QApplication and the other by QStyle::standardPalette() in the widget style.
I couldn't figure out a way to avoid this (have QStyle use the default app palette set by p-i or something without dirty hacks like using dynamic properties to communicate that we already have created the proper palette elsewhere). Perhaps this is something you could also profile/investigate :)

mwolff added a comment.Feb 5 2019, 7:29 AM

sure, but first let's get this in. @broulik or @dfaure care to give your +1?

broulik accepted this revision.Feb 5 2019, 8:28 AM
This revision is now accepted and ready to land.Feb 5 2019, 8:28 AM
mwolff added a comment.Feb 8 2019, 6:53 PM

pushed this now with a proper benchmark too, shows a ~10x performance win when a non-empty PATH is set

This revision was automatically updated to reflect the committed changes.