D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REVISION SUMMARY Use a single QNetworkAccessManager instance for all our HTTP jobs, and also add a simple diskcache to that qnam. Further ensure there is only a single qnam for the entire application using kns' http jobs, across all threads (lock when accessing the qnam). Without this, we are liable to end up creating and destroying a great many qnam instances, which certainly is something to try and avoid. CCBUG: 379193 TEST PLAN Using Discover, which creates a great many of these jobs, we have much less network traffic this way. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 AFFECTED FILES src/core/jobs/httpworker.cpp To: leinir, whiting, apol Cc: #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
apol added inline comments. INLINE COMMENTS > httpworker.cpp:33 > + > +Q_GLOBAL_STATIC(QNetworkAccessManager, NetworkManager) > +Q_GLOBAL_STATIC(QMutex, NetworkManagerMutex) How about getting a class with these 3 attributes? Looks like we're juggling here... REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol Cc: #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir marked an inline comment as done. leinir added inline comments. INLINE COMMENTS > apol wrote in httpworker.cpp:33 > How about getting a class with these 3 attributes? Looks like we're juggling > here... You know, that sounds like a good plan. Updated diff incoming :) REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol Cc: #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir updated this revision to Diff 14079. leinir marked an inline comment as done. leinir added a comment. Simpler logic, with the global qnam (etc) stored in a locally defined class, so we only have the one global static, and also allowing the qnam access to be more pleasantly locked. REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5638?vs=13920&id=14079 REVISION DETAIL https://phabricator.kde.org/D5638 AFFECTED FILES src/core/jobs/httpworker.cpp src/core/jobs/httpworker.h To: leinir, whiting, apol Cc: #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
apol added inline comments. INLINE COMMENTS > httpworker.cpp:36 > +HTTPWorkerNAM() > +: nam(new QNetworkAccessManager) > +, mutex(new QMutex) These all are leaking. > httpworker.cpp:48 > +QNetworkAccessManager* nam; > +QMutex* mutex; > + Drop *, leaking mutexes REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol Cc: #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir updated this revision to Diff 14146. leinir marked 2 inline comments as done. leinir added a comment. Unpointerify the internals, as agreed REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5638?vs=14079&id=14146 REVISION DETAIL https://phabricator.kde.org/D5638 AFFECTED FILES src/core/jobs/httpworker.cpp src/core/jobs/httpworker.h To: leinir, whiting, apol Cc: #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > httpworker.cpp:41 > +QStorageInfo storageInfo(cacheLocation); > +cache.setMaximumCacheSize(storageInfo.bytesTotal() / 1000); > +nam.setCache(&cache); 0.1% of the partition size is a rather arbitrary value, no? It could go from something very tiny to something really big... On my 470GB partition this would lead to a 470MB cache for knewstuff, that's maybe a bit much, given the average size for knewstuff stuff? Maybe a qMin() call with a maximum value would be useful? > httpworker.cpp:47 > + > +QNetworkReply* get(const QNetworkRequest& request ) > +{ remove space before ')' > httpworker.cpp:144 > +// Check if the data was obtained from cache or not > +QString fromCache = d->reply->attribute( > QNetworkRequest::SourceIsFromCacheAttribute ).toBool() ? "(cached)" : "(NOT > cached)"; > + remove spaces inside ( ... ) > httpworker.cpp:151 > if (d->redirectUrl.scheme().startsWith("http")) { > -qCInfo(KNEWSTUFFCORE) << "Redirected to " << > d->redirectUrl.toDisplayString() << "..."; > -reply->deleteLater(); > -d->reply = d->qnam->get(QNetworkRequest(d->redirectUrl)); > +qCInfo(KNEWSTUFFCORE) << d->reply->url().toDisplayString() << > "was redirected to" << d->redirectUrl.toDisplayString() << fromCache << > d->reply->attribute( QNetworkRequest::HttpStatusCodeAttribute ).toInt(); > +d->reply->deleteLater(); remove spaces inside ( ... ) Also note that you added debugging values to a qCInfo message which is user-visible even in non debug builds. I think it might be better to revert this or split it out into a qCDebug call. > httpworker.cpp:164 > +else { > +qCInfo(KNEWSTUFFCORE) << "Data for" << > d->reply->url().toDisplayString() << "was fetched" << fromCache; > +} should probably be qCDebug() REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir marked 4 inline comments as done. leinir added inline comments. INLINE COMMENTS > dfaure wrote in httpworker.cpp:41 > 0.1% of the partition size is a rather arbitrary value, no? It could go from > something very tiny to something really big... > > On my 470GB partition this would lead to a 470MB cache for knewstuff, that's > maybe a bit much, given the average size for knewstuff stuff? Maybe a qMin() > call with a maximum value would be useful? Good point yes, it's supposed to be helpful, not take over the system. I'll set a max of 50 megs, should be fine for most uses. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir updated this revision to Diff 14235. leinir marked an inline comment as done. leinir added a comment. Some style fixes, and set a reasonable maximum size for the cache REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5638?vs=14146&id=14235 REVISION DETAIL https://phabricator.kde.org/D5638 AFFECTED FILES src/core/jobs/httpworker.cpp src/core/jobs/httpworker.h To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > httpworker.cpp:41 > +QStorageInfo storageInfo(cacheLocation); > +cache.setMaximumCacheSize(qMin(50, (int)(storageInfo.bytesTotal() / > 1000))); > +nam.setCache(&cache); 50 bytes? isn't that a bit small? :) > httpworker.cpp:57 > + > +Q_GLOBAL_STATIC(HTTPWorkerNAM, NetworkManager) > the uppercase first letter is weird, for a variable. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir marked 2 inline comments as done. leinir added inline comments. INLINE COMMENTS > dfaure wrote in httpworker.cpp:41 > 50 bytes? isn't that a bit small? :) Yes, yes it is ;) > dfaure wrote in httpworker.cpp:57 > the uppercase first letter is weird, for a variable. Good point, yes. Not very Qt REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir updated this revision to Diff 14240. leinir marked 2 inline comments as done. leinir added a comment. A bit of naming cleanup, and a very silly miscalculation because things count in bytes rather than some arbitrary random multiple of them REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5638?vs=14235&id=14240 REVISION DETAIL https://phabricator.kde.org/D5638 AFFECTED FILES src/core/jobs/httpworker.cpp src/core/jobs/httpworker.h To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir updated this revision to Diff 14242. leinir added a comment. static var naming fix, for consistency with elsewhere REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5638?vs=14240&id=14242 REVISION DETAIL https://phabricator.kde.org/D5638 AFFECTED FILES src/core/jobs/httpworker.cpp src/core/jobs/httpworker.h To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
dfaure added a comment. 5 is 50kB. You wrote 50 megs which would be 5000 or 50*1024*1024. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir added a comment. In https://phabricator.kde.org/D5638#107645, @dfaure wrote: > 5 is 50kB. > You wrote 50 megs which would be 5000 or 50*1024*1024. Yes, i certainly did. I should remember to drink less caffeine sometimes. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
leinir updated this revision to Diff 14244. leinir added a comment. Work some numbers a bit REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5638?vs=14242&id=14244 REVISION DETAIL https://phabricator.kde.org/D5638 AFFECTED FILES src/core/jobs/httpworker.cpp src/core/jobs/httpworker.h To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R304 KNewStuff REVISION DETAIL https://phabricator.kde.org/D5638 To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks
D5638: Use a single QNAM (and a disk cache) for HTTP jobs
This revision was automatically updated to reflect the committed changes. Closed by commit R304:b8d0bc8818ff: Use a single QNAM (and a disk cache) for HTTP jobs (authored by leinir). REPOSITORY R304 KNewStuff CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5638?vs=14244&id=14404 REVISION DETAIL https://phabricator.kde.org/D5638 AFFECTED FILES src/core/jobs/httpworker.cpp src/core/jobs/httpworker.h To: leinir, whiting, apol, dfaure Cc: dfaure, #frameworks