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
