Mir

Merge lp:~vanvugt/mir/earlier-release into lp:mir

Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: 2828
Merged at revision: 2862
Proposed branch: lp:~vanvugt/mir/earlier-release
Merge into: lp:mir
Diff against target: 164 lines (+107/-0)
4 files modified
src/server/compositor/buffer_queue.cpp (+17/-0)
src/server/compositor/buffer_queue.h (+1/-0)
tests/integration-tests/test_buffer_scheduling.cpp (+54/-0)
tests/unit-tests/compositor/test_buffer_queue.cpp (+35/-0)
To merge this branch: bzr merge lp:~vanvugt/mir/earlier-release
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Daniel van Vugt Needs Information
Kevin DuBois (community) Approve
Cemil Azizoglu (community) Approve
Alan Griffiths Approve
Chris Halse Rogers Approve
Review via email: mp+267321@code.launchpad.net

Commit message

Reintroduce very short* buffer holds so that a starving client can
get a new buffer to render to much sooner, and potentially appear
much smoother (LP: #1480164)

In visual terms this is the equivalent of adding an extra buffer to
the queue without actually needing an extra buffer. It also means
we're now much more likely to be able to keep up using a lower
number of buffers (and hopefully can re-enable double buffering
eventually).

As an added bonus, this also means a compositor can better predict
where in its render loop we're going to block to send responses to
clients (while LP: #1395421 is not resolved), allowing it to defer
or move buffer releases to another thread so as to not slow down
compositing.

* "very short" means for the render time only (often 1ms or less),
  rather than the full frame interval.

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I suspect we need an equivalent test case added to kdub's new suite too?... But it's now Friday night. Later.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

*Needs Discussion*

I'm not completely comfortable with the "single monitor" naming here. If I understand the context correctly it identifies that the queue is being consumed by a single compositor (and isn't directly related to the number of monitors).

review: Needs Information
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Fair point.

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

To be completely pedantic, "compositor" is not always accurate either. It might be a frame dropper or a snapshot rather than a compositor. We should just call them "consumers" (and clients "producers").

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Chris Halse Rogers (raof) wrote :

Hm. The various special-cases in BufferQueue are solidifying my belief that we've made an incorrect abstraction.

That said, we'll soon have an opportunity to revisit that in the New Buffer Semantics™, and this looks like a sensible improvement.

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Don't underestimate the value of 600 lines of lowly coupled and thoroughly tested code. The new solution is likely to be bigger, more highly coupled and less mature for quite some time yet.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

compositor vs consumers is a preexisting irritation

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I just noticed, kdub's already using "producer" and "consumer" in new code.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

ok

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

The autolanding failure seems to be a timing glitch. That failing test relies on real time :P

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Actually this failure looks suspiciously real and related:
13: [ FAILED ] BufferQueue/WithThreeOrMoreBuffers.queue_size_scales_with_client_performance/1, where GetParam() = 4 (378 ms)

If only I could reproduce it.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

lgtm too

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Although I've been landing a lot of proposals by hand during the wily archive transition (CI fails for everyone), this proposal should probably wait. The only place it was ever failing was in CI so I want to see it pass in CI...

lp:~vanvugt/mir/earlier-release updated
2821. By Daniel van Vugt on 2015-08-14

Merge latest trunk

2822. By Daniel van Vugt on 2015-08-17

Merge latest trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~vanvugt/mir/earlier-release updated
2823. By Daniel van Vugt on 2015-08-17

Merge latest trunk

2824. By Daniel van Vugt on 2015-08-17

Merge latest trunk

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Measurements:
Maxing out a demo server with 49 clients (which is all my desktop can render smoothly using trunk), I get an improvement in the compositor render time from 3.7ms to 1.6ms using this branch. Also, this branch raises the maximum number of concurrent clients before frames are missed from 49 to 54.

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

> Measurements:
> Maxing out a demo server with 49 clients (which is all my desktop can render
> smoothly using trunk), I get an improvement in the compositor render time from
> 3.7ms to 1.6ms using this branch. Also, this branch raises the maximum number
> of concurrent clients before frames are missed from 49 to 54.

Interesting.

Any chance of getting these tests automated?

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I think I can see another location where a similar optimization is required:
  mc::BufferQueue::client_release()

That might explain why this branch still wasn't quite working as well as it should with my qtmir work.

review: Needs Information
lp:~vanvugt/mir/earlier-release updated
2825. By Daniel van Vugt on 2015-08-18

Merge latest trunk

2826. By Daniel van Vugt on 2015-08-19

Merge latest trunk

2827. By Daniel van Vugt on 2015-08-19

Merge latest trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~vanvugt/mir/earlier-release updated
2828. By Daniel van Vugt on 2015-08-20

Merge latest trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/compositor/buffer_queue.cpp'
2--- src/server/compositor/buffer_queue.cpp 2015-07-27 07:45:07 +0000
3+++ src/server/compositor/buffer_queue.cpp 2015-08-20 02:58:24 +0000
4@@ -144,6 +144,7 @@
5 the_properties{props},
6 force_new_compositor_buffer{false},
7 callbacks_allowed{true},
8+ single_compositor{false}, // When true we can optimize performance
9 gralloc{gralloc}
10 {
11 if (nbuffers < 1)
12@@ -275,6 +276,8 @@
13 bool use_current_buffer = false;
14 if (is_a_current_buffer_user(user_id)) // Primary/fastest display
15 {
16+ single_compositor = current_compositor_buffer_valid &&
17+ current_buffer_users.size() <= 1; // might be zero
18 if (ready_to_composite_queue.empty())
19 frame_deadlines_met = 0;
20 else if (frame_deadlines_met < frame_deadlines_threshold)
21@@ -357,6 +360,20 @@
22
23 if (current_compositor_buffer != buffer.get())
24 release(buffer.get(), std::move(lock));
25+ else if (!ready_to_composite_queue.empty() &&
26+ buffers_owned_by_client.empty() &&
27+ !client_ahead_of_compositor() &&
28+ single_compositor)
29+ {
30+ /*
31+ * The "early release" optimization:
32+ * This is fundamentally incompatible with multi-monitor frame sync
33+ * so we need to be sure there's only one compositor.
34+ */
35+ current_compositor_buffer = pop(ready_to_composite_queue);
36+ current_buffer_users.clear();
37+ release(buffer.get(), std::move(lock));
38+ }
39 }
40
41 std::shared_ptr<mg::Buffer> mc::BufferQueue::snapshot_acquire()
42
43=== modified file 'src/server/compositor/buffer_queue.h'
44--- src/server/compositor/buffer_queue.h 2015-06-17 05:20:42 +0000
45+++ src/server/compositor/buffer_queue.h 2015-08-20 02:58:24 +0000
46@@ -114,6 +114,7 @@
47 graphics::BufferProperties the_properties;
48 bool force_new_compositor_buffer;
49 bool callbacks_allowed;
50+ bool single_compositor;
51
52 std::condition_variable snapshot_released;
53 std::shared_ptr<graphics::GraphicBufferAllocator> gralloc;
54
55=== modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
56--- tests/integration-tests/test_buffer_scheduling.cpp 2015-07-27 07:45:07 +0000
57+++ tests/integration-tests/test_buffer_scheduling.cpp 2015-08-20 02:58:24 +0000
58@@ -714,6 +714,60 @@
59 }
60 }
61
62+TEST_P(WithTwoOrMoreBuffers, clients_get_new_buffers_on_compositor_release)
63+{ // Regression test for LP: #1480164
64+ mtd::MockFrameDroppingPolicyFactory policy_factory;
65+ mc::BufferQueue queue{nbuffers, mt::fake_shared(server_buffer_factory),
66+ properties, policy_factory};
67+ queue.allow_framedropping(false);
68+
69+ mg::Buffer* client_buffer = nullptr;
70+ auto callback = [&](mg::Buffer* buffer)
71+ {
72+ client_buffer = buffer;
73+ };
74+
75+ auto client_try_acquire = [&]() -> bool
76+ {
77+ queue.client_acquire(callback);
78+ return client_buffer != nullptr;
79+ };
80+
81+ auto client_release = [&]()
82+ {
83+ EXPECT_TRUE(client_buffer);
84+ queue.client_release(client_buffer);
85+ client_buffer = nullptr;
86+ };
87+
88+ // Skip over the first frame. The early release optimization is too
89+ // conservative to allow it to happen right at the start (so as to
90+ // maintain correct multimonitor frame rates if required).
91+ ASSERT_TRUE(client_try_acquire());
92+ client_release();
93+ queue.compositor_release(queue.compositor_acquire(this));
94+
95+ auto onscreen = queue.compositor_acquire(this);
96+
97+ bool blocking;
98+ do
99+ {
100+ blocking = !client_try_acquire();
101+ if (!blocking)
102+ client_release();
103+ } while (!blocking);
104+
105+ for (int f = 0; f < 100; ++f)
106+ {
107+ ASSERT_FALSE(client_buffer);
108+ queue.compositor_release(onscreen);
109+ ASSERT_TRUE(client_buffer) << "frame# " << f;
110+ client_release();
111+ onscreen = queue.compositor_acquire(this);
112+ client_try_acquire();
113+ }
114+}
115+
116 TEST_P(WithAnyNumberOfBuffers, compositor_inflates_ready_count_for_slow_clients)
117 {
118 queue.set_scaling_delay(3);
119
120=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
121--- tests/unit-tests/compositor/test_buffer_queue.cpp 2015-07-27 07:45:07 +0000
122+++ tests/unit-tests/compositor/test_buffer_queue.cpp 2015-08-20 02:58:24 +0000
123@@ -488,6 +488,41 @@
124 EXPECT_NO_THROW(q.compositor_release(comp_buffer));
125 }
126
127+TEST_P(WithTwoOrMoreBuffers, clients_get_new_buffers_on_compositor_release)
128+{ // Regression test for LP: #1480164
129+ q.allow_framedropping(false);
130+
131+ // Skip over the first frame. The early release optimization is too
132+ // conservative to allow it to happen right at the start (so as to
133+ // maintain correct multimonitor frame rates if required).
134+ auto handle = client_acquire_async(q);
135+ ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
136+ handle->release_buffer();
137+ q.compositor_release(q.compositor_acquire(this));
138+
139+ auto onscreen = q.compositor_acquire(this);
140+
141+ // This is what tests should do instead of using buffers_free_for_client()
142+ bool blocking;
143+ do
144+ {
145+ handle = client_acquire_async(q);
146+ blocking = !handle->has_acquired_buffer();
147+ if (!blocking)
148+ handle->release_buffer();
149+ } while (!blocking);
150+
151+ for (int f = 0; f < 100; ++f)
152+ {
153+ ASSERT_FALSE(handle->has_acquired_buffer());
154+ q.compositor_release(onscreen);
155+ ASSERT_TRUE(handle->has_acquired_buffer()) << "frame# " << f;
156+ handle->release_buffer();
157+ onscreen = q.compositor_acquire(this);
158+ handle = client_acquire_async(q);
159+ }
160+}
161+
162 TEST_P(WithAnyNumberOfBuffers, multiple_compositors_are_in_sync)
163 {
164 auto handle = client_acquire_async(q);

Subscribers

People subscribed via source and target branches