LibraryFeature's cannot support drop of multiple tracks. Repeated dropping can stall mixxx.

[toomuch]

As Mixxx currently cannot filter columns (https://bugs.launchpad.net/mixxx/+bug/675057) I wanted to create crates to access my genres faster. When choosing larger amounts of files it takes forever to finish. In this time, Mixxx is not usable.

Little (<100 tracks) Mixxx is "Not Responding"
Big (<5000 tracks) Mixxx is "Not Responding" forever, ergo crashing.

Win7 x64, Intel Pentium Dual Core T2390, 3GB RAM (Asus X51L laptop)
Mixxx 1.10.0beta1 x64

======

The reason for this is that LibraryFeature/SidebarModel only support a single drop method for children. dropAcceptChild(QModelIndex, QUrl). The problem with this is that WLibrarySidebar delivers each dropped item *one at a time*. In the case of toomuch's drop of 1000 tracks to a crate, this generates 1000 SQL queries instead of a single query to add the tracks in bulk.

[akash shetye (shetyeakash)]

I found a very efficient way to do bulk inserts on the database, the same approach may be extended to include other DAO functionalities that suffer from huge overhead due to frequent commits. The patch uses transactions and the bulk inserts within the transaction to overcome the need to auto-commit by committing at the end only.

[RJ Skerry-Ryan (rryan)]

Hi Akash,

Thanks for the patch! On the whole it looks good and this will surely speed up drag-and-dropping and adding to crates by a lot.

Here are my comments:

* Tabs vs. Spaces -- please make sure to configure your text editor to insert 4-spaces instead of tab characters. See the project coding guidelines here: http://mixxx.org/wiki/doku.php/coding_guidelines

* You've changed the CrateDAO API. The method "addTrackToCrate" now takes a list of track IDs and in every case this was called with a single track-id you create a 1-item list. Also, it doesn't make sense that the method name is "addTrackToCrate" when it actually adds multiple tracks to a crate. To eliminate this duplication, why not make a new method, CrateDAO::addTracksToCrate which takes a list of integers. Simply change the implementation of addTrackToCrate to just insert the single ID into a list and call addTracksToCrate.

* When adding a new method or changing an old one, it's good practice to add a comment to the header file. This way uncommented code gets commented over time.

For example, in your line where you change the method declaration, you could have written:

// Takes a list of track IDs in 'm_pTrackIdList' and adds them to the crate with the id 'crateId' and returns true if successful.
bool addTrackToCrate(QList<int> *m_pTrackIdList, int crateId);

* In addTracksToCrate
  - You execute raw SQL transaction statements. Instead use m_database.transaction(), m_database.rollback(), m_database.commit() etc. instead.
  - Don't emit() within a SQL transaction. This can lead to other database methods being called by other parts of MIxxx. Instead wait until you commit the transaction, then re-iterate over the list of track-ids and emit trackAdded() for each one.
  - Pass the QList argument by-value instead of as a pointer. Use of a pointer isn't necessary here because Qt has implicit sharing.

See http://doc.qt.nokia.com/4.7-snapshot/implicit-sharing.html for more info about implicit sharing.

* In general, try not to leave qDebug statements in your patches unless you intend for the statement to be printed to the log of every MIxxx user. Mixxx is already quite chatty on the console so we'd rather not add to the lines it prints :).

* In general, don't leave lines that you're replacing commented out. All you have to do is read plenty of Mixxx code to see the proof that commented code just sits there and rots for years without providing much value.

[akash shetye (shetyeakash)]

 Thank you for the time Ryan, I'll resubmit a patch with the suggested changes. I'll make it a point to comment code, it does help, esp. if you are new to code. I happen to use pretty basic stuff for development and things are getting unmanageable. I am planning this big PC format and Ubuntu re-installation after my midterms.

Letting you know that I'll be refining all my contributions and more after 16th of this month. I had no idea that emitting within sql is problematic, thanks for all the info, super-helpful.

[akash shetye (shetyeakash)]

Hey guys, I am working on a branch to improve the mass drop n load functionalities of Mixxx including drops from external file managers. The current patch only addresses the performance issue with the 'add to playlist or crate' from the library. I'll put a link of the bzr branch as I develop. Need a little time on this one.

[RJ Skerry-Ryan (rryan)]

I just merged Akash's branch (thanks Akash!). This is a good first step as it adds APIs for multiple-adds to the DAOs and uses them from WTrackTableView. There is one remaining aspect of this problem which is that LibraryFeature doesn't support dropping of multiple tracks. The DAO APIs are there but the LibraryFeature's can't use them since their drop-callbacks only get a single track.

[akash shetye (shetyeakash)]

Hey Ryan, I am working on that, just that the semester is drawing to an end
and exams are on. I agree it doesn't answer the fundamental problem of the
bug filed. I will be free by 18th of May and get back to developing for
Mixxx. Thanks for my first Branch Merge.

On Fri, May 4, 2012 at 11:11 AM, RJ Ryan <email address hidden> wrote:

> I just merged Akash's branch (thanks Akash!). This is a good first step
> as it adds APIs for multiple-adds to the DAOs and uses them from
> WTrackTableView. There is one remaining aspect of this problem which is
> that LibraryFeature doesn't support dropping of multiple tracks. The DAO
> APIs are there but the LibraryFeature's can't use them since their drop-
> callbacks only get a single track.
>
> --
> You received this bug notification because you are a bug assignee.
> https://bugs.launchpad.net/bugs/889825
>
> Title:
> LibraryFeature's cannot support drop of multiple tracks. Repeated
> dropping can stall mixxx.
>
> Status in Mixxx:
> Confirmed
> Status in Mixxx 1.10 series:
> Confirmed
> Status in Mixxx 1.9 series:
> Won't Fix
>
> Bug description:
> As Mixxx currently cannot filter columns
> (https://bugs.launchpad.net/mixxx/+bug/675057) I wanted to create
> crates to access my genres faster. When choosing larger amounts of
> files it takes forever to finish. In this time, Mixxx is not usable.
>
> Little (<100 tracks) Mixxx is "Not Responding"
> Big (<5000 tracks) Mixxx is "Not Responding" forever, ergo crashing.
>
> Win7 x64, Intel Pentium Dual Core T2390, 3GB RAM (Asus X51L laptop)
> Mixxx 1.10.0beta1 x64
>
> ======
>
> The reason for this is that LibraryFeature/SidebarModel only support a
> single drop method for children. dropAcceptChild(QModelIndex, QUrl).
> The problem with this is that WLibrarySidebar delivers each dropped
> item *one at a time*. In the case of toomuch's drop of 1000 tracks to
> a crate, this generates 1000 SQL queries instead of a single query to
> add the tracks in bulk.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/889825/+subscriptions
>

[Max Linke (max-linke)]

I've tuned the drag and drop performance of the sidebar model today. Before we didn't check if the source of the DropEvent was mixxx or some other programm. Luckely Qt can tell us which is the case, DropEvent.source() will return 0 if the event does not originate from within mixxx.

I used that information and now when ever a drop event occurs inside of mixxx we just look up the trackIds in the database.
This way the time for dropEvents is nearly independent of the number of tracks that are droped. For each dropEvent I noticed a freeze of about 0.25s (also droping ~1000 Tracks). This should be about the max speed we can get for a dropEvent.

Writing the patch I noticed that dropAccept(...) and dropAcceptChild(...) are reimplemented in a lot of classes but are doing nothing expect for a few exceptions (mixxxlibraryfeature,cratefeature,autodj,playlists). I would find the code easier to understand if we define these functions in LibraryFeature and only reimplement them if we want the subclass to actually use the function.

[RJ Skerry-Ryan (rryan)]

Thanks Max! The patch looks good except for the manual escaping of the track locations in the SQL query.

Please use the FieldEscaper class instead from src/library/queryutil.h and then just join the QStringList by ,

[Max Linke (max-linke)]

Here is the updated patch using the Fieldescaper method

[Max Linke (max-linke)]

Here is a version that also removes function from the features when they are not using them.

I like this version better because now it is obvious from libraryfeature.h what is absolutely required to subclass it and what features are optional. Also this way it is easy to see from the *feature.h file what operations are intended to be supported by it.
I orientated myself here on the TrackModel class.

[RJ Skerry-Ryan (rryan)]

Awesome -- thanks. Please commit it!

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/6105