Differential D5639

Internal cache for provider data on initialisation
ClosedPublic
Actions

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

Details

Reviewers
whiting
apol
dfaure
Commits
R304:6207a87b71d7: Internal cache for provider data on initialisation
Summary

This is is a simple approach to creating less network traffic when initialising multiple instances of KNSCore::Engine in the same application. Very simply, we share XML fetch jobs when many are created at the same time for the same URL.

The result of this patch in a single fetch per provider url per application launch, and in connection with D5638 we will fetch the data from the network only once, until the cache is invalidated, all in all resulting in much less traffic and less hammering of the servers.

CCBUG: 379193

Test Plan

Using Discover, this results in much less network traffic during startup and engine initialisations.

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:26 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 28 2017, 1:26 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
apol edited edge metadata.Apr 28 2017, 3:15 PM

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...)

leinir added a comment.May 2 2017, 8:53 AM
In 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...)

That was my initial impression as well, but it turns out that because the requests go out at the same time, they miss each other (i am not quite sure, but i think the redirection chain might have something to do with that)... The first step does seem to not be needed, though, as with the rechecking on the xml loader which i added after the document cache, the document cache doesn't seem to ever be hit.

The mutexes are just to safeguard the QHash access, which isn't thread safe... However, i think I could likely swap that for a thread-bound global static, since while the document cache would be completely global, the xml loader one doesn't really need to be... if i take out the document cache, i can get rid of those mutexes which certainly do annoy me as well. I will rework that a bit and see what happens.

leinir updated this revision to Diff 14080.May 2 2017, 1:24 PM
leinir edited the summary of this revision. (Show Details)

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 never triggered when i rechecked it after implementing the xmlloader cache, and so, simpler logic is a good thing. Adjusted summary to match.

dfaure added a subscriber: dfaure.May 6 2017, 9:31 AM
dfaure added inline comments.
src/core/engine.cpp
196

space after if

201

join with previous line

(Qt5/KF5 coding style)

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?

leinir marked 3 inline comments as done.May 7 2017, 4:15 PM
leinir added inline comments.
src/core/engine.cpp
206

Normally yes, it would potentially return immediately... However, it makes sense for the get job to be postponed just slightly (it's logically an async thing anyway), so that is what HTTPJob now does, and we can simplify this code muchly.

leinir updated this revision to Diff 14236.May 7 2017, 4:16 PM
leinir marked an inline comment as done.

Simplify the xmlloader cache logic a touch

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

Looks simpler indeed.

src/core/engine.cpp
58

The uppercase first letter on variable name is unusual.

I personally use a s_ prefix for static vars.

leinir added inline comments.May 7 2017, 5:17 PM
src/core/engine.cpp
58

Hmm. Right, there is no established way of naming them in KNS already, so while i am not personally so keen on underscores in my variable names, static vars are already sort of ugly, and so going with the "ugly things should look a bit ugly" that stoustrup suggests for c++ things, i kind of like that :)

leinir updated this revision to Diff 14243.May 7 2017, 5:18 PM

Static var naming change, for consistency and whatnot

dfaure accepted this revision.May 7 2017, 5:27 PM
This revision is now accepted and ready to land.May 7 2017, 5:27 PM
Closed by commit R304:6207a87b71d7: Internal cache for provider data on initialisation (authored by leinir). · Explain WhyMay 11 2017, 1:13 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/core/engine.cpp (16 lines)
Msrc/core/jobs/httpjob.cpp (4 lines)

Diff 14403

View Options

src/core/engine.cpp

Loading...
View Options

src/core/jobs/httpjob.cpp

Loading...