Library concurency refactoring

[troyane]

Hi, developers!

I need to get your code review.

Here is my branch "LibraryConcurencyRefactoring" developed as Google Summer of Code 2013 project.

Here you can read about our implemented ideas: http://mixxx.org/wiki/doku.php/lambda_scheme.

You can contact me via GitHub -- https://github.com/troyane, e-mail -- troyan3 at gmail dot com, or at mixxx irc channel -- tro.

P. S. There are some known issues with Rhythmbox playlist. It will be solved soon.

[MK-42]

I would like to add:

I've added that to the list. If you have a idea how you've done that, that would be nice. Maybe by some friend-class or sth. like this?

[MK-42]

it should be unit testable

Just thought about it again. What do you want to be testable? If the access to the db is done right or all the individual functions if they do things right?

[daschuer]

I original meant that we are able to add uinttests that verifies the new API works as desired.
Of cause it would also be nice If new API allows to implement unit tests for the individual functions.
But implementing them likely involves a change of the business logic, hat should be avoided in the first step.

[daschuer]

I have managed to recall the private DB access idea:

The lambdas are called here:
https://github.com/troyane/mixxx/blob/LibraryConcurencyRefactoring/src/library/trackcollection.cpp#L82
with the context, captured in

[...]

Here: https://github.com/troyane/mixxx/blob/LibraryConcurencyRefactoring/src/dlgautodj.cpp#L53
for example we access the database by the captured "this" pointer and the m_pTrackCollection member.

If we change the lambda prototype to

[...] (TrackCollection* pTrackCollection ) { }

and invoke it by

lambda(this) 

in TrackCollection.
we can remove all direct TrackCollection references from the library access code.

This dos not entirely hide the TrackCollection functions, but the instance pointer to it.
I think this will fulfill a huge part our requirements.

In a next step we may split TrackCollection in a way, that the library access functions are divided between GUI thread functions and TrackCollection thread functions.
Finally a new TrackCollectionDB pointer can be a private Member of a new TrackCollectionGUI class.
This split is probably the hardest part.

What do you think?

[MK-42]

Thats neat! I would prefer this over the current solution a lot, because the Compiler prevents wrong usage then! So we should go this way i think. That i would not fear that much that we Miss a call somewhere and generate a Party stopper by that.

At the Moment I think it will be easier to Start from current master with this and maybe reuse the trackcollection parts with handling and locking the lambdas. The Main executer would be usable as well maybe.

But we should stay on track with 1.12 release at first. Any point i can help there? Is it only Windows making problems?

[daschuer]

We have still a long list of 1.12.0 bugs any help is appreciated.
https://launchpad.net/mixxx/+milestone/1.12.0
However since any software has bugs and Mixxx 1.12 is much better compared to 1.11 IMHO we should push it out the door once we have fixed or moved the critical bugs from the 1.12 milestone.

[daschuer]

For this branch:
It contains the work from a whole summer of code, including many detail fixes.
If we agree, to continue a way like this, It is reasonable to me to go the 800 PR merging pass.
By going this way GIT will axiomatically point you to the regions you have to take hand on. :-)
You will face conflicts where you have to do work in a new approach as well.
For my feeling this will be faster than starting again from master.
And you will hopefully discover bugs, with your pair of unbiased eyes.

[MK-42]

I'm looking for a solution to hide the database-access from everywhere except the lambdas.

I like your proposal to make the lambda prototype
[...] (TrackCollection* pTrackCollection ) { }
to prevent anyone calling anything without a lambda.
We would have to make sure that noone uses the TrackCollection pointer without a lambda and everything would be fine.

The TrackCollection instance is already moved to mixxx.cpp in this PR. We then need to check if no one passes that instance anywhere and would be sure that no one can access the database without the lambdas.

Are these assumptions correct?

[troyane]

Hello, @daschuer @MK-42 @rryan!
I want to apologize for my absence. But probably all of you heard what is happening now in my country..

I'm happy to hear that this branch is resurrected and someone is still interested in such tricky (but working) approach to this task. It will be pleasant to me if this branch will be merged :)

I want to help, so I'll try to find several hours each week to help.

[MK-42]

Welcome back @troyane !

I noticed that I've missed something in my last comment:
we need the instance of trackCollection to call call[As|S]ync on. We can only prevent this by splitting TrackCollection as @daschuer mentioned above.

Would it make sense to do this change right from the start - to prevent wrong usage? This will not make the PR much larger but safer or what do you think?

[troyane]

@MK-42

• I need to remember all what is going on here. By the way, it can be very useful to you this article: http://mixxx.org/wiki/doku.php/lambda_scheme and this small example of lambdas usage -- https://github.com/troyane/lambdaConcurrent.
• I do really understand that there will be huge diff because code was written ~ 2 years ago. That's why IMO we need to prepare some adoption. Lets discuss how to do it in easiest way.
• I've read about splitting of TrackCollection. Unfortunately, I can't understand main reason for this. But, hope we can discuss all.

[MK-42]

I've read about splitting of TrackCollection. Unfortunately, I can't understand main reason for this. But, hope we can discuss all.

The problem is the following:

• We need every call to a dao/Database to be made through a lambda to have it in the right thread
• At the moment we can not guarantee, that we don't miss a point in the code where someone added a trackCollection->getDao->doSomething without putting that in a lambda
• We have to manually merge ~400 PRs into this branch to get it up to master again so that will happen a lot.
• Ideally no one can ever call a DB-function from the wrong thread.
  The Idea we/@daschuer had was:
• Move everything except callSync/callAsync from TrackCollection to some other class. Lets call it TrackCollectionPrivate for now
• TrackCollection holds a private pointer on TrackCollectionPrivate
• pass that pointer to each lambda on execution.
  That way you could do something like

m_pTrackCollection->callSync(
            [&playlistId] (TrackCollectionPrivate pTrackCollectionPrivate) {
        PlaylistDAO& playlistDao = pTrackCollectionPrivate->getPlaylistDAO();
        playlistId = playlistDao.getPlaylistIdFromName(AUTODJ_TABLE);
        if (playlistId < 0) {
            playlistId = playlistDao.createPlaylist(AUTODJ_TABLE,
                                                    PlaylistDAO::PLHT_AUTO_DJ);
        }
    }, __PRETTY_FUNCTION__);

what would be called like

lambda(m_pTrackCollectionPrivate);

but no one except the TrackCollection itself has a reference to TrackCollectionPrivate so the methods of it can only be executed out of a lambda in call[As|S]ync.

I hope I stated everything right, @daschuer, correct me if I'm wrong.

Maybe it would make sense to then call TrackCollection->Database and TrackCollectionPrivate->TrackCollection again.

[MK-42]

Any opinions on splitting these two classes now in the current state of the branch and then merging against master? That would make sure (i think) that no code can call the DB the wrong way.
It would make the diff a little bigger though but I think only in places where we have changes anyway.

[daschuer]

@troyane: I am happy you are still taking care for your baby. Thank you.

@MK-42: sorry for the late responds. I think you got the issue correct.

Any opinions on splitting these two classes now in the current state of the branch and then merging against master?

This sounds reasonable, if there are not much changes in TrackCollection since splitting out this branch.
Normally it makes more sense to introduce new changes in top of a merged branch but in this case, we need a proof of concept without wasting time for merge before.

I am afraid doing the entire split will be quite hard, because of all the criss cross dependencies inside
TrackCollection. Maybe it is reasonable to start with TrackCollectionPrivate and TrackCollection
which are friend classed from each others. This way you are able to cut one dependency by an other while the code still compiles and works.

[MK-42]

if there are not much changes in TrackCollection since splitting out this branch

There was some code added to introduce sorting and like in SQLite3 I think. But no major changes except from that one.

I'm working on the Split of these classes right now and making good progress... The Library-Scanner will be tricky though

[MK-42]

my work of the last day....

https://github.com/MK-42/mixxx/tree/LibraryConcurencyRefactoring

The LibraryScanner has some direct references to the db and uses its own daos... maybe @troyane or @daschuer can explain how this actually works so that I can bring in the split there too?
Edit: to make it clear, I do not understand why the library-scanner has its own daos and reference to db?

nervous to see if this will work :)

[daschuer]

If you do a PR to this branch, we can use the nice GitHub features.
We have to decide what are the best merging steps though.
@troyane: any preferences?

[MK-42]

just opened #629 for your @daschuer

[rryan]

Superseded by #629.