Improve startup performance by deferring playlist loading #312
Conversation
djmattyg007
force-pushed
the
improve_startup_performance
branch
3 times, most recently
from
bd0b97b
to
2a8f225
Compare
|
Sorry, I seemed to have missed this. I went down this path myself and I scrapped it. I think my main issue was reasoning about how it fitted into the (long-overdue) playlist modification PR. And it appeared quite confusing for non-webclient clients (e.g. MPD) who would see zero playlists. I was leaning towards just keeping a peristent cache of the playlist data (like libspotify does/did) and make the refreshing really quick. |
|
Isn't it reasonable to expect non-websocket clients to have some kind of refresh functionality? Otherwise, how are they ever notified of updates? Surely if you were expecting to see playlists and saw none, you'd just refresh. |
|
Also I'd wager that most of the time Mopidy servers are running for days or weeks without being restarted. This just impacts startup performance, a one-time cost. This is actually mostly useful for Mopidy local development. |
This change prevents loading of Spotify playlists from blocking other common Mopidy operations. These include startup and browsing, as well as refreshing of playlists in general. Loading Spotify playlists will always happen in a dedicated thread, and only one refresh thread will run at a time. The playlists_loaded event is manually re-dispatched when Spotify is done loading playlists, as a signal that clients should refresh their list of playlists. This is necessary, as otherwise clients relying solely on websocket notifications may never discover the newly-loaded playlists.
djmattyg007
force-pushed
the
improve_startup_performance
branch
from
2a8f225
to
b264935
Compare
| try: | ||
| with utils.time_logger("playlists.refresh()", logging.DEBUG): | ||
| _sp_links.clear() | ||
| self._backend._web_client.clear_cache() |
There was a problem hiding this comment.
What's the motivation for explicitly clearing cache here? Seem to come from 6dd39cf @kingosticks
There was a problem hiding this comment.
Because otherwise you might still be < max-age and therefore won't fetch the fresh data, possibly leaving you with stale data. Which you don't want because you specifically asked to refresh.
There was a problem hiding this comment.
Got it - my thinking is that the ideal behavior would be to always fetch data, but keep the cache until fresh data has arrived.
Does that make sense?
| _sp_links.clear() | ||
| self._backend._web_client.clear_cache() | ||
| count = 0 | ||
| for playlist_ref in self._get_flattened_playlist_refs(): |
There was a problem hiding this comment.
@djmattyg007 How much have you been running this branch? I haven't validated this properly but looks to me like there may be concurrency issues here if a client attempts to enumerate the playlists in the middle of this operation (concurrent enumeration of a collection as it's modified).
Would it make sense to introduce a mutex here?
|
Thanks for the great original work on this. Sorry it took so long to get the improvement in. Closing in favour of #382 which is based, in part, on this. |
This change prevents loading of Spotify playlists from blocking other
common Mopidy operations. These include startup and browsing, as well as
refreshing of playlists in general. Loading Spotify playlists will
always happen in a dedicated thread, and only one refresh thread will
run at a time.
The playlists_loaded event is manually re-dispatched when Spotify is
done loading playlists, as a signal that clients should refresh their
list of playlists. This is necessary, as otherwise clients relying
solely on websocket notifications may never discover the newly-loaded
playlists.