From 09e839353eefcbb5351546683cdd04201b8bfcf1 Mon Sep 17 00:00:00 2001 From: Simeon Bird Date: Sat, 6 Dec 2014 20:41:31 -0500 Subject: [PATCH 1/3] 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); From 401f07c7cb4a05b99d0108916a7b8d4a5738e04f Mon Sep 17 00:00:00 2001 From: Simeon Bird Date: Sat, 6 Dec 2014 23:36:27 -0500 Subject: [PATCH 2/3] Avoid db commits during startup When starting clementine, each playlist is loaded in turn. When loading a playlist, the new tab order is committed to the db, but we don't need to do that here because we know that once all the playlists are loaded the tab order will be the same as it was initially. This speeds startup substantially. kstartperf: Before: 3.5s After: 1.5s --- src/playlist/playlisttabbar.cpp | 23 ++++++++++++++++++++--- src/playlist/playlisttabbar.h | 3 +++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/playlist/playlisttabbar.cpp b/src/playlist/playlisttabbar.cpp index 341100873..f9b397a98 100644 --- a/src/playlist/playlisttabbar.cpp +++ b/src/playlist/playlisttabbar.cpp @@ -44,6 +44,7 @@ PlaylistTabBar::PlaylistTabBar(QWidget* parent) menu_(new QMenu(this)), menu_index_(-1), suppress_current_changed_(false), + initialized_(false), rename_editor_(new RenameTabLineEdit(this)) { setAcceptDrops(true); setElideMode(Qt::ElideRight); @@ -79,6 +80,16 @@ void PlaylistTabBar::SetManager(PlaylistManager* manager) { manager_ = manager; connect(manager_, SIGNAL(PlaylistFavorited(int, bool)), SLOT(PlaylistFavoritedSlot(int, bool))); + connect(manager_, SIGNAL(PlaylistManagerInitialized()), this, + SLOT(PlaylistManagerInitialized())); +} + +void PlaylistTabBar::PlaylistManagerInitialized() { + // Signal that we are done loading and thus further changes should be + // committed to the db. + initialized_ = true; + disconnect(manager_, SIGNAL(PlaylistManagerInitialized()), this, + SLOT(PlaylistManagerInitialized())); } void PlaylistTabBar::contextMenuEvent(QContextMenuEvent* e) { @@ -281,6 +292,7 @@ void PlaylistTabBar::InsertTab(int id, int index, const QString& text, insertTab(index, text); setTabData(index, id); setTabToolTip(index, text); + FavoriteWidget* widget = new FavoriteWidget(id, favorite); widget->setToolTip( tr("Click here to favorite this playlist so it will be saved and remain " @@ -291,14 +303,19 @@ void PlaylistTabBar::InsertTab(int id, int index, const QString& text, setTabButton(index, QTabBar::LeftSide, widget); suppress_current_changed_ = false; - if (currentIndex() == index) emit CurrentIdChanged(id); + // If we are still starting up, we don't need to do this, as the + // tab ordering after startup will be the same as was already in the db. + if (initialized_) { + if (currentIndex() == index) emit CurrentIdChanged(id); - // Update playlist tab order/visibility - TabMoved(); + // Update playlist tab order/visibility + TabMoved(); + } } void PlaylistTabBar::TabMoved() { QList ids; + for (int i = 0; i < count(); ++i) { ids << tabData(i).toInt(); } diff --git a/src/playlist/playlisttabbar.h b/src/playlist/playlisttabbar.h index 3b795367c..98cb74b93 100644 --- a/src/playlist/playlisttabbar.h +++ b/src/playlist/playlisttabbar.h @@ -82,6 +82,8 @@ signals: // Used when playlist's favorite flag isn't changed from the favorite widget // (e.g. from the playlistlistcontainer): will update the favorite widget void PlaylistFavoritedSlot(int id, bool favorite); + // Used to signal that the playlist manager is done starting up + void PlaylistManagerInitialized(); void TabMoved(); void Save(); @@ -99,6 +101,7 @@ signals: int drag_hover_tab_; bool suppress_current_changed_; + bool initialized_; // Editor for inline renaming RenameTabLineEdit* rename_editor_; From 3f9b5f466327c2ec60a9fe9557f122e2ce9137fb Mon Sep 17 00:00:00 2001 From: Simeon Bird Date: Sun, 7 Dec 2014 20:51:29 -0500 Subject: [PATCH 3/3] Do Podcast updates on song change off main thread Each time the song is changed, the podcast backend checks whether the new song is a podcast and, if so, mark it as listened to. This requires 1-2 db queries, so do it off the main thread. Time to change song before: 300 ms after: 50 ms usually, 80 ms sometimes --- src/podcasts/podcastservice.cpp | 15 +++++++++++---- src/podcasts/podcastservice.h | 1 + 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/podcasts/podcastservice.cpp b/src/podcasts/podcastservice.cpp index a0e501724..50842d35c 100644 --- a/src/podcasts/podcastservice.cpp +++ b/src/podcasts/podcastservice.cpp @@ -39,6 +39,7 @@ #include #include +#include const char* PodcastService::kServiceName = "Podcasts"; const char* PodcastService::kSettingsGroup = "Podcasts"; @@ -60,8 +61,7 @@ PodcastService::PodcastService(Application* app, InternetModel* parent) proxy_(new PodcastSortProxyModel(this)), context_menu_(nullptr), root_(nullptr), - organise_dialog_(new OrganiseDialog(app_->task_manager(), - nullptr)) { + organise_dialog_(new OrganiseDialog(app_->task_manager(), nullptr)) { icon_loader_->SetModel(model_); proxy_->setSourceModel(model_); proxy_->setDynamicSortFilter(true); @@ -78,8 +78,8 @@ PodcastService::PodcastService(Application* app, InternetModel* parent) connect(app_->playlist_manager(), SIGNAL(CurrentSongChanged(Song)), SLOT(CurrentSongChanged(Song))); - connect(organise_dialog_.get(), SIGNAL(FileCopied(int)), - this, SLOT(FileCopied(int))); + connect(organise_dialog_.get(), SIGNAL(FileCopied(int)), this, + SLOT(FileCopied(int))); } PodcastService::~PodcastService() {} @@ -602,6 +602,13 @@ void PodcastService::ShowConfig() { } void PodcastService::CurrentSongChanged(const Song& metadata) { + // This does two db queries, and we are called on every song change, so run + // this off the main thread. + QtConcurrent::run(this, &PodcastService::UpdatePodcastListenedStateAsync, + metadata); +} + +void PodcastService::UpdatePodcastListenedStateAsync(const Song& metadata) { // Check whether this song is one of our podcast episodes. PodcastEpisode episode = backend_->GetEpisodeByUrlOrLocalUrl(metadata.url()); if (!episode.is_valid()) return; diff --git a/src/podcasts/podcastservice.h b/src/podcasts/podcastservice.h index 3e1b5eb36..92fcf9140 100644 --- a/src/podcasts/podcastservice.h +++ b/src/podcasts/podcastservice.h @@ -96,6 +96,7 @@ class PodcastService : public InternetService { private: void EnsureAddPodcastDialogCreated(); + void UpdatePodcastListenedStateAsync(const Song& metadata); void PopulatePodcastList(QStandardItem* parent); void UpdatePodcastText(QStandardItem* item, int unlistened_count) const; void UpdateEpisodeText(