Use a single QNAM (and a disk cache) for HTTP jobs
ClosedPublic

Authored by leinir on Apr 28 2017, 1:20 PM.

Details

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.

Diff Detail

Repository
R304 KNewStuff
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
leinir created this revision.Apr 28 2017, 1:20 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 28 2017, 1:20 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
apol added inline comments.Apr 28 2017, 2:26 PM
src/core/jobs/httpworker.cpp
33

How about getting a class with these 3 attributes? Looks like we're juggling here...

leinir marked an inline comment as done.May 2 2017, 1:18 PM
leinir added inline comments.
src/core/jobs/httpworker.cpp
33

You know, that sounds like a good plan. Updated diff incoming :)

leinir updated this revision to Diff 14079.May 2 2017, 1:20 PM
leinir marked an inline comment as done.

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.

apol added inline comments.May 4 2017, 10:29 PM
src/core/jobs/httpworker.cpp
36

These all are leaking.

48

Drop *, leaking mutexes

leinir updated this revision to Diff 14146.May 5 2017, 8:44 AM
leinir marked 2 inline comments as done.

Unpointerify the internals, as agreed

dfaure requested changes to this revision.May 6 2017, 9:17 AM
dfaure added a subscriber: dfaure.
dfaure added inline comments.
src/core/jobs/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?

47

remove space before ')'

144

remove spaces inside ( ... )

151

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.

164

should probably be qCDebug()

This revision now requires changes to proceed.May 6 2017, 9:17 AM
leinir marked 4 inline comments as done.May 7 2017, 4:11 PM
leinir added inline comments.
src/core/jobs/httpworker.cpp
41

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.

leinir updated this revision to Diff 14235.May 7 2017, 4:13 PM
leinir edited edge metadata.
leinir marked an inline comment as done.

Some style fixes, and set a reasonable maximum size for the cache

dfaure requested changes to this revision.May 7 2017, 5:05 PM
dfaure added inline comments.
src/core/jobs/httpworker.cpp
41

50 bytes? isn't that a bit small? :)

57

the uppercase first letter is weird, for a variable.

This revision now requires changes to proceed.May 7 2017, 5:05 PM
leinir marked 2 inline comments as done.May 7 2017, 5:10 PM
leinir added inline comments.
src/core/jobs/httpworker.cpp
41

Yes, yes it is ;)

57

Good point, yes. Not very Qt

leinir updated this revision to Diff 14240.May 7 2017, 5:12 PM
leinir edited edge metadata.
leinir marked 2 inline comments as done.

A bit of naming cleanup, and a very silly miscalculation because things count in bytes rather than some arbitrary random multiple of them

leinir updated this revision to Diff 14242.May 7 2017, 5:16 PM

static var naming fix, for consistency with elsewhere

dfaure added a comment.May 7 2017, 5:26 PM

50000 is 50kB.
You wrote 50 megs which would be 50000000 or 50*1024*1024.

leinir added a comment.May 7 2017, 5:28 PM

50000 is 50kB.
You wrote 50 megs which would be 50000000 or 50*1024*1024.

Yes, i certainly did. I should remember to drink less caffeine sometimes.

leinir updated this revision to Diff 14244.May 7 2017, 5:30 PM

Work some numbers a bit

dfaure accepted this revision.May 7 2017, 8:36 PM
This revision is now accepted and ready to land.May 7 2017, 8:36 PM
This revision was automatically updated to reflect the committed changes.