D18149: Share Plasma::Theme instances between multiple Svg and ColorScope
broulik added a comment. > And QSharedDataPointer? For that `Theme` would need to be `QSharedData`, no? REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D18149 To: broulik, #plasma Cc: apol, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D18149: Share Plasma::Theme instances between multiple Svg and ColorScope
apol added inline comments. INLINE COMMENTS > broulik wrote in colorscope.h:134 > Complained with "QSharedPointer: cannot create a QSharedPointer from a > QObject-tracking QWeakPointer" Seems odd. And QSharedDataPointer? http://doc.qt.io/qt-5/qshareddatapointer.html or std::shared_ptr. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D18149 To: broulik, #plasma Cc: apol, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D18149: Share Plasma::Theme instances between multiple Svg and ColorScope
broulik added inline comments. INLINE COMMENTS > apol wrote in colorscope.h:134 > How about using a QSharedPointer? Complained with "QSharedPointer: cannot create a QSharedPointer from a QObject-tracking QWeakPointer" REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D18149 To: broulik, #plasma Cc: apol, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D18149: Share Plasma::Theme instances between multiple Svg and ColorScope
apol added inline comments. INLINE COMMENTS > colorscope.h:134 > + > +static Plasma::Theme *s_theme; > +static int s_themeRefCount; How about using a QSharedPointer? REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D18149 To: broulik, #plasma Cc: apol, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D18149: Share Plasma::Theme instances between multiple Svg and ColorScope
broulik added a comment. > Now in multi-thread environment will have problems especially on actualTheme(), no? `ColorScope` is QML-only and should be fine? Not sure about `Svg`, it never mentioned being thread-safe. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D18149 To: broulik, #plasma Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D18149: Share Plasma::Theme instances between multiple Svg and ColorScope
anthonyfieroni added a comment. Now in multi-thread environment will have problems especially on actualTheme(), no? REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D18149 To: broulik, #plasma Cc: anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns
D18149: Share Plasma::Theme instances between multiple Svg and ColorScope
broulik edited the test plan for this revision. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D18149 To: broulik, #plasma Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D18149: Share Plasma::Theme instances between multiple Svg and ColorScope
broulik created this revision. broulik added a reviewer: Plasma. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. broulik requested review of this revision. REVISION SUMMARY Especially since the `Theme` isn't modified in any way but just used to read some data. While the `Private` part of `Theme` is already shared, creating a `Theme` instance still has some setup cost. TEST PLAN Noticed in GammaRay there was a tonne of `Theme` objects. Just the usage in `Svg` alone accounted for 5ms startup time, and `ColorScope` is also widely used. After this patch there's merely 25 `Theme` objects being created. Wanted to use `QSharedPointer` for this but failed.. Doesn't reduce refcount and cleanup when you assign a different theme using `Svg::setTheme` but this is hardly used anyway. REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D18149 AFFECTED FILES src/declarativeimports/core/colorscope.cpp src/declarativeimports/core/colorscope.h src/plasma/private/svg_p.h src/plasma/svg.cpp To: broulik, #plasma Cc: kde-frameworks-devel, michaelh, ngraham, bruns
