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
1=== modified file 'src/server/compositor/dropping_schedule.cpp'
2--- src/server/compositor/dropping_schedule.cpp 2017-01-18 02:29:37 +0000
3+++ src/server/compositor/dropping_schedule.cpp 2017-03-03 08:47:14 +0000
4@@ -32,10 +32,26 @@
5
6 void mc::DroppingSchedule::schedule(std::shared_ptr<mg::Buffer> const& buffer)
7 {
8+ auto drop = schedule_nonblocking(buffer);
9+ if (drop.valid())
10+ drop.wait();
11+}
12+
13+std::future<void> mc::DroppingSchedule::schedule_nonblocking(
14+ std::shared_ptr<mg::Buffer> const& buffer)
15+{
16+ std::future<void> drop;
17 std::lock_guard<decltype(mutex)> lk(mutex);
18 if ((the_only_buffer != buffer) && the_only_buffer)
19- sender->send_buffer(the_only_buffer->id());
20+ {
21+ drop = std::async(std::launch::deferred,
22+ [sender=sender, dropped=the_only_buffer]()
23+ {
24+ sender->send_buffer(dropped->id());
25+ });
26+ }
27 the_only_buffer = buffer;
28+ return drop;
29 }
30
31 unsigned int mc::DroppingSchedule::num_scheduled()
32
33=== modified file 'src/server/compositor/dropping_schedule.h'
34--- src/server/compositor/dropping_schedule.h 2017-03-01 08:35:04 +0000
35+++ src/server/compositor/dropping_schedule.h 2017-03-03 08:47:14 +0000
36@@ -33,6 +33,8 @@
37 public:
38 DroppingSchedule(std::shared_ptr<frontend::ClientBuffers> const&);
39 void schedule(std::shared_ptr<graphics::Buffer> const& buffer) override;
40+ std::future<void> schedule_nonblocking(
41+ std::shared_ptr<graphics::Buffer> const& buffer) override;
42 unsigned int num_scheduled() override;
43 std::shared_ptr<graphics::Buffer> next_buffer() override;
44
45
46=== modified file 'src/server/compositor/queueing_schedule.cpp'
47--- src/server/compositor/queueing_schedule.cpp 2017-01-18 02:29:37 +0000
48+++ src/server/compositor/queueing_schedule.cpp 2017-03-03 08:47:14 +0000
49@@ -32,6 +32,13 @@
50 queue.emplace_back(buffer);
51 }
52
53+std::future<void> mc::QueueingSchedule::schedule_nonblocking(
54+ std::shared_ptr<graphics::Buffer> const& buffer)
55+{
56+ schedule(buffer);
57+ return {};
58+}
59+
60 unsigned int mc::QueueingSchedule::num_scheduled()
61 {
62 std::lock_guard<decltype(mutex)> lk(mutex);
63
64=== modified file 'src/server/compositor/queueing_schedule.h'
65--- src/server/compositor/queueing_schedule.h 2017-03-01 08:35:04 +0000
66+++ src/server/compositor/queueing_schedule.h 2017-03-03 08:47:14 +0000
67@@ -32,6 +32,8 @@
68 {
69 public:
70 void schedule(std::shared_ptr<graphics::Buffer> const& buffer) override;
71+ std::future<void> schedule_nonblocking(
72+ std::shared_ptr<graphics::Buffer> const& buffer) override;
73 unsigned int num_scheduled() override;
74 std::shared_ptr<graphics::Buffer> next_buffer() override;
75
76
77=== modified file 'src/server/compositor/schedule.h'
78--- src/server/compositor/schedule.h 2016-01-29 08:18:22 +0000
79+++ src/server/compositor/schedule.h 2017-03-03 08:47:14 +0000
80@@ -19,6 +19,7 @@
81 #define MIR_COMPOSITOR_SCHEDULE_H_
82
83 #include <memory>
84+#include <future>
85
86 namespace mir
87 {
88@@ -30,6 +31,8 @@
89 {
90 public:
91 virtual void schedule(std::shared_ptr<graphics::Buffer> const& buffer) = 0;
92+ virtual std::future<void> schedule_nonblocking(
93+ std::shared_ptr<graphics::Buffer> const& buffer) = 0;
94 virtual unsigned int num_scheduled() = 0;
95 virtual std::shared_ptr<graphics::Buffer> next_buffer() = 0;
96
97
98=== modified file 'src/server/compositor/stream.cpp'
99--- src/server/compositor/stream.cpp 2017-03-02 11:08:16 +0000
100+++ src/server/compositor/stream.cpp 2017-03-03 08:47:14 +0000
101@@ -90,6 +90,8 @@
102
103 void mc::Stream::submit_buffer(std::shared_ptr<mg::Buffer> const& buffer)
104 {
105+ std::future<void> deferred_io;
106+
107 if (!buffer)
108 BOOST_THROW_EXCEPTION(std::invalid_argument("cannot submit null buffer"));
109
110@@ -97,11 +99,19 @@
111 std::lock_guard<decltype(mutex)> lk(mutex);
112 first_frame_posted = true;
113 buffers->receive_buffer(buffer->id());
114- schedule->schedule(buffers->get(buffer->id()));
115+ deferred_io = schedule->schedule_nonblocking(buffers->get(buffer->id()));
116 if (!associated_buffers.empty() && (client_owned_buffer_count(lk) == 0))
117 drop_policy->swap_now_blocking();
118 }
119 observers.frame_posted(1, buffer->size());
120+
121+ // Ensure that mutex is not locked while we do this (synchronous!) socket
122+ // IO. Holding it locked blocks the compositor thread(s) from rendering.
123+ if (deferred_io.valid())
124+ {
125+ // TODO: Throttling of GPU hogs goes here (LP: #1211700, LP: #1665802)
126+ deferred_io.wait();
127+ }
128 }
129
130 void mc::Stream::with_most_recent_buffer_do(std::function<void(mg::Buffer&)> const& fn)
131
132=== modified file 'src/server/compositor/stream.h'
133--- src/server/compositor/stream.h 2017-02-21 21:43:12 +0000
134+++ src/server/compositor/stream.h 2017-03-03 08:47:14 +0000
135@@ -83,7 +83,7 @@
136 std::unique_ptr<compositor::FrameDroppingPolicy> drop_policy;
137 ScheduleMode schedule_mode;
138 std::shared_ptr<Schedule> schedule;
139- std::shared_ptr<frontend::ClientBuffers> buffers;
140+ std::shared_ptr<frontend::ClientBuffers> const buffers;
141 std::shared_ptr<MultiMonitorArbiter> const arbiter;
142 geometry::Size size;
143 MirPixelFormat const pf;
144
145=== modified file 'tests/unit-tests/compositor/test_dropping_schedule.cpp'
146--- tests/unit-tests/compositor/test_dropping_schedule.cpp 2017-03-02 11:08:16 +0000
147+++ tests/unit-tests/compositor/test_dropping_schedule.cpp 2017-03-03 08:47:14 +0000
148@@ -88,6 +88,31 @@
149 EXPECT_THAT(queue[0]->id(), Eq(buffers[4]->id()));
150 }
151
152+TEST_F(DroppingSchedule, nonblocking_schedule_avoids_socket_io)
153+{
154+ for (auto i = 0u; i < num_buffers; i++)
155+ {
156+ EXPECT_CALL(mock_client_buffers, send_buffer(_))
157+ .Times(0);
158+
159+ auto deferred_io = schedule.schedule_nonblocking(buffers[i]);
160+
161+ testing::Mock::VerifyAndClearExpectations(&mock_client_buffers);
162+ if (i > 0)
163+ {
164+ EXPECT_CALL(mock_client_buffers, send_buffer(buffers[i-1]->id()))
165+ .Times(1);
166+ ASSERT_TRUE(deferred_io.valid());
167+ deferred_io.wait();
168+ testing::Mock::VerifyAndClearExpectations(&mock_client_buffers);
169+ }
170+ }
171+
172+ auto queue = drain_queue();
173+ ASSERT_THAT(queue, SizeIs(1));
174+ EXPECT_THAT(queue[0]->id(), Eq(buffers[4]->id()));
175+}
176+
177 TEST_F(DroppingSchedule, queueing_same_buffer_many_times_doesnt_drop)
178 {
179 EXPECT_CALL(mock_client_buffers, send_buffer(_)).Times(0);
180
181=== modified file 'tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp'
182--- tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp 2017-03-03 04:42:24 +0000
183+++ tests/unit-tests/compositor/test_multi_monitor_arbiter.cpp 2017-03-03 08:47:14 +0000
184@@ -49,6 +49,12 @@
185 {
186 throw std::runtime_error("this stub doesnt support this");
187 }
188+ std::future<void> schedule_nonblocking(
189+ std::shared_ptr<mg::Buffer> const&) override
190+ {
191+ throw std::runtime_error("this stub doesnt support this");
192+ return {};
193+ }
194 unsigned int num_scheduled() override
195 {
196 return sched.size() - current;
197
198=== modified file 'tests/unit-tests/compositor/test_queueing_schedule.cpp'
199--- tests/unit-tests/compositor/test_queueing_schedule.cpp 2016-01-29 08:18:22 +0000
200+++ tests/unit-tests/compositor/test_queueing_schedule.cpp 2017-03-03 08:47:14 +0000
201@@ -75,6 +75,25 @@
202 EXPECT_FALSE(schedule.num_scheduled());
203 }
204
205+TEST_F(QueueingSchedule, nonblocking_schedule_queues_buffers_up)
206+{
207+ EXPECT_EQ(0u, schedule.num_scheduled());
208+
209+ std::vector<std::shared_ptr<mg::Buffer>> scheduled_buffers {
210+ buffers[1], buffers[3], buffers[0], buffers[2], buffers[4]
211+ };
212+
213+ for (auto& buffer : scheduled_buffers)
214+ {
215+ auto deferred_io = schedule.schedule_nonblocking(buffer);
216+ EXPECT_FALSE(deferred_io.valid());
217+ }
218+
219+ EXPECT_EQ(scheduled_buffers.size(), schedule.num_scheduled());
220+ EXPECT_THAT(drain_queue(), ContainerEq(scheduled_buffers));
221+ EXPECT_EQ(0u, schedule.num_scheduled());
222+}
223+
224 TEST_F(QueueingSchedule, queuing_the_same_buffer_moves_it_to_front_of_queue)
225 {
226 for(auto i = 0u; i < num_buffers; i++)
227
228=== modified file 'tests/unit-tests/compositor/test_stream.cpp'
229--- tests/unit-tests/compositor/test_stream.cpp 2017-03-02 11:08:16 +0000
230+++ tests/unit-tests/compositor/test_stream.cpp 2017-03-03 08:47:14 +0000
231@@ -183,6 +183,23 @@
232 stream.submit_buffer(buffers[0]);
233 }
234
235+TEST_F(Stream, wakes_compositor_before_starting_socket_io)
236+{
237+ auto observer = std::make_shared<MockSurfaceObserver>();
238+
239+ InSequence seq;
240+ EXPECT_CALL(*observer, frame_posted(_,_)).Times(2);
241+ EXPECT_CALL(mock_sink, send_buffer(_,_,_)).Times(1);
242+
243+ stream.add_observer(observer);
244+ stream.allow_framedropping(true);
245+ stream.submit_buffer(buffers[0]);
246+ stream.submit_buffer(buffers[1]);
247+ stream.remove_observer(observer);
248+
249+ Mock::VerifyAndClearExpectations(&mock_sink);
250+}
251+
252 TEST_F(Stream, calls_observers_call_doesnt_hold_lock)
253 {
254 auto observer = std::make_shared<MockSurfaceObserver>();

Subscribers

People subscribed via source and target branches