Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement caching framework #258

Merged
merged 1 commit into from May 20, 2019
Merged

Conversation

dagwieers
Copy link
Collaborator

@dagwieers dagwieers commented May 20, 2019

This PR includes:

  • Caching and fallback framework for listings (e.g. A-Z, or categories)
  • New troubleshooting option to invalidate caches

This fixes #241

@dagwieers dagwieers added the enhancement New feature or request label May 20, 2019
@dagwieers dagwieers added this to the v1.10.0 milestone May 20, 2019
@dagwieers dagwieers force-pushed the unit-tests-rewrite branch 5 times, most recently from 12cbd85 to 329b115 Compare May 20, 2019 04:34
@dagwieers
Copy link
Collaborator Author

I would also like to add Recent/Offline caching as well. But the filtered content (My Recent/My Offline) needs special invalidation when things are followed/unfollowed.

Please test. I flagged it experimental for now.

@mediaminister
Copy link
Collaborator

Does this speed up menu browsing? Didn't notice a difference with the master branch.

@dagwieers
Copy link
Collaborator Author

It depends.

On Android I don't see a lot of improvements, except if I disable this caching some lookups are from time to time slower. (Likely network interruptions)

On Raspberry Pi the difference is bigger. What caching brings you is a more predictable load-time, but it definitely depends on the speed of the SD card, or whether it is still buffered in file system buffers. I noticed that even when things are cached, the JSON processing is the biggest culprit now.

And maybe in some cases getting it from the network may be faster than reading it from SD card. In this case you'd better replaced that SD card (I have had a LibreELEC that was quite slow due to the SD card wearing out).

Also, we have some use-cases where the same file is being loaded multiple times (e.g. tvguide if you first look what's on Eén, and then on Canvas it would be downloaded twice, or live tv, where it would get the EPG information 3 times when building the menu)

I will probably make a distinction between optional caching and mandatory caching. E.g. for categories if you would disable caching it will also not fall back to the categories cache, so for categories and the tvguide I would probably keep a separate option (or enable it always).

@dagwieers
Copy link
Collaborator Author

dagwieers commented May 20, 2019

@mediaminister Beware that for testing you always have to quit the addon, start another addon, and then go back to the addon. This is needed to ensure the new code has been loaded, since we enable uselanguageinvoker that's no longer a guarantee.

@dagwieers
Copy link
Collaborator Author

Just to indicate that the menu speeds on Raspberry Pi is bound to the JSON/menu processing, enable caching and go to the Categories menu. Then open and re-open the Kinderen-category (slow, 54 items) and the Koken-category (instant, 2 items).

We do caching for categories and channel listings as well. I wasn't planning to, but doing that change was pretty easy anyway and the results are even more visible (as the network is a larger part of the total load-time).

That is the reason why you don't notice e.g. the A-Z listing being a big speed up (something I expected). The cheer number of menu items slows everything down on Raspberry Pi.

(Slow here is quite relative, it's still close to a second here !)

@dagwieers
Copy link
Collaborator Author

Recent and Offline is not being cached now, which I intend to do too.

@dagwieers
Copy link
Collaborator Author

BTW I also intend to add a setting to disable fanart/thumbnails, as this may also play a part in slowing down menu listings (indirectly). Plus it helps me understand disk-access and network-access better (now this activity makes things less transparent).

@dagwieers
Copy link
Collaborator Author

For offline and recent caching the differences (on Raspberry Pi) are also quite visible.
I only need to add a means for invalidating the filtered (My favorites) lists whenever programs are followed/unfollowed. But that's not that difficult now.

@dagwieers dagwieers force-pushed the unit-tests-rewrite branch 6 times, most recently from 6f44da0 to c5f7e1f Compare May 20, 2019 17:01
@dagwieers dagwieers force-pushed the unit-tests-rewrite branch 2 times, most recently from 25ef683 to dc76083 Compare May 20, 2019 18:55
Copy link
Collaborator

@mediaminister mediaminister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I tested and it didn't crash.

Copy link
Collaborator

@pietje666 pietje666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good i like that it is something enabled by a setting, btw why don't you use a mocking framework instead of writing your own mocks?

test/xbmcaddon.py Outdated Show resolved Hide resolved
@dagwieers
Copy link
Collaborator Author

Why don't you use a mocking framework instead of writing your own mocks?

Well, if you look at what we did before, it was a kludge. And when I tried to improve it using Mock things were a lot harder then just writing proper modules for it. Also, mocking a library wasn't that easy and my aim was to import kodiwrapper as-is.

The xbmc libraries are not feature-complete, which is fine. If we ever add functionality we either need to add a stub, or add an alternative implementation.

PS This also opens up the possibility to run addon.py directly to achieve a higher coverage.

@dagwieers dagwieers force-pushed the unit-tests-rewrite branch 2 times, most recently from 707d4eb to 28bbb0e Compare May 20, 2019 23:02
@dagwieers
Copy link
Collaborator Author

dagwieers commented May 20, 2019

So currently this is disabled by default, but I think we ought to enable this. If we ship this disabled, all our existing users will have it disabled. So if we enable it by default later, they will keep on having it disabled. What do you guys think ?

The TTL is 7 days for categories.json (with a fallback forever). The TTL for other files is 1 hour. And only for favorites.json we also use it as fallback forever. (This in case the favorites system would stop working, it would continue working locally, although updates would not be pushed :-/)

This PR includes:

- Caching and fallback framework for listings (e.g. A-Z, or categories)
- Now troubleshooting option to invalidate caches
@dagwieers
Copy link
Collaborator Author

Feedback welcome. In the meantime I'll merge it.

@dagwieers dagwieers merged commit 2e751ea into add-ons:master May 20, 2019
@mediaminister
Copy link
Collaborator

I agree that we enable this by default, but before a release this feature must be thoroughly tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add caching for A-Z listing and categories
3 participants