Persist caches to disk to avoid slow initialization upon restart #60
Conversation
|
@tehkillerbee I have added I also think that it may be worth refactoring of the caches used by the extension, what do you think? Right now there is |
blacklight
changed the title
blacklight
changed the title
|
@blacklight Great to see you've made progress! I agree, refactoring the caches used in Mopidy-Tidal is a good idea. I noticed the same thing back when I started maintaining this project but did not find the time to do it. (If it works, right?). I'm looking forward to test it later today. We also need to make sure that the default directory is suitable as a cache directory. I'd look at Mopidy documentation for pointers on where these data should be stored: link |
|
Other caches are here: |
If an item is unlikely to change (like a track, album or artist associated to a Tidal ID) then there's usually no need to remove it from the filesystem cache when it's removed from the memory cache because of `max_items` limit.
Good point. I'll prepare a commit to change the cache dir to |
- Retrieve `cache_dir` from a static configuration-based context object instead of adding an additional level of indirection through the backend instance. - Use mopidy's defaults for `cache_dir` - no need to explicitly set it among the supported configuration options.
The extension previously used three different caches:
- A simple in-memory dictionary for playlists
- A more sophisticated local `Cache` for tracks
- `LruCache` (extended by `SearchCache`) for everything else
There were also many overlaps between the tracks `Cache` and `LruCache`.
This commit rationalizes the caches and implements a single `LruCache`
for all the items, with support for filesystem persistence.
Items cached on the filesystem are indexed as it follows:
```
<cache_dir>
-> <type> # e.g. track or playlist
-> <prefix> # first two characters of the ID
-> <id>.cache
```
Where `<id>.cache` is an object representation serialized by `pickle`.
|
The persisted cache on storage has been implemented and the caches in the extension have been refactored. We now have a single The LRU cache keeps a limited amount of items in memory (default: 1024) and all the other cached items are persisted on an indexed cache on the filesystem. The structure of the fs cache is inspired by that of In case of a miss on the memory cache, the filesystem cache will be inspected, and in case of a miss on the fs as well the extension would perform a lookup over API. Some design decisions:
Still pending:
Testing volunteers are welcome! p.s. @kingosticks @adamcik I guess that several extensions that rely on slow remote backends have this cache persistence problem. The issue has been tackled quite well on |
|
@blacklight Great work, looking forward to test it! |
|
I don't disagree that this type of cache case is probably shared by multiple plugins, but I'm not sure if adding this to mopidy itself really makes sense. In a similar vein I've been wanting to factor out a bunch of the webservice/oauth type helpers so that we can just reuse things more. But I suspect that it might be better to just have someone create a If there are parts of this that can only be solved in mopidy, and there is real value to having it solved in that codebase then definitely open a bug with why this is the case. Otherwise I think this would benefit more from not being in "core", similar to how we've moved e.g. |
|
@adamcik I totally agree - if the plan is to keep the core "lightweight" then non-core features should be moved out of the core, not in. There's still a valid point though for features that are non-core but still shared by many extensions (such as caching, OAuth flows and playlist updates). Caching and OAuth in particular are implemented independently by multiple extensions (Spotify, SoundCloud, Tidal, YouTube etc.) even though they mostly share the same code - and, in some cases, most of the code of these features is copy-pasted from other extensions. I like the idea of something like a |
|
The PR is ready for test/review. The latest changes involve a more consistent cache implementation/usage in I have also fixed the logic for playlist cache invalidation upon updates. We still have a problem though because the current stable branch of More attributes are apparently exposed on the |
|
Browsing appears very fast indeed! Only issue I see is the images are continuously requested again from the server. I guess this is intentional, as we do not have a local cache of the downloaded images - only the URIs themselves. Perhaps we should consider a local image cache as well? Edit: This is mainly an issue when requesting the full resolution images. See below comments for details on how to reduce the resolution. |
Not completely true. It is thrown whenever Mopidy receive a |
- Added debug trace for items with no images found
@kingosticks this is now addressed in 6416ed3 |
It's not very relevant, and it may crash the function because not all the objects have a `type` attribute
These should both be fixed by b92064c
This should be fixed by 2adfb83
This should be fixed by 063fd0b
This is actually expected, it's just the error logging trace that is misleading. It happens because the extension builds URIs such as
I have tried to look up the same album (both through ncmpcpp and Iris) and I couldn't reproduce the error. I can't reproduce it when rendering My Albums either. Maybe it's a cache issue? If you have tried several versions of this PR it's likely that the cache folder may be screwed up :) if wiping the cache doesn't work can you try and replicate it when printing |
Thank you. I'm going to update to latest commit and I will report any test. |
|
I have updated to the latest version of better-cache. These are the findings: This is after a refresh (for every playlist):
I don't understand this other error: |
The second error is either an album that isn't found in a market, or a maximum requests exceeded. The first one should be fixed. Have you installed the latest version? If it still occurs, you may want to wipe the cache (older cache files may store playlists as list of tracks instead of objects). |
|
I'm not saying we need this during development but what do you think about a mechanism to automatically clear the cache when the extension version changes? |
Yes, wiping the cache the error is gone. But wiping the cache is anyway a major PITA because I have to refresh EACH playlist, one by one. Sending a |
I don't expect changes in the cached formats to occur that often. While developing the feature and figuring things out the format can obviously change, but now that we've settled for
Manually wiping the cache should no longer be required - we've settled for storing playlists as native mopidy objects instead of lists of tracks, so there shouldn't be any more breaking changes. When Depending on the client implementation, however, a mopidy restart may be required in case of updates. Or at least that's the case for ncmpcpp, because the current playlist |
Got it, thanks. So i have to install your python-tidal 0.6.x. Don't you think it should be set as a dependency on the final mopidy-tidal? |
I still hope that I won't need to do that. My PR is a temporary workaround while waiting (for the past two years) for the maintainer of The maintainer recently promised that 0.7.x should be coming up soon, and that's why I'm holding on that PR: it may soon become irrelevant, and since 0.7.x may also require the rewrite of some parts of However, if this PR gets approved before the new |
I fully understand and agree with you. A friend of mine is used to say: "We are on this Earth to suffer". (maybe too much Calvinistic!) |
These methods can be used to wipe cache entries both from memory and disk
Playlist metadata is always fetched upstream upon `PlaylistsProvider.as_list()` calls - the performance impact of a single API call isn't very high. This allows us to detect added/removed playlists at runtime and refresh the cache accordingly, without requiring mopidy to be restarted.
|
|
||
| cache_updates[cache_name][uri] = cache_data | ||
|
|
||
| tracks += data if hasattr(data, '__iter__') else [data] |
There was a problem hiding this comment.
We should change L251 to the following:
if item_type == 'playlist' and not cache_miss:
tracks += data.tracks
else:
tracks += data if hasattr(data, '__iter__') else [data]Otherwise, we will still get the error:
ERROR [Core-7] mopidy.core.library TidalBackend backend returned bad data: Expected a list of Track, not [Playlist
|
Yes. That's what I meant: caching the url, not the image data.
- Sent from my mobile phone. Sorry for typos -
Il Dom 29 Mag 2022, 19:04 Johannes L ***@***.***> ha scritto:
… IMHO there is no sense to cache the image data, if ever the images url...
@fmarzocca <https://github.com/fmarzocca> It appears to be faster to use
the cached image URL instead of requesting it anew. So it will provide a
better user experience and images will be displayed faster.
—
Reply to this email directly, view it on GitHub
<#60 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACBCQ3XHBKBAQG3ZG5Z2HDLVMOPRNANCNFSM5WTJERFA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Note: this is still work in progress, but it already considerably reduces the extension bootstrap time upon restart.
Currently implemented:
LruCachebase class, with filesystem caching support