Merge lp://qastaging/~albaguirre/mir/recycle-compositor-threads into lp://qastaging/mir

PASSED: Continuous integration, rev:1879
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2563/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1544
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1550
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1526
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1085
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1085/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/463
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/463/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2603
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12409

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2563/rebuild

Sounds like a good-old thread pool pattern [1]. Does it therefore have to be compositor specific? That would seem to limit code reuse.

[1] http://en.wikipedia.org/wiki/Thread_pool_pattern

OK, so two questions right now:

(1) Does the thread pool really need to be compositor-specific? That does limit code reuse.

(2) Given this is a workaround for one specific chipset, isn't "1605 lines (+996/-137)" an overkill? Seems rather large given it's a workaround for just one specific chip. Such workarounds you would hope are temporary, and so should be small so that they might be eliminated in future.

The size of the hammer seems disproportionate to the nail :)

If it's feasible I would too like to reduce the changes in the Compositor class. Based on Daniel's ThreadPool observation an alternative idea is:

class ThreadPool
{
// When the shared_ptr is released the thread will be marked as available for reuse
// The 'id' is used to enable reusing threads if the ThreadPool implementation supports it.
virtual std::shared_ptr<std::thread> run(void const* id, std::function<void()> const& functor) = 0;
virtual void clear_unused_threads() = 0;
};

// And either dependency on abstract ThreadPool:
MultiThreadedCompositor(...std::shared_ptr<ThreadPool> const& tp...);
// or it might as well use an object variable (abstract class not necessary) for now if this doesn't hinder testing (probably not a problem for testing since we are currently using threads directly internally anyway):
class MultiThreadedCompositor { ReusableThreadPool thread_pool; };

So MultiThreadedCompositor stays as it is more or less, and reverting to the previous behavior is easy: either revert the thread pool changes in MTC, or use a simple thread pool implementation that doesn't reuse threads.

"Needs discussion"

> OK, so two questions right now:
>
> (1) Does the thread pool really need to be compositor-specific? That does
> limit code reuse.
>
> (2) Given this is a workaround for one specific chipset, isn't "1605 lines
> (+996/-137)" an overkill? Seems rather large given it's a workaround for just
> one specific chip. Such workarounds you would hope are temporary, and so
> should be small so that they might be eliminated in future.
>
> The size of the hammer seems disproportionate to the nail :)

1) No it doesn't need to be. I'll make it generic.

2) I don't think so, since a bulk of the MP is adding tests (which we should never see as overkill).

> If it's feasible I would too like to reduce the changes in the Compositor
> class. Based on Daniel's ThreadPool observation an alternative idea is:
>
> class ThreadPool
> {
> // When the shared_ptr is released the thread will be marked as available for
> reuse
> // The 'id' is used to enable reusing threads if the ThreadPool implementation
> supports it.
> virtual std::shared_ptr<std::thread> run(void const* id, std::function<void()>
> const& functor) = 0;
> virtual void clear_unused_threads() = 0;
> };
>
> // And either dependency on abstract ThreadPool:
> MultiThreadedCompositor(...std::shared_ptr<ThreadPool> const& tp...);
> // or it might as well use an object variable (abstract class not necessary)
> for now if this doesn't hinder testing (probably not a problem for testing
> since we are currently using threads directly internally anyway):
> class MultiThreadedCompositor { ReusableThreadPool thread_pool; };
>
> So MultiThreadedCompositor stays as it is more or less, and reverting to the
> previous behavior is easy: either revert the thread pool changes in MTC, or
> use a simple thread pool implementation that doesn't reuse threads.
>
> "Needs discussion"

Ok, I'll rework.

Just being careful what we wish for...

If you do rework it and find the result is more complex/larger than this branch then maybe we should keep this one. A non-generalised solution like this one is easier to remove later if a driver fix or simpler workaround materializes.