Mir

Merge lp:~vanvugt/mir/disable-early-release into lp:mir

disable-early-release
Merge into development-branch

Proposed by Daniel van Vugt on 2016-03-30

Status:

Merged

Approved by:

Daniel van Vugt on 2016-04-06

Approved revision:

no longer in the source branch.

Merged at revision:

3441

Proposed branch:

lp:~vanvugt/mir/disable-early-release

Merge into:

lp:mir

Diff against target:

277 lines (+83/-13)

10 files modified

src/server/compositor/buffer_bundle.h (+8/-0)
src/server/compositor/buffer_queue.cpp (+10/-1)
src/server/compositor/buffer_queue.h (+2/-0)
src/server/compositor/multi_monitor_arbiter.h (+1/-7)
tests/acceptance-tests/test_latency.cpp (+1/-5)
tests/include/mir/test/doubles/mock_buffer_bundle.h (+1/-0)
tests/integration-tests/test_buffer_scheduling.cpp (+27/-0)
tests/integration-tests/test_exchange_buffer.cpp (+1/-0)
tests/unit-tests/compositor/test_buffer_queue.cpp (+31/-0)
tests/unit-tests/compositor/test_temporary_buffers.cpp (+1/-0)

To merge this branch:

bzr merge lp:~vanvugt/mir/disable-early-release

Low

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Cemil Azizoglu (community)			Approve on 2016-04-05
Kevin DuBois (community)		2016-03-30	Approve on 2016-04-05
Mir CI Bot	continuous-integration		Approve on 2016-04-01
Review via email: mp+290414@code.launchpad.net

Commit message

Disable the 'early release optimization' in BufferQueue, and add more
tests to verify it's really disabled (and the equivalent in NBS too).

The purpose of 'early release' (AKA single_monitor_fast in NBS) was to
reduce the chance of frame skipping when the number of buffers is reduced.
However now it seems unlikely we will ever reduce the number of buffers
to two.

'early release' has the side-effect of increasing buffer lag by one
frame. And that would have been canceled out by switching to double
buffering, but now it won't be. So in terms of latency we're one frame
better off not using 'early release'.

NBS's equivalent mode 'single_monitor_fast' is already disabled by
default (LP: #1561418), so no change required there.

The new 'mirvanity' tool also confirms that this branch measurably reduces
latency by about 17ms (one frame).

That all said, Unity8 was never affected by the 'early release' lag
because (a) its QtMir compositor does not early-release; and (b) the
system compositor generally uses bypass/overlays that prevents the
possibility of early release anyway. But at least now BufferQueue and
NBS have the same default mode, and it is the lowest latency mode.

The branch incidentally also provides a basis for fixing LP: #1561418.

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-30:

When it was introduced in the nbs code, MultMonitorMode was criticized for not being very descriptive about what the benefits/tradeoffs are. If we don't need the mode anymore, could it just be removed? Since it still remains an internal interface (and is not exposed over protocol or command line options), that's a non-blocking concern.

>The purpose of 'early release' (AKA single_monitor_fast in NBS) was to
>reduce the chance of frame skipping when the number of buffers is reduced.
>However now it seems unlikely we will ever reduce the number of buffers
>to two.

The other thought I had was that we have info about reducing latency, but no info on the effect on throughput. The other thing 'early release' does is return the buffer to the client sooner so it can start to work on it sooner.

If its a good throughput/latency tradeoff, +1, but I guess needs info for now.

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-31:

Yes I remember asking you to go round in circles coming up with the name MultiMonitorMode (sorry). It's still not ideal, but I intentionally want to separate renaming discussions from this just being a minor fix.

As for deleting the setting completely, I did consider that but recommend against it right now because:
* The diff would be much larger, and require changes to NBS; and
* It's a stable well-tested feature we might want to re-enable at some point in future.

Normally the argument is to remove unused code because it can always be restored easily. But that's not true for buffering-related features because we tend to break things enough that not having it permanently under test makes it very difficult to work back in in future (think of double buffering support which is in the same boat and I've progressively had to reinforce the test suite as we kept breaking it while it was unused). So I'd rather keep the feature and its tests for now.

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-31:

> Yes I remember asking you to go round in circles coming up with the name
> MultiMonitorMode (sorry). It's still not ideal, but I intentionally want to
> separate renaming discussions from this just being a minor fix.

I should have said it was "rightly criticized". It still is a unintuitive and needs some comment-reading to figure out what the mode is doing. I probably would object if it leaked out to the command line options, server API or client API, but as its still internal, I don't mind its internal scope increase.

> As for deleting the setting completely, I did consider that but recommend
> against it right now because:
> * The diff would be much larger, and require changes to NBS; and
> * It's a stable well-tested feature we might want to re-enable at some point
> in future.
>
> Normally the argument is to remove unused code because it can always be
> restored easily. But that's not true for buffering-related features because we
> tend to break things enough that not having it permanently under test makes it
> very difficult to work back in in future (think of double buffering support
> which is in the same boat and I've progressively had to reinforce the test
> suite as we kept breaking it while it was unused). So I'd rather keep the
> feature and its tests for now.

Fair enough, although eventually, I think things should be removed just so we don't get bogged down by 'features' that we don't need.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-31:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-04-01:

I do have some better naming ideas on this subject, but won't raise them here. Keeping the same name/type makes this a babystep and nice small diff.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-04-01:

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

review: Approve (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-04-01:

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

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-04-01:

The comment starting this line will need to be fixed, as well.

55 * The "early release" optimisation: Note "single_compositor" above

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-04-04:

^ That comment in buffer_queue.cpp is correct and does not need changing.

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-04-05:

I poked around The throughput with the demo servers, so appreciable difference.

Seemingly, set_mode is unused (so, we're probably not going to need it).
As long as the name gets improved before any more "scope elevation" (becoming present in an option/environment variable, ipc, server API), then I'm okay with the change, +1

review: Approve

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-04-05:

Huh I should have my eyes checked.... Sorry about that.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Daniel van Vugt

Emanuele Antonio Faraone

Gerry Boland

Mir development team

1	=== modified file 'src/server/compositor/buffer_bundle.h'
2	--- src/server/compositor/buffer_bundle.h 2016-01-29 08:18:22 +0000
3	+++ src/server/compositor/buffer_bundle.h 2016-04-01 09:06:21 +0000
4	@@ -30,6 +30,12 @@
5	namespace compositor
6	{
7
8	+enum class MultiMonitorMode
9	+{
10	+ multi_monitor_sync, // lower latency+framerate, and supports multi-monitor
11	+ single_monitor_fast // higher latency+framerate, no multi-monitor
12	+};
13	+
14	class BufferAcquisition
15	{
16	public:
17	@@ -49,6 +55,7 @@
18	virtual void compositor_release(std::shared_ptr<graphics::Buffer> const&) = 0;
19	virtual std::shared_ptr<graphics::Buffer> snapshot_acquire() = 0;
20	virtual void snapshot_release(std::shared_ptr<graphics::Buffer> const&) = 0;
21	+ virtual void set_mode(MultiMonitorMode mode) = 0;
22	virtual ~BufferAcquisition() = default;
23
24	protected:
25	@@ -66,6 +73,7 @@
26
27	virtual graphics::BufferProperties properties() const = 0;
28	virtual void allow_framedropping(bool dropping_allowed) = 0;
29	+ virtual void set_mode(MultiMonitorMode mode) = 0;
30	virtual void force_requests_to_complete() = 0;
31	virtual void resize(const geometry::Size &newsize) = 0;
32	virtual int buffers_ready_for_compositor(void const* user_id) const = 0;
33
34	=== modified file 'src/server/compositor/buffer_queue.cpp'
35	--- src/server/compositor/buffer_queue.cpp 2016-03-29 11:48:11 +0000
36	+++ src/server/compositor/buffer_queue.cpp 2016-04-01 09:06:21 +0000
37	@@ -140,6 +140,7 @@
38	frame_deadlines_threshold{-1}, // Disable scaling by default
39	frame_deadlines_met{0},
40	scheduled_extra_frames{0},
41	+ mm_mode{MultiMonitorMode::multi_monitor_sync},
42	frame_dropping_enabled{false},
43	current_compositor_buffer_valid{false},
44	the_properties{props},
45	@@ -379,7 +380,9 @@
46
47	if (current_compositor_buffer != buffer.get())
48	release(buffer.get(), std::move(lock));
49	- else if (!ready_to_composite_queue.empty() && single_compositor)
50	+ else if (mm_mode == MultiMonitorMode::single_monitor_fast &&
51	+ !ready_to_composite_queue.empty() &&
52	+ single_compositor)
53	{
54	/*
55	* The "early release" optimisation: Note "single_compositor" above
56	@@ -426,6 +429,12 @@
57	return the_properties;
58	}
59
60	+void mc::BufferQueue::set_mode(MultiMonitorMode mode)
61	+{
62	+ std::lock_guard<decltype(guard)> lock(guard);
63	+ mm_mode = mode;
64	+}
65	+
66	void mc::BufferQueue::allow_framedropping(bool flag)
67	{
68	std::lock_guard<decltype(guard)> lock(guard);
69
70	=== modified file 'src/server/compositor/buffer_queue.h'
71	--- src/server/compositor/buffer_queue.h 2016-03-29 11:48:11 +0000
72	+++ src/server/compositor/buffer_queue.h 2016-04-01 09:06:21 +0000
73	@@ -56,6 +56,7 @@
74
75	graphics::BufferProperties properties() const override;
76	void allow_framedropping(bool dropping_allowed) override;
77	+ void set_mode(MultiMonitorMode mode) override;
78	void force_requests_to_complete() override;
79	void resize(const geometry::Size &newsize) override;
80	int buffers_ready_for_compositor(void const* user_id) const override;
81	@@ -110,6 +111,7 @@
82	int frame_deadlines_threshold;
83	int frame_deadlines_met;
84	int scheduled_extra_frames;
85	+ MultiMonitorMode mm_mode;
86	bool frame_dropping_enabled;
87	bool current_compositor_buffer_valid;
88	graphics::BufferProperties the_properties;
89
90	=== modified file 'src/server/compositor/multi_monitor_arbiter.h'
91	--- src/server/compositor/multi_monitor_arbiter.h 2016-01-29 08:18:22 +0000
92	+++ src/server/compositor/multi_monitor_arbiter.h 2016-04-01 09:06:21 +0000
93	@@ -34,12 +34,6 @@
94	{
95	class Schedule;
96
97	-enum class MultiMonitorMode
98	-{
99	- multi_monitor_sync,
100	- single_monitor_fast
101	-};
102	-
103	class MultiMonitorArbiter : public BufferAcquisition
104	{
105	public:
106	@@ -53,7 +47,7 @@
107	std::shared_ptr<graphics::Buffer> snapshot_acquire() override;
108	void snapshot_release(std::shared_ptr<graphics::Buffer> const&) override;
109	void set_schedule(std::shared_ptr<Schedule> const& schedule);
110	- void set_mode(MultiMonitorMode mode);
111	+ void set_mode(MultiMonitorMode mode) override;
112	bool buffer_ready_for(compositor::CompositorID id);
113	bool has_buffer();
114	void advance_schedule();
115
116	=== modified file 'tests/acceptance-tests/test_latency.cpp'
117	--- tests/acceptance-tests/test_latency.cpp 2016-03-23 06:39:56 +0000
118	+++ tests/acceptance-tests/test_latency.cpp 2016-04-01 09:06:21 +0000
119	@@ -286,11 +286,7 @@
120	ASSERT_TRUE(stats.wait_for_posts(test_submissions,
121	std::chrono::seconds(60)));
122
123	- // Note: Using the "early release" optimization without dynamic queue
124	- // scaling enabled makes the expected latency possibly up to
125	- // nbuffers instead of nbuffers-1. After dynamic queue scaling is
126	- // enabled, the average will be lower than this.
127	- float const expected_max_latency = expected_client_buffers;
128	+ float const expected_max_latency = expected_client_buffers - 1;
129
130	if (server.get_options()->get<bool>(mtd::logging_opt))
131	display.group.dump_latency();
132
133	=== modified file 'tests/include/mir/test/doubles/mock_buffer_bundle.h'
134	--- tests/include/mir/test/doubles/mock_buffer_bundle.h 2016-01-29 08:18:22 +0000
135	+++ tests/include/mir/test/doubles/mock_buffer_bundle.h 2016-04-01 09:06:21 +0000
136	@@ -49,6 +49,7 @@
137	MOCK_METHOD0(snapshot_acquire, std::shared_ptr<graphics::Buffer>());
138	MOCK_METHOD1(snapshot_release, void(std::shared_ptr<graphics::Buffer> const&));
139	MOCK_METHOD1(allow_framedropping, void(bool));
140	+ MOCK_METHOD1(set_mode, void(compositor::MultiMonitorMode));
141	MOCK_CONST_METHOD0(properties, graphics::BufferProperties());
142	MOCK_METHOD0(force_client_abort, void());
143	MOCK_METHOD0(force_requests_to_complete, void());
144
145	=== modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
146	--- tests/integration-tests/test_buffer_scheduling.cpp 2016-03-23 06:39:56 +0000
147	+++ tests/integration-tests/test_buffer_scheduling.cpp 2016-04-01 09:06:21 +0000
148	@@ -899,6 +899,31 @@
149	EXPECT_THAT(production_log[nbuffers + 1].blockage, Eq(Access::unblocked));
150	}
151
152	+TEST_P(WithTwoOrMoreBuffers, client_is_not_woken_by_compositor_release)
153	+{
154	+ // If early release is accidentally active, make sure we see it. But it
155	+ // requires a dummy frame first:
156	+ producer->produce();
157	+ auto onscreen = stream->lock_compositor_buffer(this);
158	+ onscreen.reset();
159	+
160	+ while (producer->can_produce())
161	+ producer->produce();
162	+
163	+ ASSERT_FALSE(producer->can_produce());
164	+ onscreen = stream->lock_compositor_buffer(this);
165	+
166	+ // This varies between NBS and BufferQueue. Should it?
167	+ if (producer->can_produce())
168	+ producer->produce();
169	+ ASSERT_FALSE(producer->can_produce());
170	+
171	+ onscreen.reset();
172	+ // single_monitor_fast -> can produce here
173	+ // multi_monitor_sync -> can't produce here
174	+ ASSERT_FALSE(producer->can_produce());
175	+}
176	+
177	// Regression test for LP: #1319765
178	TEST_P(WithTwoBuffers, client_is_not_blocked_prematurely)
179	{
180	@@ -1017,6 +1042,7 @@
181	mtd::MockFrameDroppingPolicyFactory policy_factory;
182	mc::BufferQueue queue{nbuffers, mt::fake_shared(server_buffer_factory),
183	properties, policy_factory};
184	+ queue.set_mode(mc::MultiMonitorMode::single_monitor_fast);
185	queue.allow_framedropping(false);
186
187	mg::Buffer* client_buffer = nullptr;
188	@@ -1083,6 +1109,7 @@
189	mtd::MockFrameDroppingPolicyFactory policy_factory;
190	mc::BufferQueue queue{nbuffers, mt::fake_shared(server_buffer_factory),
191	properties, policy_factory};
192	+ queue.set_mode(mc::MultiMonitorMode::single_monitor_fast);
193	queue.allow_framedropping(false);
194
195	const void* const leftid = "left";
196
197	=== modified file 'tests/integration-tests/test_exchange_buffer.cpp'
198	--- tests/integration-tests/test_exchange_buffer.cpp 2016-01-29 08:18:22 +0000
199	+++ tests/integration-tests/test_exchange_buffer.cpp 2016-04-01 09:06:21 +0000
200	@@ -90,6 +90,7 @@
201	void snapshot_release(std::shared_ptr<mg::Buffer> const&) {}
202	mg::BufferProperties properties() const { return mg::BufferProperties{}; }
203	void allow_framedropping(bool) {}
204	+ void set_mode(mc::MultiMonitorMode) override {}
205	void force_requests_to_complete() {}
206	void resize(const geom::Size&) {}
207	int buffers_ready_for_compositor(void const*) const { return 1; }
208
209	=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
210	--- tests/unit-tests/compositor/test_buffer_queue.cpp 2016-03-23 06:39:56 +0000
211	+++ tests/unit-tests/compositor/test_buffer_queue.cpp 2016-04-01 09:06:21 +0000
212	@@ -477,8 +477,37 @@
213	EXPECT_NO_THROW(q.compositor_release(comp_buffer));
214	}
215
216	+TEST_P(WithTwoOrMoreBuffers, clients_dont_get_new_buffers_on_compositor_release)
217	+{
218	+ q.set_mode(mc::MultiMonitorMode::multi_monitor_sync);
219	+ q.allow_framedropping(false);
220	+
221	+ // This is what tests should do instead of using buffers_free_for_client()
222	+ std::shared_ptr<AcquireWaitHandle> client;
223	+ bool blocking;
224	+ do
225	+ {
226	+ client = client_acquire_async(q);
227	+ blocking = !client->has_acquired_buffer();
228	+ if (!blocking)
229	+ client->release_buffer();
230	+ } while (!blocking);
231	+
232	+ for (int f = 0; f < 100; ++f)
233	+ {
234	+ ASSERT_FALSE(client->has_acquired_buffer());
235	+ auto onscreen = q.compositor_acquire(this);
236	+ ASSERT_TRUE(client->has_acquired_buffer());
237	+ client->release_buffer();
238	+ client = client_acquire_async(q);
239	+ q.compositor_release(onscreen);
240	+ ASSERT_FALSE(client->has_acquired_buffer());
241	+ }
242	+}
243	+
244	TEST_P(WithTwoOrMoreBuffers, clients_get_new_buffers_on_compositor_release)
245	{ // Regression test for LP: #1480164
246	+ q.set_mode(mc::MultiMonitorMode::single_monitor_fast);
247	q.allow_framedropping(false);
248
249	// Skip over the first frame. The early release optimization is too
250	@@ -526,6 +555,7 @@
251
252	TEST_P(WithTwoOrMoreBuffers, short_buffer_holds_dont_overclock_multimonitor)
253	{ // Regression test related to LP: #1480164
254	+ q.set_mode(mc::MultiMonitorMode::single_monitor_fast);
255	q.allow_framedropping(false);
256
257	// Skip over the first frame. The early release optimization is too
258	@@ -572,6 +602,7 @@
259
260	TEST_P(WithThreeOrMoreBuffers, greedy_clients_get_new_buffers_on_compositor_release)
261	{ // Regression test for LP: #1480164
262	+ q.set_mode(mc::MultiMonitorMode::single_monitor_fast);
263	q.allow_framedropping(false);
264
265	// Skip over the first frame. The early release optimization is too
266
267	=== modified file 'tests/unit-tests/compositor/test_temporary_buffers.cpp'
268	--- tests/unit-tests/compositor/test_temporary_buffers.cpp 2016-01-29 08:18:22 +0000
269	+++ tests/unit-tests/compositor/test_temporary_buffers.cpp 2016-04-01 09:06:21 +0000
270	@@ -45,6 +45,7 @@
271	MOCK_METHOD1(compositor_release, void(std::shared_ptr<mg::Buffer> const&));
272	MOCK_METHOD0(snapshot_acquire, std::shared_ptr<mg::Buffer>());
273	MOCK_METHOD1(snapshot_release, void(std::shared_ptr<mg::Buffer> const&));
274	+ MOCK_METHOD1(set_mode, void(mc::MultiMonitorMode));
275	};
276
277	class TemporaryBuffersTest : public ::testing::Test

Mir

Merge lp:~vanvugt/mir/disable-early-release into lp:mir

Commit message

Description of the change

Preview Diff

Subscribers