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

AutoDJ: fix GUI freeze when updating duration for many selected tracks #12530

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jan 7, 2024

fixes #12520

  • move the duration collection to TrackDAO
  • query the database duration column instead of inspecting each Track object

  • move the duration collection to PlaylistTableModel
    (could also be in any of the classes that is derived from but... YAGNI?)
  • query the duration column of each row instead of inspecting each Track object

Now updating the info for ~16k tracks takes ~5ms here.
With the 2.4 code, updating the info for 3 tracks already takes ~40ms 😬

@ronso0
Copy link
Member Author

ronso0 commented Jan 7, 2024

@uklotzde Do you have time to take a look?

@ronso0
Copy link
Member Author

ronso0 commented Jan 8, 2024

Hmm, no, this should better be a helper in PlaylistTableModel

@ronso0 ronso0 marked this pull request as draft January 8, 2024 00:42
@ronso0
Copy link
Member Author

ronso0 commented Jan 8, 2024

Hmm, no, this should better be a helper in PlaylistTableModel

Yes, and that's also much faster: ~5ms vs. 28ms for 16k tracks (maybe because no QHash and no second iteration?)

edit
I was curious if with SQL but interestingly querying the db for the total duration of certain playlist positions is slower.
This takes ~20ms for 16k tracks (regardless the number of selected rows of course):

SELECT 
  SUM(library.duration) 
FROM PlaylistTracks 
LEFT JOIN library 
  ON PlaylistTracks.track_id = library.id 
  AND PlaylistTracks.playlist_id = playlistId
  AND PlaylistTracks.position IN (selectedPositions)

@ronso0 ronso0 marked this pull request as ready for review January 8, 2024 01:43
@ronso0 ronso0 added the gui label Jan 8, 2024
@@ -319,6 +319,22 @@ void PlaylistTableModel::shuffleTracks(const QModelIndexList& shuffle, const QMo
m_pTrackCollectionManager->internalCollection()->getPlaylistDAO().shuffleTracks(m_iPlaylistId, positions, allIds);
}

double PlaylistTableModel::getDurationOfRows(const QModelIndexList& indices) {
Copy link
Member

Choose a reason for hiding this comment

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

can you add a short explanatory comment, like:
// Derive total duration from the display widget instead of the database to speed up calculation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add that in DlgAutoDJ where it is called, no need to mention that here IMO

}

const int durationColumnIndex = fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_DURATION);
for (const auto index : indices) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (const auto index : indices) {
for (const auto& index : indices) {

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

just on missing &

@ronso0
Copy link
Member Author

ronso0 commented Jan 9, 2024

Done.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@daschuer daschuer merged commit c28a789 into mixxxdj:2.4 Jan 9, 2024
13 checks passed
@ronso0
Copy link
Member Author

ronso0 commented Jan 9, 2024

Oh, I just pushed the changes suggested by @uklotzde (it was a comment not a formal change request).
4240e90?notification_referrer_id=NT_kwDOAFqMd7I5MDIwODc3MjE3OjU5MzQxOTk

The update is #12537

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants