Merge lp:~mir-team/mir/enable-dynamic-buffer-queue into lp:mir
| Status: | Work in progress |
|---|---|
| Proposed branch: | lp:~mir-team/mir/enable-dynamic-buffer-queue |
| Merge into: | lp:mir |
| Diff against target: |
1537 lines (+400/-361) 10 files modified
include/test/mir_test/signal.h (+1/-0) src/server/compositor/buffer_queue.cpp (+85/-33) src/server/compositor/buffer_queue.h (+12/-3) src/server/compositor/buffer_stream_factory.cpp (+2/-3) tests/integration-tests/compositor/test_buffer_stream.cpp (+77/-63) tests/integration-tests/compositor/test_swapping_swappers.cpp (+1/-1) tests/integration-tests/graphics/android/test_buffer_integration.cpp (+2/-1) tests/integration-tests/test_surface_first_frame_sync.cpp (+1/-1) tests/mir_test/signal.cpp (+6/-0) tests/unit-tests/compositor/test_buffer_queue.cpp (+213/-256) |
| To merge this branch: | bzr merge lp:~mir-team/mir/enable-dynamic-buffer-queue |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Kevin DuBois (community) | Needs Fixing on 2014-07-07 | ||
| Daniel van Vugt | Needs Fixing on 2014-07-04 | ||
| Alexandros Frantzis (community) | Needs Fixing on 2014-07-03 | ||
| Alan Griffiths | 2014-06-27 | Abstain on 2014-07-02 | |
| PS Jenkins bot (community) | continuous-integration | Approve on 2014-07-02 | |
|
Review via email:
|
|||
Commit message
Enable dynamic reallocation in BufferQueue
Description of the change
Default to double buffering and enable dynamic reallocation in BufferQueue
This continues the work by Daniel from https:/
Change to double buffering by default - it'll will allocate more for async clients (async clients are not possible with the current client api) or in special cases when frame dropping is enabled (multiple compositors or a compositor in bypass).
| Alan Griffiths (alan-griffiths) wrote : | # |
[ FAILED ] BufferQueueTest
- 1679. By Alberto Aguirre on 2014-06-27
-
Fix failing test
- 1680. By Alberto Aguirre on 2014-06-27
-
merge lp:mir/devel
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1680
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Griffiths (alan-griffiths) wrote : | # |
In TEST_F(
Maybe we just need a better name for the test?
- 1681. By Alberto Aguirre on 2014-06-30
-
ASSERT upperbound in framerate matching tests.
| Alberto Aguirre (albaguirre) wrote : | # |
> In TEST_F(
> removing the expectation ASSERT_
> - which means we're no longer validating that the "client framerate matches
> compositor".
>
> Maybe we just need a better name for the test?
I've added back the upper bound assertion.
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1681
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 1682. By Alberto Aguirre on 2014-06-30
-
Increase test granularity
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1682
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 1683. By Alberto Aguirre on 2014-06-30
-
test slow_client_
framerate_ matches_ compositor:
Give a bit more time diff to account for thread switching in arm platform
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1683
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alberto Aguirre (albaguirre) wrote : | # |
^-- "Start 53: mir_integration
Build timed out (after 120 minutes). Marking the build as failed."
Seems like https:/
- 1684. By Alberto Aguirre on 2014-07-01
-
empty commit to kick CI
| PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1684
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 1685. By Alberto Aguirre on 2014-07-02
-
merge lp:mir/devel
| Daniel van Vugt (vanvugt) wrote : | # |
(1) A useful FIXME comment is missing from mc::BufferQueue
// FIXME: Sometimes required_buffers > nbuffers (LP: #1317403)
(2) An important test has been deleted:
1061 -TEST_F(
It needs to stay, and to pass(!). At least bypass needed 3+ buffers at some peak times last I checked. Otherwise it will intermittently deadlock. Although I forget the full reasoning right now.
(3) You have fudged slow_client_
Not sure how safe the fudge is or if it risks regression. I would be more sure if I knew how to reintroduce the bug it protects against :)
| PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1685
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Alan Griffiths (alan-griffiths) wrote : | # |
Still haven't entirely got my head around this - so changing to Abstain.
~~~~
1211 + auto const compositor_
Any reason for writing in this verbose style? C.f.
| Alberto Aguirre (albaguirre) wrote : | # |
> (1) A useful FIXME comment is missing from mc::BufferQueue
> // FIXME: Sometimes required_buffers > nbuffers (LP: #1317403)
>
> (2) An important test has been deleted:
> 1061 -TEST_F(
> It needs to stay, and to pass(!). At least bypass needed 3+ buffers at some
> peak times last I checked. Otherwise it will intermittently deadlock. Although
> I forget the full reasoning right now.
>
> (3) You have fudged slow_client_
> cheating as mentioned in
> https:/
> Not sure how safe the fudge is or if it risks regression. I would be more sure
> if I knew how to reintroduce the bug it protects against :)
(1) is not applicable anymore
(2) It's no longer an important test. Bypass does not need more than 2 buffers. If you find a deadlock scenario, let's fix the deadlock not increase the number of buffers.
(3) No cheating...you have to account for the thread/context switch time since there's no pipelining - which under valgrind running on armhf seems to be significant.
| Daniel van Vugt (vanvugt) wrote : | # |
(1) Bug 1317403 was really a consequence of bypass needing triple buffers, so read on... Also if you find a bug invalid, the first place it should be mentioned is in the bug.
(2) That's a bit surprising. For most of the year or so since I implemented bypass, 3 buffers was the minimum. Less than that and the client/server would be waiting for each other indefinitely (hence visibly freeze). And a month ago I could still reproduce some deadlocks, sometimes. But I can't today. It looks like possibly:
(a) Alan's newish async client buffer swapping solved the primary problem, as there is now significant time during swap_buffers where the client owns zero buffers. So that leaves the minimum 2 available for the server to bypass with, and then release one back to the client. Also...
(b) Bug 1319765 was unsolved around the time I was seeing deadlocks most recently so that could have been all of the remaining deadlocks, now solved.
Although I'm not yet 100% confident, it does look like bypass can now work with only two buffers. A win for performance!
(3) It's cheating because I do remember I could only reproduce the original performance bug at 16ms. It was right on the edge but I guess too brittle to rely on such timing. But I can't see either bug 1241371 or bug 1241369 occurring any more so I guess that is the important thing.
(4) BufferAllocBeha
(5) BufferAllocatio
This is just a high level review. I haven't started to read the code really but it mostly looks like my double branch.
| Daniel van Vugt (vanvugt) wrote : | # |
(6) Why delete this regression test?
973 -/* Regression test for LP#1270964 */
974 -TEST_F(
I had it passing in the double branch.
| Daniel van Vugt (vanvugt) wrote : | # |
(7) It might be slightly unnecessary but I think nice to have the extra coverage starting with 2 buffers:
1498 -/* FIXME (enabling this optimization breaks timing tests) */
1499 -TEST_F(
1500 +TEST_F(
1501 {
1502 - for (int nbuffers = 2; nbuffers <= max_nbuffers_
1503 + for (int nbuffers = 3; nbuffers <= max_nbuffers_
That test case is the most important one of this whole branch. And the best indication that it's working is if you can enable the test case without modifying it at all.
| Gerry Boland (gerboland) wrote : | # |
Just a probably stupid question from me:
> (2) That's a bit surprising. For most of the year or so since I implemented
> bypass, 3 buffers was the minimum. Less than that and the client/server would
> be waiting for each other indefinitely (hence visibly freeze). And a month ago
> I could still reproduce some deadlocks, sometimes. But I can't today. It looks
> like possibly:
> (a) Alan's newish async client buffer swapping solved the primary problem,
> as there is now significant time during swap_buffers where the client owns
> zero buffers. So that leaves the minimum 2 available for the server to bypass
> with, and then release one back to the client. Also...
> (b) Bug 1319765 was unsolved around the time I was seeing deadlocks most
> recently so that could have been all of the remaining deadlocks, now solved.
>
> Although I'm not yet 100% confident, it does look like bypass can now work
> with only two buffers. A win for performance!
If the client is being blocked from having any buffer to render into, doesn't that reduce the amount of time it has a buffer to render into? So for a client which takes just under 16ms to render each frame, and the server holds onto 2 buffers for >1ms, then the client will keep missing vsync?
| Alexandros Frantzis (afrantzis) wrote : | # |
Some suggestions from a first pass (a weak Needs Fixing).
61 + for(int i = 0; i < nalloc; i++)
Space between for and '('.
75 + if (alloc_
76 + allocated_buffers < min_buffers())
If alloc_behavior != on_demand isn't "allocated_buffers < min_buffers()" always false (we have pre-allocated the maximum number of buffers). So, we could just write "if (allocated_buffers < min_buffers())"
85 + bool use_current_buffer = force_use_
How about replacing the current logic (which has become a bit complex) with something more straightforward (IMO) like:
bool current_
{
return !current_
}
bool have_new_
{
return !ready_
}
compositor_
{
bool const use_current_buffer =
if (!use_current_
{
...
}
else if (!is_a_
{
}
}
150 + int const alloced_buffers = buffers.size();
"allocated_
| Alexandros Frantzis (afrantzis) wrote : | # |
> 85 + bool use_current_buffer = force_use_
>
> How about replacing the current logic (which has become a bit complex) with
> something more straightforward (IMO) like:
Actually, what I proposed above probably doesn't work, but the point is that we should try to simplify the paths in that function since it has become too complex.
| Alexandros Frantzis (afrantzis) wrote : | # |
> > 85 + bool use_current_buffer = force_use_
> >
> > How about replacing the current logic (which has become a bit complex) with
> > something more straightforward (IMO) like:
>
> Actually, what I proposed above probably doesn't work, but the point is that
> we should try to simplify the paths in that function since it has become too
> complex.
But this version seems to work (at least it passes all the tests):
bool const use_current_buffer =
force_
current_
!have_
| Daniel van Vugt (vanvugt) wrote : | # |
> Just a probably stupid question from me:
> ...
> If the client is being blocked from having any buffer to render into, doesn't
> that reduce the amount of time it has a buffer to render into? So for a client
> which takes just under 16ms to render each frame, and the server holds onto 2
> buffers for >1ms, then the client will keep missing vsync?
That's a good point actually. Having 2 buffers increases the risk of missing vsync compared to 3, however the increase in risk is still negligible I think. Mir is now much better at meeting frame deadlines than ever and it's regularly improving still. I don't think the slight delay of 2 vs 3 buffers is likely to ever cause skipped frames on the systems Mir supports. What's more important (and certain) is that the latency is now halved, and that's always going to be true.
It's also worth noting that double buffering probably only impacts the client render time window for a fullscreen bypassed surface (where the server requires ownership of two buffers briefly during the flip). For general compositing, the server only ever holds one brief (but mostly zero) surface buffers and the client is never delayed in getting one.
| Daniel van Vugt (vanvugt) wrote : | # |
Correction: Since SwitchingBundle became BufferQueue, the server always holds a minimum one surface buffer for compositing. Never zero.
| Daniel van Vugt (vanvugt) wrote : | # |
(8) I just added some printfs and the code is actually still triple buffering on bypass (mostly but not always). If you're not seeing that behaviour then try:
mir_
and Ctrl+Alt+arrows to rotate a few times (force compositing) before going back to bypass (Ctrl+Alt+Up).
So either:
(a) that old behaviour is still correct and you need to reinstate tests for it like bypass_
(b) it shouldn't be triple buffering bypass clients and it's a bug; or
(c) it's just bad timing in the wrong phase and sometimes really does need 3 buffers to keep up.
It looks like the bypass detection code is still present and that's what's pushing it up to 3:
increase_demand = multiple_
| Daniel van Vugt (vanvugt) wrote : | # |
(9) This test needs to be retained, and ported per the double branch...
1102 -TEST_F(
Because there's no value in allowing framedropping clients to have more than 2 buffers while the (slower) compositor uses a third. One of the two client buffers is newest (will eventually be composited) and the other can be safely discarded, overwritten by the client. This was working well in May but it seems some recent changes around framedropping policy has broken it. Certainly needs fixing because the new behaviour only wastes resources without any possible gain in performance.
| Kevin DuBois (kdub) wrote : | # |
BufferAllocatio
240 + BufferAllocatio
251 + int const shrink_treshold;
typo
273 + int allocated_buffers() const;
This is public and is only used in test code, maybe BufferQueueTest could monitor how many times the GraphicBufferAl
- 1686. By Daniel van Vugt on 2014-07-08
-
Merge latest development-branch
- 1687. By Daniel van Vugt on 2014-07-08
-
Start simplifying; replaced Policy and Behaviour classes with a simple
integer range. - 1688. By Daniel van Vugt on 2014-07-08
-
Shrink the diff (remove whitespace changes)
| Daniel van Vugt (vanvugt) wrote : | # |
Work in progress... I'll carry on with this tomorrow. Or fix the original branch. Not sure.
Unmerged revisions
- 1688. By Daniel van Vugt on 2014-07-08
-
Shrink the diff (remove whitespace changes)
- 1687. By Daniel van Vugt on 2014-07-08
-
Start simplifying; replaced Policy and Behaviour classes with a simple
integer range. - 1686. By Daniel van Vugt on 2014-07-08
-
Merge latest development-branch
- 1685. By Alberto Aguirre on 2014-07-02
-
merge lp:mir/devel
- 1684. By Alberto Aguirre on 2014-07-01
-
empty commit to kick CI
- 1683. By Alberto Aguirre on 2014-06-30
-
test slow_client_
framerate_ matches_ compositor:
Give a bit more time diff to account for thread switching in arm platform - 1682. By Alberto Aguirre on 2014-06-30
-
Increase test granularity
- 1681. By Alberto Aguirre on 2014-06-30
-
ASSERT upperbound in framerate matching tests.
- 1680. By Alberto Aguirre on 2014-06-27
-
merge lp:mir/devel
- 1679. By Alberto Aguirre on 2014-06-27
-
Fix failing test
FAILED: Continuous integration, rev:1678
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://