Merge lp:~alan-griffiths/mir/fix-1317370 into lp:mir

[Alan Griffiths]

tests: BufferQueueTest.compositor_never_owns_client_buffers - allow a moment for clients to acquire a buffer

tests: BufferQueueTest.compositor_never_owns_client_buffers - allow a moment for clients to acquire a buffer

The previous code led to the "compositor" thread looping millions of times whilst the client tried to acquire the client_buffer_guard mutex a couple of thousand times. By introducing a condition variable the "compositor" waits briefly for the client to acquire a buffer (so that there is something to test).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1681
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1820/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/461
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/462
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/460
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/339
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/339/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/338
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/338/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1298
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1298/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1670
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/8111

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

Yes, this cuts down the time in half in my desktop. (from 90-100ms to 43-50ms)

review: Approve

[Kevin DuBois (kdub)]

okay

review: Approve

[Alexandros Frantzis (afrantzis)]

This doesn't change the situation on my desktop (which is good to begin with, runtime < 1s), but I noticed that the unit test execution time under valgrind increased (vs previous and subsequent runs) for amd64 (https://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/340/consoleFull), but decreased significantly for armhf (https://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/338/consoleFull). So overall a net win.

I retriggered the CI just to get extra data points for the timings.

review: Approve

[Alan Griffiths (alan-griffiths)]

> This doesn't change the situation on my desktop (which is good to begin with,
> runtime < 1s),

I'm not sure why Daniel and I see this and others do not. (And I can get good timings too if I prevent the test using multiple CPUs.)

The bug mechanism is clear though: the "composite" thread releases and re-acquires the lock thousands of times before the waiting "client" thread can acquire it. This is clearly valid, but unfortunate, thread scheduling. The client thread "getting in" as soon as the lock is released by the compositor is also valid. Different kit exhibits different behavior.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1681
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1830/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/477
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/478
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/476
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/349
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/349/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/348
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/348/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1347
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1347/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1683
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/8160

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

review: Approve (continuous-integration)

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2014-06-03 02:13:59 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2014-06-05 10:46:20 +0000
@@ -1269,11 +1269,14 @@
 
 TEST_F(BufferQueueTest, compositor_never_owns_client_buffers)
 {
+    static std::chrono::nanoseconds const time_for_client_to_acquire{1};
+
     for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
     {
         mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
 
         std::mutex client_buffer_guard;
+        std::condition_variable client_buffer_cv;
         mg::Buffer* client_buffer = nullptr;
         std::atomic<bool> done(false);
 
@@ -1285,9 +1288,15 @@
                 auto buffer = q.compositor_acquire(this);
 
                 {
-                    std::lock_guard<std::mutex> lock(client_buffer_guard);
-                    if (client_buffer)
+                    std::unique_lock<std::mutex> lock(client_buffer_guard);
+
+                    if (client_buffer_cv.wait_for(
+                        lock,
+                        time_for_client_to_acquire,
+                        [&]()->bool{ return client_buffer; }))
+                    {
                         ASSERT_THAT(buffer->id(), Ne(client_buffer->id()));
+                    }
                 }
 
                 std::this_thread::yield();
@@ -1304,6 +1313,7 @@
             {
                 std::lock_guard<std::mutex> lock(client_buffer_guard);
                 client_buffer = handle->buffer();
+                client_buffer_cv.notify_one();
             }
 
             std::this_thread::yield();