Merge lp:~vanvugt/mir/simplify-BufferQueue into lp:mir

[Daniel van Vugt]

Simplify the new BufferQueue a little bit.

It seems bzr has made a mess of the diff. It might be clearer if you view the source files directly...

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1612
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1560/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/102
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/103/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/103
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/79
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/79/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/77
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/77/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/230
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/230/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1368
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6955

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Unexpectedly, this branch appears to solve bug 1317370, consistently.

[lp:~vanvugt/mir/simplify-BufferQueue]

1613. By Daniel van Vugt on 2014-05-09

Merge latest development-branch

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1613
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1573/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/119
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/120
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/120
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/92
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/92/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/90
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/90/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/259
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/259/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1383
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6984

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

Wasn't the use of std::vector for current_buffer_users driven by profiling? What is the cost of an unordered_set for this?

The simplification of client_acquire looks good to me.

It seems like the behavioural change here is not checking for !resize_buffer - what is the consequence of that? Is the fact that you can make this change without breaking tests a bug in the existing code, or a bug in the tests?

review: Needs Information

[Daniel van Vugt (vanvugt)]

Found out why this fixes bug 1317370. In this particular test case, with the compositor thread running much faster (1000x) than the client thread, the list current_buffer_users was regularly growing to several thousand elements in size on every client frame. This is obviously unnecessary and explains why this branch is in fact a /correct/ fix for the bug :)

Chris: Re:
73 - if (!resize_buffer && contains(buffer, pending_snapshots))
74 - {
75 + else if (contains(buffer, pending_snapshots))
The new context ("else") already logically guarantees !resize_buffer. It's one of those cases where a simple diff fails to show the whole story.

[Daniel van Vugt (vanvugt)]

The compositor rendering more frames than a client is producing is in fact a common occurrence as most apps are mostly idle. So in that case the growth of the current_buffer_users vector is unbounded. Regardless of any performance benefit to the average case, the worst case for a vector is clearly unacceptable (bug 1317370). We need to use a container that guarantees uniqueness of its elements.

[Chris Halse Rogers (raof)]

Or ensure uniqueness some other way, yes - buffers_sent_to_compositor could suffer the same problem, but doesn't.

[Daniel van Vugt (vanvugt)]

buffers_sent_to_compositor can't suffer the same bug because it gets pruned on every compositor_release().

[Chris Halse Rogers (raof)]

Indeed, that's my point; current_buffer_users could be likewise pruned.

Actually, now that I say that, why *isn't* it pruned? The compositor is no longer using the current buffer!

[Daniel van Vugt (vanvugt)]

Nice idea. But I just tried that and the bug persists. I think other factors are at play.

[Alan Griffiths (alan-griffiths)]

I'm sure that std::vector<> was guided by profiling. There should have been a comment to that effect to guide maintainers of the code.

I'd expect vector<> to be more efficient even if I didn't know this - and this is an area of the code we need to be careful of performance.

Even if this stops the bug 1317370 manifesting I'd like to understand better why and the performance implications of this change.

review: Needs Information

Unmerged revisions

1613. By Daniel van Vugt on 2014-05-09

Merge latest development-branch

1612. By Daniel van Vugt on 2014-05-08

Simplify more

1611. By Daniel van Vugt on 2014-05-08

More simplification

1610. By Daniel van Vugt on 2014-05-08

Simplify user recording

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2014-05-07 03:22:19 +0000
+++ src/server/compositor/buffer_queue.cpp	2014-05-09 03:30:00 +0000
@@ -135,33 +135,25 @@
 
     pending_client_notifications.push_back(std::move(complete));
 
+    mg::Buffer* buf = nullptr;
     if (!free_buffers.empty())
     {
-        auto const buffer = free_buffers.back();
+        buf = free_buffers.back();
         free_buffers.pop_back();
-        give_buffer_to_client(buffer, std::move(lock));
-        return;
     }
-
-    /* No empty buffers, attempt allocating more
-     * TODO: need a good heuristic to switch
-     * between double-buffering to n-buffering
-     */
-    int const allocated_buffers = buffers.size();
-    if (allocated_buffers < nbuffers)
+    else if (int(buffers.size()) < nbuffers)
     {
         auto const& buffer = gralloc->alloc_buffer(the_properties);
         buffers.push_back(buffer);
-        give_buffer_to_client(buffer.get(), std::move(lock));
-        return;
+        buf = buffer.get();
+    }
+    else if (frame_dropping_enabled && !ready_to_composite_queue.empty())
+    {   /* Last resort, drop oldest buffer from the ready queue */
+        buf = pop(ready_to_composite_queue);
     }
 
-    /* Last resort, drop oldest buffer from the ready queue */
-    if (frame_dropping_enabled && !ready_to_composite_queue.empty())
-    {
-        auto const buffer = pop(ready_to_composite_queue);
-        give_buffer_to_client(buffer, std::move(lock));
-    }
+    if (buf)
+        give_buffer_to_client(buf, std::move(lock));
 }
 
 void mc::BufferQueue::client_release(graphics::Buffer* released_buffer)
@@ -190,7 +182,8 @@
     std::unique_lock<decltype(guard)> lock(guard);
 
     bool const use_current_buffer =
-        should_reuse_current_buffer(user_id) ||
+        (!current_buffer_users.empty() &&
+         current_buffer_users.find(user_id) == current_buffer_users.end()) ||
         ready_to_composite_queue.empty();
 
     mg::Buffer* buffer_to_release = nullptr;
@@ -210,7 +203,7 @@
     }
 
     buffers_sent_to_compositor.push_back(current_compositor_buffer);
-    current_buffer_users.push_back(user_id);
+    current_buffer_users.insert(user_id);
 
     auto const& acquired_buffer = buffer_for(current_compositor_buffer, buffers);
     if (buffer_to_release)
@@ -327,10 +320,8 @@
         if (nbuffers == 1)
             current_compositor_buffer = buffer;
     }
-
-    /* Don't give to the client just yet if there's a pending snapshot */
-    if (!resize_buffer && contains(buffer, pending_snapshots))
-    {
+    else if (contains(buffer, pending_snapshots))
+    {   /* Don't give to the client just yet if there's a pending snapshot */
         snapshot_released.wait(lock,
             [&]{ return !contains(buffer, pending_snapshots); });
     }
@@ -348,31 +339,6 @@
     }
 }
 
-bool mc::BufferQueue::should_reuse_current_buffer(void const* user_id)
-{
-    if (!current_buffer_users.empty())
-    {
-        int size = current_buffer_users.size();
-        for (int i = 0; i < size; ++i)
-        {
-            /* The compositor calling had already acquired
-             * the latest buffer. So it should use the next
-             * buffer available and not reuse the one it already composited.
-             */
-            if (current_buffer_users[i] == user_id)
-                return false;
-        }
-        /* This is new compositor calling so share the latest buffer for
-         * multi-monitor sync
-         */
-        return true;
-    }
-    /* The id list is really only empty the very first time.
-     * False is returned so that the compositor buffer is advanced.
-     */
-    return false;
-}
-
 void mc::BufferQueue::release(
     mg::Buffer* buffer,
     std::unique_lock<std::mutex> lock)

=== modified file 'src/server/compositor/buffer_queue.h'
--- src/server/compositor/buffer_queue.h	2014-05-07 03:22:19 +0000
+++ src/server/compositor/buffer_queue.h	2014-05-09 03:30:00 +0000
@@ -24,6 +24,7 @@
 #include <condition_variable>
 #include <queue>
 #include <vector>
+#include <unordered_set>
 
 namespace mir
 {
@@ -61,7 +62,6 @@
 private:
     void give_buffer_to_client(graphics::Buffer* buffer,
         std::unique_lock<std::mutex> lock);
-    bool should_reuse_current_buffer(void const* user_id);
     void release(graphics::Buffer* buffer,
         std::unique_lock<std::mutex> lock);
 
@@ -74,7 +74,7 @@
     std::vector<graphics::Buffer*> buffers_sent_to_compositor;
     std::vector<graphics::Buffer*> pending_snapshots;
 
-    std::vector<void const*> current_buffer_users;
+    std::unordered_set<void const*> current_buffer_users;
     graphics::Buffer* current_compositor_buffer;
 
     std::deque<Callback> pending_client_notifications;