Mir

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

earlier-release
Merge into development-branch

Proposed by Daniel van Vugt on 2015-08-07

Status:

Merged

Approved by:

Daniel van Vugt on 2015-08-20

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

Related bugs:

Bug #1395421: Protocol (buffer) replies are being sent from the compositor thread, holding up the render loop	Medium	Fix Released
Bug #1480164: [regression] Mir is holding buffers much longer than it used to.	Medium	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2015-08-20
Daniel van Vugt			Needs Information on 2015-08-18
Kevin DuBois (community)			Approve on 2015-08-12
Cemil Azizoglu (community)			Approve on 2015-08-10
Alan Griffiths		2015-08-07	Approve on 2015-08-10
Chris Halse Rogers			Approve on 2015-08-10
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.

Daniel van Vugt (vanvugt) wrote on 2015-08-07:

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

PS Jenkins bot (ps-jenkins) wrote on 2015-08-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4504/rebuild

review: Approve (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2015-08-07:

*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

Daniel van Vugt (vanvugt) wrote on 2015-08-10:

Fair point.

Daniel van Vugt (vanvugt) wrote on 2015-08-10:

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

PS Jenkins bot (ps-jenkins) wrote on 2015-08-10:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4507/rebuild

review: Approve (continuous-integration)

Chris Halse Rogers (raof) wrote on 2015-08-10:

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

Daniel van Vugt (vanvugt) wrote on 2015-08-10:

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.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-10:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4509/rebuild

review: Approve (continuous-integration)

Alan Griffiths (alan-griffiths) wrote on 2015-08-10:

compositor vs consumers is a preexisting irritation

review: Approve

Daniel van Vugt (vanvugt) wrote on 2015-08-10:

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

PS Jenkins bot (ps-jenkins) wrote on 2015-08-10:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4510/rebuild

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-08-10:

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1758/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3460
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/320
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/992
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3408
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/311/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3408
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3408/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6195
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22418

review: Needs Fixing (continuous-integration)

Cemil Azizoglu (cemil-azizoglu) wrote on 2015-08-10:

review: Approve

Daniel van Vugt (vanvugt) wrote on 2015-08-11:

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

PS Jenkins bot (ps-jenkins) wrote on 2015-08-11:

review: Needs Fixing (continuous-integration)

PS Jenkins bot (ps-jenkins) wrote on 2015-08-11:

review: Needs Fixing (continuous-integration)

Daniel van Vugt (vanvugt) wrote on 2015-08-11:

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.

PS Jenkins bot (ps-jenkins) wrote on 2015-08-12:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4545/rebuild

review: Needs Fixing (continuous-integration)

Kevin DuBois (kdub) wrote on 2015-08-12:

lgtm too

review: Approve

Daniel van Vugt (vanvugt) wrote on 2015-08-13:

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 on 2015-08-17

2821. By Daniel van Vugt on 2015-08-14: Merge latest trunk
2822. By Daniel van Vugt on 2015-08-17: Merge latest trunk

PS Jenkins bot (ps-jenkins) wrote on 2015-08-17:

FAILED: Continuous integration, rev:2822
http://jenkins.qa.ubuntu.com/job/mir-ci/4581/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3545
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/406/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/1078/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3493/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/730/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3493
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3493/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6271/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22606

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4581/rebuild

review: Needs Fixing (continuous-integration)

lp:~vanvugt/mir/earlier-release updated on 2015-08-17

2823. By Daniel van Vugt on 2015-08-17: Merge latest trunk
2824. By Daniel van Vugt on 2015-08-17: Merge latest trunk

Daniel van Vugt (vanvugt) wrote on 2015-08-17:

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.

Alan Griffiths (alan-griffiths) wrote on 2015-08-17:

> 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?

Daniel van Vugt (vanvugt) wrote on 2015-08-18:

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 on 2015-08-19

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

PS Jenkins bot (ps-jenkins) wrote on 2015-08-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4610/rebuild

review: Needs Fixing (continuous-integration)

lp:~vanvugt/mir/earlier-release updated on 2015-08-20

2828. By Daniel van Vugt on 2015-08-20: Merge latest trunk

PS Jenkins bot (ps-jenkins) wrote on 2015-08-20:

PASSED: Continuous integration, rev:2828
http://jenkins.qa.ubuntu.com/job/mir-ci/4624/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3594
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2505
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3544
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/773
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/773/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3544
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3544/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6317
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22705

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4624/rebuild

review: Approve (continuous-integration)

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

 === modified file 'src/server/compositor/buffer_queue.cpp'
 --- src/server/compositor/buffer_queue.cpp	2015-07-27 07:45:07 +0000
 +++ src/server/compositor/buffer_queue.cpp	2015-08-20 02:58:24 +0000
@@ -144,6 +144,7 @@
        the_properties{props},
        force_new_compositor_buffer{false},
        callbacks_allowed{true},
++      single_compositor{false},  // When true we can optimize performance
        gralloc{gralloc}
+ {
      if (nbuffers < 1)
@@ -275,6 +276,8 @@
      bool use_current_buffer = false;
      if (is_a_current_buffer_user(user_id))   // Primary/fastest display
+     {
++        single_compositor = current_compositor_buffer_valid &&
++                            current_buffer_users.size() <= 1;  // might be zero
          if (ready_to_composite_queue.empty())
              frame_deadlines_met = 0;
          else if (frame_deadlines_met < frame_deadlines_threshold)
@@ -357,6 +360,20 @@
      if (current_compositor_buffer != buffer.get())
          release(buffer.get(), std::move(lock));
++    else if (!ready_to_composite_queue.empty() &&
++             buffers_owned_by_client.empty() &&
++             !client_ahead_of_compositor() &&
++             single_compositor)
++    {
++        /*
++         * The "early release" optimization:
++         * This is fundamentally incompatible with multi-monitor frame sync
++         * so we need to be sure there's only one compositor.
++         */
++        current_compositor_buffer = pop(ready_to_composite_queue);
++        current_buffer_users.clear();
++        release(buffer.get(), std::move(lock));
++    }
+ }
  std::shared_ptr<mg::Buffer> mc::BufferQueue::snapshot_acquire()
 === modified file 'src/server/compositor/buffer_queue.h'
 --- src/server/compositor/buffer_queue.h	2015-06-17 05:20:42 +0000
 +++ src/server/compositor/buffer_queue.h	2015-08-20 02:58:24 +0000
@@ -114,6 +114,7 @@
      graphics::BufferProperties the_properties;
      bool force_new_compositor_buffer;
      bool callbacks_allowed;
++    bool single_compositor;
      std::condition_variable snapshot_released;
      std::shared_ptr<graphics::GraphicBufferAllocator> gralloc;
 === modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
 --- tests/integration-tests/test_buffer_scheduling.cpp	2015-07-27 07:45:07 +0000
 +++ tests/integration-tests/test_buffer_scheduling.cpp	2015-08-20 02:58:24 +0000
@@ -714,6 +714,60 @@
+     }
+ }
++TEST_P(WithTwoOrMoreBuffers, clients_get_new_buffers_on_compositor_release)
++{   // Regression test for LP: #1480164
++    mtd::MockFrameDroppingPolicyFactory policy_factory;
++    mc::BufferQueue queue{nbuffers, mt::fake_shared(server_buffer_factory),
++                          properties, policy_factory};
++    queue.allow_framedropping(false);
++
++    mg::Buffer* client_buffer = nullptr;
++    auto callback = [&](mg::Buffer* buffer)
++    {
++        client_buffer = buffer;
++    };
++
++    auto client_try_acquire = [&]() -> bool
++    {
++        queue.client_acquire(callback);
++        return client_buffer != nullptr;
++    };
++
++    auto client_release = [&]()
++    {
++        EXPECT_TRUE(client_buffer);
++        queue.client_release(client_buffer);
++        client_buffer = nullptr;
++    };
++
++    // Skip over the first frame. The early release optimization is too
++    // conservative to allow it to happen right at the start (so as to
++    // maintain correct multimonitor frame rates if required).
++    ASSERT_TRUE(client_try_acquire());
++    client_release();
++    queue.compositor_release(queue.compositor_acquire(this));
++
++    auto onscreen = queue.compositor_acquire(this);
++
++    bool blocking;
++    do
++    {
++        blocking = !client_try_acquire();
++        if (!blocking)
++            client_release();
++    } while (!blocking);
++
++    for (int f = 0; f < 100; ++f)
++    {
++        ASSERT_FALSE(client_buffer);
++        queue.compositor_release(onscreen);
++        ASSERT_TRUE(client_buffer) << "frame# " << f;
++        client_release();
++        onscreen = queue.compositor_acquire(this);
++        client_try_acquire();
++    }
++}
++
  TEST_P(WithAnyNumberOfBuffers, compositor_inflates_ready_count_for_slow_clients)
+ {
      queue.set_scaling_delay(3);
 === modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
 --- tests/unit-tests/compositor/test_buffer_queue.cpp	2015-07-27 07:45:07 +0000
 +++ tests/unit-tests/compositor/test_buffer_queue.cpp	2015-08-20 02:58:24 +0000
@@ -488,6 +488,41 @@
      EXPECT_NO_THROW(q.compositor_release(comp_buffer));
+ }
++TEST_P(WithTwoOrMoreBuffers, clients_get_new_buffers_on_compositor_release)
++{   // Regression test for LP: #1480164
++    q.allow_framedropping(false);
++
++    // Skip over the first frame. The early release optimization is too
++    // conservative to allow it to happen right at the start (so as to
++    // maintain correct multimonitor frame rates if required).
++    auto handle = client_acquire_async(q);
++    ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
++    handle->release_buffer();
++    q.compositor_release(q.compositor_acquire(this));
++
++    auto onscreen = q.compositor_acquire(this);
++
++    // This is what tests should do instead of using buffers_free_for_client()
++    bool blocking;
++    do
++    {
++        handle = client_acquire_async(q);
++        blocking = !handle->has_acquired_buffer();
++        if (!blocking)
++            handle->release_buffer();
++    } while (!blocking);
++
++    for (int f = 0; f < 100; ++f)
++    {
++        ASSERT_FALSE(handle->has_acquired_buffer());
++        q.compositor_release(onscreen);
++        ASSERT_TRUE(handle->has_acquired_buffer()) << "frame# " << f;
++        handle->release_buffer();
++        onscreen = q.compositor_acquire(this);
++        handle = client_acquire_async(q);
++    }
++}
++
  TEST_P(WithAnyNumberOfBuffers, multiple_compositors_are_in_sync)
+ {
      auto handle = client_acquire_async(q);

Mir

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

Commit Message

Description of the Change

Preview Diff

Subscribers