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

lp1845837: Speed up purging of tracks #2393

Merged
merged 6 commits into from
Dec 14, 2019
Merged

lp1845837: Speed up purging of tracks #2393

merged 6 commits into from
Dec 14, 2019

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Dec 10, 2019

https://bugs.launchpad.net/mixxx/+bug/1845837

This feature is almost unusable in its current state! Now only a single signal for multiple playlists is send after purging multiple tracks from multiple playlists.

@uklotzde uklotzde requested a review from daschuer December 10, 2019 13:00
@uklotzde uklotzde added this to the 2.3.0 milestone Dec 10, 2019
@Be-ing
Copy link
Contributor

Be-ing commented Dec 10, 2019

This looks like a fairly small change. Should we target it for the 2.2 branch?

Verified

This commit was signed with the committer’s verified signature.
DomPeliniAerospike Dominic Pelini
@uklotzde uklotzde changed the base branch from master to 2.2 December 10, 2019 16:22
@uklotzde
Copy link
Contributor Author

uklotzde commented Dec 10, 2019

I have backported the changes to 2.2. The merge conflicts seem to be manageable. Let's give it a try...

More merge conflicts need to be resolved when porting to master.

@uklotzde uklotzde modified the milestones: 2.3.0, 2.2.x, 2.2.4 Dec 10, 2019
@uklotzde
Copy link
Contributor Author

The Travis CI build failures on macOS are unrelated.

@uklotzde
Copy link
Contributor Author

Includes #2396

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.

Some comments Thank you for picking this up.

.travis.yml Show resolved Hide resolved
}

void PlaylistDAO::removeTracksFromPlaylist(const int playlistId, QList<int>& positions) {
void PlaylistDAO::removeTracksFromPlaylist(int playlistId, QList<int> positions) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not const QList& ? A copy by value increases the shared counter, which requires qt:as_const() in range based loops to be efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a local, writable copy is need for sorting. First line of the function body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The QList is actually not shared, but moved into this function. The caller never accesses it again after calling the function. The copy happens, because instead of move semantics Qt relies on implicit sharing.

I could add a std::move at the caller's site, because QList has a move constructor. This should eliminate the temporary copy.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, thank you for explaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I omitted to use std::move in the first place because I was recently told not to use it in Qt code ;) What I definitely won't do is checking each and every Qt class before using it. I prefer to use it if it is semantically correct, whether it is supported or not.

src/library/dao/playlistdao.cpp Outdated Show resolved Hide resolved
src/library/setlogfeature.cpp Outdated Show resolved Hide resolved
src/library/setlogfeature.cpp Show resolved Hide resolved
@uklotzde
Copy link
Contributor Author

All issues addressed.

@daschuer
Copy link
Member

Thank you. LGTM

@daschuer daschuer merged commit c6953e5 into mixxxdj:2.2 Dec 14, 2019
@uklotzde uklotzde deleted the purge_tracks_from_playlists branch December 15, 2019 08:45
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.

None yet

3 participants