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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7687
Build 7705: arc lint + arc unit
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.