D5639: Internal cache for provider data on initialisation

This revision was automatically updated to reflect the committed changes. Closed by commit R304:6207a87b71d7: Internal cache for provider data on initialisation (authored by leinir). REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5639?vs=14243=14403

D5639: Internal cache for provider data on initialisation

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5639 To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks

D5639: Internal cache for provider data on initialisation

leinir updated this revision to Diff 14243. leinir added a comment. Static var naming change, for consistency and whatnot REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5639?vs=14236=14243 REVISION DETAIL https://phabricator.kde.org/D5639 AFFECTED

D5639: Internal cache for provider data on initialisation

leinir added inline comments. INLINE COMMENTS > dfaure wrote in engine.cpp:58 > The uppercase first letter on variable name is unusual. > > I personally use a s_ prefix for static vars. Hmm. Right, there is no established way of naming them in KNS already, so while i am not personally so keen

D5639: Internal cache for provider data on initialisation

dfaure added a comment. Looks simpler indeed. INLINE COMMENTS > engine.cpp:58 > +typedef QHash EngineProviderLoaderHash; > +Q_GLOBAL_STATIC(QThreadStorage, > EngineProviderLoaders) > + The uppercase first letter on variable name is unusual. I personally use a s_

D5639: Internal cache for provider data on initialisation

leinir updated this revision to Diff 14236. leinir marked an inline comment as done. leinir added a comment. Simplify the xmlloader cache logic a touch REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5639?vs=14080=14236 REVISION DETAIL

D5639: Internal cache for provider data on initialisation

leinir marked 3 inline comments as done. leinir added inline comments. INLINE COMMENTS > dfaure wrote in engine.cpp:206 > This connect (and the following) could be done outside of the if/else, so > avoid being repeated, no? > Or is there a risk that load() will emit those signals immediately?

D5639: Internal cache for provider data on initialisation

dfaure added inline comments. INLINE COMMENTS > engine.cpp:196 > +XmlLoader *loader = > EngineProviderLoaders()->localData().value(m_providerFileUrl); > +if(loader) { > +qCDebug(KNEWSTUFFCORE) << "Duplicate xml loader for this url, so > use that" <<

D5639: Internal cache for provider data on initialisation

leinir updated this revision to Diff 14080. leinir edited the summary of this revision. leinir added a comment. Simplify the logic, and only do the xmlloader caching, not the documents themselves which (as apol points out) should be cached already anyway. Not only that, but that codepath was

D5639: Internal cache for provider data on initialisation

leinir added a comment. In https://phabricator.kde.org/D5639#105620, @apol wrote: > Won't caching already fix the problem of traffic there? This change adds quite some complexity (all these QMutex make me cringe, these methods should always be called from the same thread anyway...)

D5639: Internal cache for provider data on initialisation

apol added a comment. Won't caching already fix the problem of traffic there? This change adds quite some complexity (all these QMutex make me cringe, these methods should always be called from the same thread anyway...) REPOSITORY R304 KNewStuff REVISION DETAIL

D5639: Internal cache for provider data on initialisation

leinir created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY This is a two layer approach to creating less network traffic when initialising a KNSCore::Engine: - Firstly, cache the xml documents