Merge lp:~vanvugt/mir/schedule-nonblocking into lp:mir

[Daniel van Vugt]

Improve concurrency of the IPC and compositor threads.

The compositor thread is presently blocked waiting for the IPC thread to
complete socket IO, which is bad. This reorders operations to avoid
that delay so the compositor thread is not hostage to IPC performance.

This is the first step on the road to fixing GPU saturation issues
(LP: #1211700, LP: #1665802) but is not yet a fix in itself.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:4078
https://mir-jenkins.ubuntu.com/job/mir-ci/3098/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4156
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4243
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4233
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4233
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4233
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4183
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4183/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4183
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4183/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4183
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4183/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4183
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4183/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4183
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4183/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4183
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4183/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3098/rebuild

review: Approve (continuous-integration)

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:4079
https://mir-jenkins.ubuntu.com/job/mir-ci/3099/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4157
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4244
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4234
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4234
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4234
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4184
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4184/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4184
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4184/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4184
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4184/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4184
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4184/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4184
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4184/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4184
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4184/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3099/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I remember there's something in this area that doesn't work as I expect: This codepath ought to be a corner case that occurs rarely, if ever. But it happens a lot.

We shouldn't be doing IPC on the composite thread at all: re-ordering the operations is just moving the deckchairs.

I haven't tried, but I don't see why this is easier than queuing the IPC to happen on the "Mir/IPC" thread.

review: Needs Information

[Daniel van Vugt (vanvugt)]

The IPC _is_ happening on the IPC thread, but that is still indirectly blocking the compositor thread due to poor mutex management...

We just have a dumb ordering of operations right now:
.. 1. Received new buffer
.. 2. Lock the Stream lock (which hangs the compositor if it is trying to redraw).
.. 3. Queue the new buffer for compositing.
** 4. Send dropped buffer back to the client (blocking socket IO).
.. 5. Unlock the Stream lock so the compositor won't hang.
.. 6. Notify the compositor that the scene has changed and that it should start redrawing.

This branch changes the order of operations:
.. 1. Received new buffer
.. 2. Lock the Stream lock (which hangs the compositor if it is trying to redraw).
.. 3. Queue the new buffer for compositing.
.. 4. Unlock the Stream lock so the compositor won't hang.
.. 5. Notify the compositor that the scene has changed and that it should start redrawing.
** 6. Send dropped buffer back to the client (blocking socket IO).

[Alan Griffiths (alan-griffiths)]

Thanks for the explanation. Makes sense.

review: Approve

=== modified file 'src/server/compositor/dropping_schedule.cpp'
--- src/server/compositor/dropping_schedule.cpp	2017-01-18 02:29:37 +0000
+++ src/server/compositor/dropping_schedule.cpp	2017-03-03 08:47:14 +0000
@@ -32,10 +32,26 @@
 
 void mc::DroppingSchedule::schedule(std::shared_ptr<mg::Buffer> const& buffer)
 {
+    auto drop = schedule_nonblocking(buffer);
+    if (drop.valid())
+        drop.wait();
+}
+
+std::future<void> mc::DroppingSchedule::schedule_nonblocking(
+    std::shared_ptr<mg::Buffer> const& buffer)
+{
+    std::future<void> drop;
     std::lock_guard<decltype(mutex)> lk(mutex);
     if ((the_only_buffer != buffer) && the_only_buffer)
-        sender->send_buffer(the_only_buffer->id());
+    {
+        drop = std::async(std::launch::deferred,
+            [sender=sender, dropped=the_only_buffer]()
+            {
+                sender->send_buffer(dropped->id());
+            });
+    }
     the_only_buffer = buffer;
+    return drop;
 }
 
 unsigned int mc::DroppingSchedule::num_scheduled()

=== modified file 'src/server/compositor/dropping_schedule.h'
--- src/server/compositor/dropping_schedule.h	2017-03-01 08:35:04 +0000
+++ src/server/compositor/dropping_schedule.h	2017-03-03 08:47:14 +0000
@@ -33,6 +33,8 @@
 public:
     DroppingSchedule(std::shared_ptr<frontend::ClientBuffers> const&);
     void schedule(std::shared_ptr<graphics::Buffer> const& buffer) override;
+    std::future<void> schedule_nonblocking(
+        std::shared_ptr<graphics::Buffer> const& buffer) override;
     unsigned int num_scheduled() override;
     std::shared_ptr<graphics::Buffer> next_buffer() override;
 

=== modified file 'src/server/compositor/queueing_schedule.cpp'
--- src/server/compositor/queueing_schedule.cpp	2017-01-18 02:29:37 +0000
+++ src/server/compositor/queueing_schedule.cpp	2017-03-03 08:47:14 +0000
@@ -32,6 +32,13 @@
     queue.emplace_back(buffer);
 }
 
+std::future<void> mc::QueueingSchedule::schedule_nonblocking(
+    std::shared_ptr<graphics::Buffer> const& buffer)
+{
+    schedule(buffer);
+    return {};
+}
+
 unsigned int mc::QueueingSchedule::num_scheduled()
 {
     std::lock_guard<decltype(mutex)> lk(mutex);

=== modified file 'src/server/compositor/queueing_schedule.h'
--- src/server/compositor/queueing_schedule.h	2017-03-01 08:35:04 +0000
+++ src/server/compositor/queueing_schedule.h	2017-03-03 08:47:14 +0000
@@ -32,6 +32,8 @@
 {
 public:
     void schedule(std::shared_ptr<graphics::Buffer> const& buffer) override;
+    std::future<void> schedule_nonblocking(
+        std::shared_ptr<graphics::Buffer> const& buffer) override;
     unsigned int num_scheduled() override;
     std::shared_ptr<graphics::Buffer> next_buffer() override;
 

=== modified file 'src/server/compositor/schedule.h'
--- src/server/compositor/schedule.h	2016-01-29 08:18:22 +0000
+++ src/server/compositor/schedule.h	2017-03-03 08:47:14 +0000
@@ -19,6 +19,7 @@
 #define MIR_COMPOSITOR_SCHEDULE_H_
 
 #include <memory>
+#include <future>
 
 namespace mir
 {
@@ -30,6 +31,8 @@
 {
 public:
     virtual void schedule(std::shared_ptr<graphics::Buffer> const& buffer) = 0;
+    virtual std::future<void> schedule_nonblocking(
+        std::shared_ptr<graphics::Buffer> const& buffer) = 0;
     virtual unsigned int num_scheduled() = 0;
     virtual std::shared_ptr<graphics::Buffer> next_buffer() = 0;
 

=== modified file 'src/server/compositor/stream.cpp'
--- src/server/compositor/stream.cpp	2017-03-02 11:08:16 +0000
+++ src/server/compositor/stream.cpp	2017-03-03 08:47:14 +0000
@@ -90,6 +90,8 @@
 
 void mc::Stream::submit_buffer(std::shared_ptr<mg::Buffer> const& buffer)
 {
+    std::future<void> deferred_io;
+
     if (!buffer)
         BOOST_THROW_EXCEPTION(std::invalid_argument("cannot submit null buffer"));
 
@@ -97,11 +99,19 @@
         std::lock_guard<decltype(mutex)> lk(mutex); 
         first_frame_posted = true;
         buffers->receive_buffer(buffer->id());
-        schedule->schedule(buffers->get(buffer->id()));
+        deferred_io = schedule->schedule_nonblocking(buffers->get(buffer->id()));
         if (!associated_buffers.empty() && (client_owned_buffer_count(lk) == 0))
             drop_policy->swap_now_blocking();
     }
     observers.frame_posted(1, buffer->size());
+
+    // Ensure that mutex is not locked while we do this (synchronous!) socket
+    // IO. Holding it locked blocks the compositor thread(s) from rendering.
+    if (deferred_io.valid())
+    {
+        // TODO: Throttling of GPU hogs goes here (LP: #1211700, LP: #1665802)
+        deferred_io.wait();
+    }
 }
 
 void mc::Stream::with_most_recent_buffer_do(std::function<void(mg::Buffer&)> const& fn)

=== modified file 'src/server/compositor/stream.h'
--- src/server/compositor/stream.h	2017-02-21 21:43:12 +0000
+++ src/server/compositor/stream.h	2017-03-03 08:47:14 +0000
@@ -83,7 +83,7 @@
     std::unique_ptr<compositor::FrameDroppingPolicy> drop_policy;
     ScheduleMode schedule_mode;
     std::shared_ptr<Schedule> schedule;
-    std::shared_ptr<frontend::ClientBuffers> buffers;
+    std::shared_ptr<frontend::ClientBuffers> const buffers;
     std::shared_ptr<MultiMonitorArbiter> const arbiter;
     geometry::Size size; 
     MirPixelFormat const pf;

=== modified file 'tests/unit-tests/compositor/test_dropping_schedule.cpp'
--- tests/unit-tests/compositor/test_dropping_schedule.cpp	2017-03-02 11:08:16 +0000
+++ tests/unit-tests/compositor/test_dropping_schedule.cpp	2017-03-03 08:47:14 +0000
@@ -88,6 +88,31 @@
     EXPECT_THAT(queue[0]->id(), Eq(buffers[4]->id()));
 }
 
+TEST_F(DroppingSchedule, nonblocking_schedule_avoids_socket_io)
+{
+    for (auto i = 0u; i < num_buffers; i++)
+    {
+        EXPECT_CALL(mock_client_buffers, send_buffer(_))
+            .Times(0);
+
+        auto deferred_io = schedule.schedule_nonblocking(buffers[i]);
+
+        testing::Mock::VerifyAndClearExpectations(&mock_client_buffers);
+        if (i > 0)
+        {
+            EXPECT_CALL(mock_client_buffers, send_buffer(buffers[i-1]->id()))
+                .Times(1);
+            ASSERT_TRUE(deferred_io.valid());
+            deferred_io.wait();
+            testing::Mock::VerifyAndClearExpectations(&mock_client_buffers);
+        }
+    }
+
+    auto queue = drain_queue();
+    ASSERT_THAT(queue, SizeIs(1));
+    EXPECT_THAT(queue[0]->id(), Eq(buffers[4]->id()));
+}
+
 TEST_F(DroppingSchedule, queueing_same_buffer_many_times_doesnt_drop)
 {
     EXPECT_CALL(mock_client_buffers, send_buffer(_)).Times(0);

=== modified file 'tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp'
--- tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp	2017-03-03 04:42:24 +0000
+++ tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp	2017-03-03 08:47:14 +0000
@@ -49,6 +49,12 @@
     {
         throw std::runtime_error("this stub doesnt support this");
     }
+    std::future<void> schedule_nonblocking(
+        std::shared_ptr<mg::Buffer> const&) override
+    {
+        throw std::runtime_error("this stub doesnt support this");
+        return {};
+    }
     unsigned int num_scheduled() override
     {
         return sched.size() - current;

=== modified file 'tests/unit-tests/compositor/test_queueing_schedule.cpp'
--- tests/unit-tests/compositor/test_queueing_schedule.cpp	2016-01-29 08:18:22 +0000
+++ tests/unit-tests/compositor/test_queueing_schedule.cpp	2017-03-03 08:47:14 +0000
@@ -75,6 +75,25 @@
     EXPECT_FALSE(schedule.num_scheduled());
 }
 
+TEST_F(QueueingSchedule, nonblocking_schedule_queues_buffers_up)
+{
+    EXPECT_EQ(0u, schedule.num_scheduled());
+
+    std::vector<std::shared_ptr<mg::Buffer>> scheduled_buffers {
+        buffers[1], buffers[3], buffers[0], buffers[2], buffers[4]
+    };
+
+    for (auto& buffer : scheduled_buffers)
+    {
+        auto deferred_io = schedule.schedule_nonblocking(buffer);
+        EXPECT_FALSE(deferred_io.valid());
+    }
+
+    EXPECT_EQ(scheduled_buffers.size(), schedule.num_scheduled());
+    EXPECT_THAT(drain_queue(), ContainerEq(scheduled_buffers));
+    EXPECT_EQ(0u, schedule.num_scheduled());
+}
+
 TEST_F(QueueingSchedule, queuing_the_same_buffer_moves_it_to_front_of_queue)
 {
     for(auto i = 0u; i < num_buffers; i++)

=== modified file 'tests/unit-tests/compositor/test_stream.cpp'
--- tests/unit-tests/compositor/test_stream.cpp	2017-03-02 11:08:16 +0000
+++ tests/unit-tests/compositor/test_stream.cpp	2017-03-03 08:47:14 +0000
@@ -183,6 +183,23 @@
     stream.submit_buffer(buffers[0]);
 }
 
+TEST_F(Stream, wakes_compositor_before_starting_socket_io)
+{
+    auto observer = std::make_shared<MockSurfaceObserver>();
+
+    InSequence seq;
+    EXPECT_CALL(*observer, frame_posted(_,_)).Times(2);
+    EXPECT_CALL(mock_sink, send_buffer(_,_,_)).Times(1);
+
+    stream.add_observer(observer);
+    stream.allow_framedropping(true);
+    stream.submit_buffer(buffers[0]);
+    stream.submit_buffer(buffers[1]);
+    stream.remove_observer(observer);
+
+    Mock::VerifyAndClearExpectations(&mock_sink);
+}
+
 TEST_F(Stream, calls_observers_call_doesnt_hold_lock)
 {
     auto observer = std::make_shared<MockSurfaceObserver>();