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=14404
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
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=14244
REVISION DETAIL
https://phabricator.kde.org/D5638
AFFECTED FILES
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
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
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
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() /
>
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=14235
REVISION
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
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);
> +
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=14146
REVISION DETAIL
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
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.
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
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
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
16 matches
Mail list logo