From 09e839353eefcbb5351546683cdd04201b8bfcf1 Mon Sep 17 00:00:00 2001 From: Simeon Bird Date: Sat, 6 Dec 2014 20:41:31 -0500 Subject: [PATCH] Speed up playlist restoring by moving sqlite query off main thread The playlist fetching uses QtConcurrent to make the playlist on a different thread (possibly concurrently for each item). However, profiling reveals that the slow operation is fetching the rows from the SQLite database, making this redundant. Instead move the whole playlist loading, including the database access, into a single function, and call that function in a different thread via QtConcurrent::run. This has the side effect of moving all the concurrent stuff from PlaylistBackend into the callers. kstartperf measures: Before: 7.5s cold 3.6 s warm After: ~4.0 s cold 3.5 s warm --- src/playlist/playlist.cpp | 7 ++-- src/playlist/playlistbackend.cpp | 55 +++++++++++++++++--------------- src/playlist/playlistbackend.h | 9 +++--- src/playlist/playlistmanager.cpp | 18 ++++++----- src/playlist/playlistmanager.h | 2 +- 5 files changed, 48 insertions(+), 43 deletions(-) diff --git a/src/playlist/playlist.cpp b/src/playlist/playlist.cpp index 04c8c53dd..a5a04e258 100644 --- a/src/playlist/playlist.cpp +++ b/src/playlist/playlist.cpp @@ -1446,7 +1446,7 @@ void Playlist::Save() const { } namespace { -typedef QFutureWatcher> PlaylistItemFutureWatcher; +typedef QFutureWatcher> PlaylistItemFutureWatcher; } void Playlist::Restore() { @@ -1456,7 +1456,8 @@ void Playlist::Restore() { virtual_items_.clear(); library_items_by_id_.clear(); - PlaylistBackend::PlaylistItemFuture future = backend_->GetPlaylistItems(id_); + QFuture> future = + QtConcurrent::run(backend_, &PlaylistBackend::GetPlaylistItems, id_); PlaylistItemFutureWatcher* watcher = new PlaylistItemFutureWatcher(this); watcher->setFuture(future); connect(watcher, SIGNAL(finished()), SLOT(ItemsLoaded())); @@ -1467,7 +1468,7 @@ void Playlist::ItemsLoaded() { static_cast(sender()); watcher->deleteLater(); - PlaylistItemList items = watcher->future().results(); + PlaylistItemList items = watcher->future().result(); // backend returns empty elements for library items which it couldn't // match (because they got deleted); we don't need those diff --git a/src/playlist/playlistbackend.cpp b/src/playlist/playlistbackend.cpp index 82fd33e53..d8f3d3fd7 100644 --- a/src/playlist/playlistbackend.cpp +++ b/src/playlist/playlistbackend.cpp @@ -24,7 +24,6 @@ #include #include #include -#include #include #include "core/application.h" @@ -138,7 +137,7 @@ PlaylistBackend::Playlist PlaylistBackend::GetPlaylist(int id) { return p; } -QList PlaylistBackend::GetPlaylistRows(int playlist) { +QSqlQuery PlaylistBackend::GetPlaylistRows(int playlist) { QMutexLocker l(db_->Mutex()); QSqlDatabase db(db_->Connect()); @@ -162,42 +161,46 @@ QList PlaylistBackend::GetPlaylistRows(int playlist) { " LEFT JOIN jamendo.songs AS jamendo_songs" " ON p.library_id = jamendo_songs.ROWID" " WHERE p.playlist = :playlist"; - QSqlQuery q(query, db); - + QSqlQuery q(db); + // Forward iterations only may be faster + q.setForwardOnly(true); + q.prepare(query); q.bindValue(":playlist", playlist); q.exec(); - if (db_->CheckErrors(q)) return QList(); - QList rows; + return q; +} +QList PlaylistBackend::GetPlaylistItems(int playlist) { + QSqlQuery q = GetPlaylistRows(playlist); + // Note that as this only accesses the query, not the db, we don't need the + // mutex. + if (db_->CheckErrors(q)) return QList(); + + // it's probable that we'll have a few songs associated with the + // same CUE so we're caching results of parsing CUEs + std::shared_ptr state_ptr(new NewSongFromQueryState()); + QList playlistitems; while (q.next()) { - rows << SqlRow(q); + playlistitems << NewPlaylistItemFromQuery(SqlRow(q), state_ptr); } - - return rows; + return playlistitems; } -QFuture PlaylistBackend::GetPlaylistItems(int playlist) { - QMutexLocker l(db_->Mutex()); - QList rows = GetPlaylistRows(playlist); +QList PlaylistBackend::GetPlaylistSongs(int playlist) { + QSqlQuery q = GetPlaylistRows(playlist); + // Note that as this only accesses the query, not the db, we don't need the + // mutex. + if (db_->CheckErrors(q)) return QList(); // it's probable that we'll have a few songs associated with the // same CUE so we're caching results of parsing CUEs std::shared_ptr state_ptr(new NewSongFromQueryState()); - return QtConcurrent::mapped( - rows, std::bind(&PlaylistBackend::NewPlaylistItemFromQuery, this, _1, - state_ptr)); -} - -QFuture PlaylistBackend::GetPlaylistSongs(int playlist) { - QMutexLocker l(db_->Mutex()); - QList rows = GetPlaylistRows(playlist); - - // it's probable that we'll have a few songs associated with the - // same CUE so we're caching results of parsing CUEs - std::shared_ptr state_ptr(new NewSongFromQueryState()); - return QtConcurrent::mapped( - rows, std::bind(&PlaylistBackend::NewSongFromQuery, this, _1, state_ptr)); + QList songs; + while (q.next()) { + songs << NewSongFromQuery(SqlRow(q), state_ptr); + } + return songs; } PlaylistItemPtr PlaylistBackend::NewPlaylistItemFromQuery( diff --git a/src/playlist/playlistbackend.h b/src/playlist/playlistbackend.h index 1d7c10179..f9d347c4e 100644 --- a/src/playlist/playlistbackend.h +++ b/src/playlist/playlistbackend.h @@ -18,7 +18,6 @@ #ifndef PLAYLISTBACKEND_H #define PLAYLISTBACKEND_H -#include #include #include #include @@ -53,7 +52,6 @@ class PlaylistBackend : public QObject { QString special_type; }; typedef QList PlaylistList; - typedef QFuture PlaylistItemFuture; static const int kSongTableJoins; @@ -61,8 +59,9 @@ class PlaylistBackend : public QObject { PlaylistList GetAllOpenPlaylists(); PlaylistList GetAllFavoritePlaylists(); PlaylistBackend::Playlist GetPlaylist(int id); - PlaylistItemFuture GetPlaylistItems(int playlist); - QFuture GetPlaylistSongs(int playlist); + + QList GetPlaylistItems(int playlist); + QList GetPlaylistSongs(int playlist); void SetPlaylistOrder(const QList& ids); void SetPlaylistUiPath(int id, const QString& path); @@ -87,7 +86,7 @@ class PlaylistBackend : public QObject { QMutex mutex_; }; - QList GetPlaylistRows(int playlist); + QSqlQuery GetPlaylistRows(int playlist); Song NewSongFromQuery(const SqlRow& row, std::shared_ptr state); diff --git a/src/playlist/playlistmanager.cpp b/src/playlist/playlistmanager.cpp index 2aa5be678..c958baf71 100644 --- a/src/playlist/playlistmanager.cpp +++ b/src/playlist/playlistmanager.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include using smart_playlists::GeneratorPtr; @@ -182,21 +183,22 @@ void PlaylistManager::Save(int id, const QString& filename, // Playlist is not in the playlist manager: probably save action was // triggered // from the left side bar and the playlist isn't loaded. - QFuture future = playlist_backend_->GetPlaylistSongs(id); - QFutureWatcher* watcher = new QFutureWatcher(this); + QFuture> future = QtConcurrent::run( + playlist_backend_, &PlaylistBackend::GetPlaylistSongs, id); + QFutureWatcher* watcher = new QFutureWatcher(this); watcher->setFuture(future); NewClosure(watcher, SIGNAL(finished()), this, - SLOT(ItemsLoadedForSavePlaylist(QFutureWatcher*, QString, - Playlist::Path)), + SLOT(ItemsLoadedForSavePlaylist(QFutureWatcher*, + QString, Playlist::Path)), watcher, filename); } } -void PlaylistManager::ItemsLoadedForSavePlaylist(QFutureWatcher* watcher, - const QString& filename, - Playlist::Path path_type) { - SongList song_list = watcher->future().results(); +void PlaylistManager::ItemsLoadedForSavePlaylist( + QFutureWatcher* watcher, const QString& filename, + Playlist::Path path_type) { + SongList song_list = watcher->future().result(); parser_->Save(song_list, filename, path_type); } diff --git a/src/playlist/playlistmanager.h b/src/playlist/playlistmanager.h index 4d1072827..b0dc53187 100644 --- a/src/playlist/playlistmanager.h +++ b/src/playlist/playlistmanager.h @@ -232,7 +232,7 @@ class PlaylistManager : public PlaylistManagerInterface { void OneOfPlaylistsChanged(); void UpdateSummaryText(); void SongsDiscovered(const SongList& songs); - void ItemsLoadedForSavePlaylist(QFutureWatcher* watcher, + void ItemsLoadedForSavePlaylist(QFutureWatcher* watcher, const QString& filename, Playlist::Path path_type);