Mir

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

Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: 4079
Merged at revision: 4072
Proposed branch: lp:~vanvugt/mir/schedule-nonblocking
Merge into: lp:mir
Diff against target: 254 lines (+110/-3)
11 files modified
src/server/compositor/dropping_schedule.cpp (+17/-1)
src/server/compositor/dropping_schedule.h (+2/-0)
src/server/compositor/queueing_schedule.cpp (+7/-0)
src/server/compositor/queueing_schedule.h (+2/-0)
src/server/compositor/schedule.h (+3/-0)
src/server/compositor/stream.cpp (+11/-1)
src/server/compositor/stream.h (+1/-1)
tests/unit-tests/compositor/test_dropping_schedule.cpp (+25/-0)
tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp (+6/-0)
tests/unit-tests/compositor/test_queueing_schedule.cpp (+19/-0)
tests/unit-tests/compositor/test_stream.cpp (+17/-0)
To merge this branch: bzr merge lp:~vanvugt/mir/schedule-nonblocking
Reviewer Review Type Date Requested Status
Alan Griffiths Approve
Mir CI Bot continuous-integration Approve
Review via email: mp+318879@code.launchpad.net

Commit message

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.

To post a comment you must log in.
lp:~vanvugt/mir/schedule-nonblocking updated
4079. By Daniel van Vugt

Ensure the std::future carries shared_ptr's, not raw

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

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)
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

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)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

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
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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).

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Thanks for the explanation. Makes sense.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
3232
33void mc::DroppingSchedule::schedule(std::shared_ptr<mg::Buffer> const& buffer)33void mc::DroppingSchedule::schedule(std::shared_ptr<mg::Buffer> const& buffer)
34{34{
35 auto drop = schedule_nonblocking(buffer);
36 if (drop.valid())
37 drop.wait();
38}
39
40std::future<void> mc::DroppingSchedule::schedule_nonblocking(
41 std::shared_ptr<mg::Buffer> const& buffer)
42{
43 std::future<void> drop;
35 std::lock_guard<decltype(mutex)> lk(mutex);44 std::lock_guard<decltype(mutex)> lk(mutex);
36 if ((the_only_buffer != buffer) && the_only_buffer)45 if ((the_only_buffer != buffer) && the_only_buffer)
37 sender->send_buffer(the_only_buffer->id());46 {
47 drop = std::async(std::launch::deferred,
48 [sender=sender, dropped=the_only_buffer]()
49 {
50 sender->send_buffer(dropped->id());
51 });
52 }
38 the_only_buffer = buffer;53 the_only_buffer = buffer;
54 return drop;
39}55}
4056
41unsigned int mc::DroppingSchedule::num_scheduled()57unsigned int mc::DroppingSchedule::num_scheduled()
4258
=== 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 @@
33public:33public:
34 DroppingSchedule(std::shared_ptr<frontend::ClientBuffers> const&);34 DroppingSchedule(std::shared_ptr<frontend::ClientBuffers> const&);
35 void schedule(std::shared_ptr<graphics::Buffer> const& buffer) override;35 void schedule(std::shared_ptr<graphics::Buffer> const& buffer) override;
36 std::future<void> schedule_nonblocking(
37 std::shared_ptr<graphics::Buffer> const& buffer) override;
36 unsigned int num_scheduled() override;38 unsigned int num_scheduled() override;
37 std::shared_ptr<graphics::Buffer> next_buffer() override;39 std::shared_ptr<graphics::Buffer> next_buffer() override;
3840
3941
=== 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 @@
32 queue.emplace_back(buffer);32 queue.emplace_back(buffer);
33}33}
3434
35std::future<void> mc::QueueingSchedule::schedule_nonblocking(
36 std::shared_ptr<graphics::Buffer> const& buffer)
37{
38 schedule(buffer);
39 return {};
40}
41
35unsigned int mc::QueueingSchedule::num_scheduled()42unsigned int mc::QueueingSchedule::num_scheduled()
36{43{
37 std::lock_guard<decltype(mutex)> lk(mutex);44 std::lock_guard<decltype(mutex)> lk(mutex);
3845
=== 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 @@
32{32{
33public:33public:
34 void schedule(std::shared_ptr<graphics::Buffer> const& buffer) override;34 void schedule(std::shared_ptr<graphics::Buffer> const& buffer) override;
35 std::future<void> schedule_nonblocking(
36 std::shared_ptr<graphics::Buffer> const& buffer) override;
35 unsigned int num_scheduled() override;37 unsigned int num_scheduled() override;
36 std::shared_ptr<graphics::Buffer> next_buffer() override;38 std::shared_ptr<graphics::Buffer> next_buffer() override;
3739
3840
=== 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 @@
19#define MIR_COMPOSITOR_SCHEDULE_H_19#define MIR_COMPOSITOR_SCHEDULE_H_
2020
21#include <memory>21#include <memory>
22#include <future>
2223
23namespace mir24namespace mir
24{25{
@@ -30,6 +31,8 @@
30{31{
31public:32public:
32 virtual void schedule(std::shared_ptr<graphics::Buffer> const& buffer) = 0;33 virtual void schedule(std::shared_ptr<graphics::Buffer> const& buffer) = 0;
34 virtual std::future<void> schedule_nonblocking(
35 std::shared_ptr<graphics::Buffer> const& buffer) = 0;
33 virtual unsigned int num_scheduled() = 0;36 virtual unsigned int num_scheduled() = 0;
34 virtual std::shared_ptr<graphics::Buffer> next_buffer() = 0;37 virtual std::shared_ptr<graphics::Buffer> next_buffer() = 0;
3538
3639
=== 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 @@
9090
91void mc::Stream::submit_buffer(std::shared_ptr<mg::Buffer> const& buffer)91void mc::Stream::submit_buffer(std::shared_ptr<mg::Buffer> const& buffer)
92{92{
93 std::future<void> deferred_io;
94
93 if (!buffer)95 if (!buffer)
94 BOOST_THROW_EXCEPTION(std::invalid_argument("cannot submit null buffer"));96 BOOST_THROW_EXCEPTION(std::invalid_argument("cannot submit null buffer"));
9597
@@ -97,11 +99,19 @@
97 std::lock_guard<decltype(mutex)> lk(mutex); 99 std::lock_guard<decltype(mutex)> lk(mutex);
98 first_frame_posted = true;100 first_frame_posted = true;
99 buffers->receive_buffer(buffer->id());101 buffers->receive_buffer(buffer->id());
100 schedule->schedule(buffers->get(buffer->id()));102 deferred_io = schedule->schedule_nonblocking(buffers->get(buffer->id()));
101 if (!associated_buffers.empty() && (client_owned_buffer_count(lk) == 0))103 if (!associated_buffers.empty() && (client_owned_buffer_count(lk) == 0))
102 drop_policy->swap_now_blocking();104 drop_policy->swap_now_blocking();
103 }105 }
104 observers.frame_posted(1, buffer->size());106 observers.frame_posted(1, buffer->size());
107
108 // Ensure that mutex is not locked while we do this (synchronous!) socket
109 // IO. Holding it locked blocks the compositor thread(s) from rendering.
110 if (deferred_io.valid())
111 {
112 // TODO: Throttling of GPU hogs goes here (LP: #1211700, LP: #1665802)
113 deferred_io.wait();
114 }
105}115}
106116
107void mc::Stream::with_most_recent_buffer_do(std::function<void(mg::Buffer&)> const& fn)117void mc::Stream::with_most_recent_buffer_do(std::function<void(mg::Buffer&)> const& fn)
108118
=== 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 @@
83 std::unique_ptr<compositor::FrameDroppingPolicy> drop_policy;83 std::unique_ptr<compositor::FrameDroppingPolicy> drop_policy;
84 ScheduleMode schedule_mode;84 ScheduleMode schedule_mode;
85 std::shared_ptr<Schedule> schedule;85 std::shared_ptr<Schedule> schedule;
86 std::shared_ptr<frontend::ClientBuffers> buffers;86 std::shared_ptr<frontend::ClientBuffers> const buffers;
87 std::shared_ptr<MultiMonitorArbiter> const arbiter;87 std::shared_ptr<MultiMonitorArbiter> const arbiter;
88 geometry::Size size; 88 geometry::Size size;
89 MirPixelFormat const pf;89 MirPixelFormat const pf;
9090
=== 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 @@
88 EXPECT_THAT(queue[0]->id(), Eq(buffers[4]->id()));88 EXPECT_THAT(queue[0]->id(), Eq(buffers[4]->id()));
89}89}
9090
91TEST_F(DroppingSchedule, nonblocking_schedule_avoids_socket_io)
92{
93 for (auto i = 0u; i < num_buffers; i++)
94 {
95 EXPECT_CALL(mock_client_buffers, send_buffer(_))
96 .Times(0);
97
98 auto deferred_io = schedule.schedule_nonblocking(buffers[i]);
99
100 testing::Mock::VerifyAndClearExpectations(&mock_client_buffers);
101 if (i > 0)
102 {
103 EXPECT_CALL(mock_client_buffers, send_buffer(buffers[i-1]->id()))
104 .Times(1);
105 ASSERT_TRUE(deferred_io.valid());
106 deferred_io.wait();
107 testing::Mock::VerifyAndClearExpectations(&mock_client_buffers);
108 }
109 }
110
111 auto queue = drain_queue();
112 ASSERT_THAT(queue, SizeIs(1));
113 EXPECT_THAT(queue[0]->id(), Eq(buffers[4]->id()));
114}
115
91TEST_F(DroppingSchedule, queueing_same_buffer_many_times_doesnt_drop)116TEST_F(DroppingSchedule, queueing_same_buffer_many_times_doesnt_drop)
92{117{
93 EXPECT_CALL(mock_client_buffers, send_buffer(_)).Times(0);118 EXPECT_CALL(mock_client_buffers, send_buffer(_)).Times(0);
94119
=== 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 @@
49 {49 {
50 throw std::runtime_error("this stub doesnt support this");50 throw std::runtime_error("this stub doesnt support this");
51 }51 }
52 std::future<void> schedule_nonblocking(
53 std::shared_ptr<mg::Buffer> const&) override
54 {
55 throw std::runtime_error("this stub doesnt support this");
56 return {};
57 }
52 unsigned int num_scheduled() override58 unsigned int num_scheduled() override
53 {59 {
54 return sched.size() - current;60 return sched.size() - current;
5561
=== 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 @@
75 EXPECT_FALSE(schedule.num_scheduled());75 EXPECT_FALSE(schedule.num_scheduled());
76}76}
7777
78TEST_F(QueueingSchedule, nonblocking_schedule_queues_buffers_up)
79{
80 EXPECT_EQ(0u, schedule.num_scheduled());
81
82 std::vector<std::shared_ptr<mg::Buffer>> scheduled_buffers {
83 buffers[1], buffers[3], buffers[0], buffers[2], buffers[4]
84 };
85
86 for (auto& buffer : scheduled_buffers)
87 {
88 auto deferred_io = schedule.schedule_nonblocking(buffer);
89 EXPECT_FALSE(deferred_io.valid());
90 }
91
92 EXPECT_EQ(scheduled_buffers.size(), schedule.num_scheduled());
93 EXPECT_THAT(drain_queue(), ContainerEq(scheduled_buffers));
94 EXPECT_EQ(0u, schedule.num_scheduled());
95}
96
78TEST_F(QueueingSchedule, queuing_the_same_buffer_moves_it_to_front_of_queue)97TEST_F(QueueingSchedule, queuing_the_same_buffer_moves_it_to_front_of_queue)
79{98{
80 for(auto i = 0u; i < num_buffers; i++)99 for(auto i = 0u; i < num_buffers; i++)
81100
=== 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 @@
183 stream.submit_buffer(buffers[0]);183 stream.submit_buffer(buffers[0]);
184}184}
185185
186TEST_F(Stream, wakes_compositor_before_starting_socket_io)
187{
188 auto observer = std::make_shared<MockSurfaceObserver>();
189
190 InSequence seq;
191 EXPECT_CALL(*observer, frame_posted(_,_)).Times(2);
192 EXPECT_CALL(mock_sink, send_buffer(_,_,_)).Times(1);
193
194 stream.add_observer(observer);
195 stream.allow_framedropping(true);
196 stream.submit_buffer(buffers[0]);
197 stream.submit_buffer(buffers[1]);
198 stream.remove_observer(observer);
199
200 Mock::VerifyAndClearExpectations(&mock_sink);
201}
202
186TEST_F(Stream, calls_observers_call_doesnt_hold_lock)203TEST_F(Stream, calls_observers_call_doesnt_hold_lock)
187{204{
188 auto observer = std::make_shared<MockSurfaceObserver>();205 auto observer = std::make_shared<MockSurfaceObserver>();

Subscribers

People subscribed via source and target branches