Mir

Merge lp:~kdub/mir/nbs-overallocation into lp:mir

nbs-overallocation
Merge into development-branch

Proposed by Kevin DuBois on 2016-03-23

Status:

Merged

Approved by:

Kevin DuBois on 2016-04-01

Approved revision:

no longer in the source branch.

Merged at revision:

3433

Proposed branch:

lp:~kdub/mir/nbs-overallocation

Merge into:

lp:mir

Diff against target:

280 lines (+136/-25)

4 files modified

src/client/buffer_stream.cpp (+21/-0)
src/client/buffer_vault.cpp (+45/-11)
src/client/buffer_vault.h (+5/-0)
tests/unit-tests/client/test_buffer_vault.cpp (+65/-14)

To merge this branch:

bzr merge lp:~kdub/mir/nbs-overallocation

Related bugs:

Bug #1557442: NBS causes sudden loss of performance after un-fullscreening a swap interval zero client	Medium	Fix Released
Bug #1557962: NBS fullscreen overlay benchmark performance is much lower than BufferQueue	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Alan Griffiths			Approve on 2016-04-01
Daniel van Vugt			Abstain on 2016-04-01
Mir CI Bot	continuous-integration		Approve on 2016-03-31
Cemil Azizoglu (community)		2016-03-23	Approve on 2016-03-28
Review via email: mp+289912@code.launchpad.net

Commit message

nbs: add client-side overallocation when there's buffer pressure due to overlay/bypass scenarios and swapinterval is 0.

fixes: LP: #1557962

Description of the change

nbs: add client-side overallocation when there's buffer pressure due to overlay/bypass scenarios and swapinterval is 0.

fixes: LP: #1557962

note: thought we may not have needed, but turns out we do. I suppose this should land before lp:~kdub/mir/nbs-by-default lands.

Revision history for this message

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

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

^^ seems a problem with the devices

Revision history for this message

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

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

review: Approve (continuous-integration)

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-03-28:

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-03-29:

+ if (current_swap_interval == interval)
+ return;

Should current_swap_interval be guarded by "mutex"?

If so, the lock is missing.

If not, then it is unclear which variables should be protected by "mutex".

review: Needs Fixing

Revision history for this message

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

This isn't a review, but I suggest more explicit logic to explain the reasons here. Something like:

// Calculate how many concurrent buffers are required to keep all our processes running smoothly in parallel:

int overlaid_buffers = (bypass||overlay) ? 1 : 0;
int compositing_buffers = 1;
int client_buffers = (swap_interval == 0) ? 2 : 1;
int total_buffers_required = client_buffers + compositing_buffers + overlaid_buffers;

Revision history for this message

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

'Needs Information' just to remind me to check this again later.

review: Needs Information

Revision history for this message

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

Or even better:

// Calculate how many concurrent buffers are required to keep all our processes running smoothly in parallel:

int frames_visible_at_once = (nmonitors > 1) ? 2 : 1;
int overlaid_buffers = (bypass||overlay) ? frames_visible_at_once : 0;
int compositing_buffers = frames_visible_at_once;
int client_buffers = (swap_interval == 0) ? 2 : 1;
int min_buffers_required = client_buffers + compositing_buffers + overlaid_buffers;

Because holding an overlay/bypass buffer on the screen for scanout happens in parallel to compositing the next server frame, happens in parallel to the client preparing for the next frame after that.

Revision history for this message

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

@swapinterval, working on fixing.

@more explicit calculation
From the client's perspective, the client just really knows how many buffers the client has or how many the server has. It can increase its allocation count when it runs out (according to a policy), but it doesn't have to know why its increasing its count. In this MP, the policy is simplistic (increase when swapinterval==0), but sufficient for the bug. In the future, a better policy would be to detect periods of buffer pressure and adjust on the fly.

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/711/rebuild

review: Approve (continuous-integration)

Revision history for this message

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

review: Abstain

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-04-01:

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:

Emanuele Antonio Faraone

Gerry Boland

Kevin DuBois

Mir development team

 === modified file 'src/client/buffer_stream.cpp'
 --- src/client/buffer_stream.cpp	2016-03-23 06:39:56 +0000
 +++ src/client/buffer_stream.cpp	2016-03-30 12:22:48 +0000
@@ -63,6 +63,7 @@
      virtual void lost_connection() = 0;
      virtual void set_size(geom::Size) = 0;
      virtual MirWaitHandle* set_scale(float, mf::BufferStreamId) = 0;
++    virtual void set_interval(int interval) = 0;
      virtual ~ServerBufferSemantics() = default;
      ServerBufferSemantics() = default;
      ServerBufferSemantics(ServerBufferSemantics const&) = delete;
@@ -196,6 +197,10 @@
          return &scale_wait_handle;
+     }
++    void set_interval(int) override
++    {
++    }
++
      std::mutex mutex;
      mcl::ClientBufferDepository wrapped;
      mir::protobuf::DisplayServer& display_server;
@@ -349,11 +354,25 @@
          return &scale_wait_handle;
+     }
++    void set_interval(int interval) override
++    {
++        std::unique_lock<decltype(mutex)> lk(mutex);
++        interval = std::max(0, std::min(1, interval));
++        if (current_swap_interval == interval)
++            return;
++        if (interval == 0)
++            vault.increase_buffer_count();
++        else
++            vault.decrease_buffer_count();
++        current_swap_interval = interval;
++    }
++
      mcl::BufferVault vault;
      std::mutex mutex;
      mcl::BufferInfo current{nullptr, 0};
      MirWaitHandle next_buffer_wait_handle;
      MirWaitHandle scale_wait_handle;
++    int current_swap_interval = 1;
  };
+ }
@@ -638,6 +657,8 @@
      configuration.mutable_id()->set_value(protobuf_bs->id().value());
      configuration.set_swapinterval(interval);
++    buffer_depository->set_interval(interval);
++
      interval_wait_handle.expect_result();
      display_server.configure_buffer_stream(&configuration, protobuf_void.get(),
          google::protobuf::NewCallback(this, &mcl::BufferStream::on_swap_interval_set, interval));
 === modified file 'src/client/buffer_vault.cpp'
 --- src/client/buffer_vault.cpp	2016-03-23 06:39:56 +0000
 +++ src/client/buffer_vault.cpp	2016-03-30 12:22:48 +0000
@@ -32,8 +32,9 @@
  enum class mcl::BufferVault::Owner
+ {
      Server,
++    Self,
      ContentProducer,
--    Self
++    SelfWithContent
  };
  mcl::BufferVault::BufferVault(
@@ -45,9 +46,12 @@
      format(format),
      usage(usage),
      size(size),
--    disconnected_(false)
++    disconnected_(false),
++    current_buffer_count(initial_nbuffers),
++    needed_buffer_count(initial_nbuffers),
++    initial_buffer_count(initial_nbuffers)
+ {
--    for (auto i = 0u; i < initial_nbuffers; i++)
++    for (auto i = 0u; i < initial_buffer_count; i++)
          server_requests->allocate_buffer(size, format, usage);
+ }
@@ -95,7 +99,7 @@
      if (it == buffers.end() || it->second.owner != Owner::ContentProducer)
          BOOST_THROW_EXCEPTION(std::logic_error("buffer cannot be deposited"));
--    it->second.owner = Owner::Self;
++    it->second.owner = Owner::SelfWithContent;
      it->second.buffer->increment_age();
+ }
@@ -106,7 +110,7 @@
      std::unique_lock<std::mutex> lk(mutex);
      auto it = std::find_if(buffers.begin(), buffers.end(),
          [&buffer](std::pair<int, BufferEntry> const& entry) { return buffer == entry.second.buffer; });
--    if (it == buffers.end() || it->second.owner != Owner::Self)
++    if (it == buffers.end() || it->second.owner != Owner::SelfWithContent)
          BOOST_THROW_EXCEPTION(std::logic_error("buffer cannot be transferred"));
      it->second.owner = Owner::Server;
@@ -143,17 +147,18 @@
      else
+     {
          it->second.buffer->update_from(*package);
--        if (size == it->second.buffer->size())
--        {
--            it->second.owner = Owner::Self;
--        }
--        else
++        it->second.owner = Owner::Self;
++        auto should_decrease_count = (current_buffer_count > needed_buffer_count);
++        if ((size != it->second.buffer->size()) || should_decrease_count)
+         {
              int id = it->first;
              buffers.erase(it);
              lk.unlock();
              server_requests->free_buffer(id);
--            server_requests->allocate_buffer(size, format, usage);
++            if (should_decrease_count)
++                current_buffer_count--;
++            else
++                server_requests->allocate_buffer(size, format, usage);
              return;
+         }
+     }
@@ -210,3 +215,32 @@
          server_requests->free_buffer(id);
+     }
+ }
++
++void mcl::BufferVault::increase_buffer_count()
++{
++    std::unique_lock<std::mutex> lk(mutex);
++    current_buffer_count++;
++    needed_buffer_count++;
++    lk.unlock();
++
++    server_requests->allocate_buffer(size, format, usage);
++}
++
++void mcl::BufferVault::decrease_buffer_count()
++{
++    std::unique_lock<std::mutex> lk(mutex);
++    if (current_buffer_count == initial_buffer_count)
++        return;
++    needed_buffer_count--;
++
++    auto it = std::find_if(buffers.begin(), buffers.end(),
++        [](std::pair<int, BufferEntry> const& entry) { return entry.second.owner == Owner::Self; });
++    if (it != buffers.end())
++    {
++        current_buffer_count--;
++        int free_id = it->first;
++        buffers.erase(it);
++        lk.unlock();
++        server_requests->free_buffer(free_id);
++    }
++}
 === modified file 'src/client/buffer_vault.h'
 --- src/client/buffer_vault.h	2016-03-23 06:39:56 +0000
 +++ src/client/buffer_vault.h	2016-03-30 12:22:48 +0000
@@ -71,6 +71,8 @@
      void set_size(geometry::Size);
      void disconnected();
      void set_scale(float scale);
++    void increase_buffer_count();
++    void decrease_buffer_count();
  private:
      std::shared_ptr<ClientBufferFactory> const factory;
@@ -90,6 +92,9 @@
      std::deque<NoTLSPromise<BufferInfo>> promises;
      geometry::Size size;
      bool disconnected_;
++    size_t current_buffer_count;
++    size_t needed_buffer_count;
++    size_t const initial_buffer_count;
  };
+ }
+ }
 === modified file 'tests/unit-tests/client/test_buffer_vault.cpp'
 --- tests/unit-tests/client/test_buffer_vault.cpp	2016-03-23 06:39:56 +0000
 +++ tests/unit-tests/client/test_buffer_vault.cpp	2016-03-30 12:22:48 +0000
@@ -294,20 +294,6 @@
      vault.wire_transfer_outbound(buffer);
+ }
--//handy for android's cancelbuffer
--TEST_F(StartedBufferVault, can_withdraw_and_deposit)
--{
--    auto a_few_times = 5u;
--    std::vector<std::shared_ptr<mcl::ClientBuffer>> buffers(a_few_times);
--    for (auto i = 0u; i < a_few_times; i++)
--    {
--        buffers[i] = vault.withdraw().get().buffer;
--        vault.deposit(buffers[i]);
--    }
--    EXPECT_THAT(buffers, Each(buffers[0]));
--}
--
--
  TEST_F(StartedBufferVault, reallocates_incoming_buffers_of_incorrect_size_with_immediate_response)
+ {
      mp::Buffer package4;
@@ -484,3 +470,68 @@
      vault.wire_transfer_inbound(package3);
+ }
++
++TEST_F(StartedBufferVault, can_increase_allocation_count)
++{
++    EXPECT_CALL(mock_requests, allocate_buffer(_,_,_))
++        .Times(1);
++    vault.increase_buffer_count();
++
++    Mock::VerifyAndClearExpectations(&mock_requests);
++    //no buffers
++}
++
++TEST_F(StartedBufferVault, cannot_decrease_allocation_count_below_initial)
++{
++    EXPECT_CALL(mock_requests, free_buffer(_))
++        .Times(0);
++    vault.decrease_buffer_count();
++    Mock::VerifyAndClearExpectations(&mock_requests);
++}
++
++TEST_F(StartedBufferVault, can_decrease_allocation_count)
++{
++    EXPECT_CALL(mock_requests, allocate_buffer(_,_,_))
++        .Times(1);
++    EXPECT_CALL(mock_requests, free_buffer(_))
++        .Times(1);
++    vault.increase_buffer_count();
++    vault.decrease_buffer_count();
++    Mock::VerifyAndClearExpectations(&mock_requests);
++}
++
++TEST_F(StartedBufferVault, delayed_decrease_allocation_count)
++{
++    mp::Buffer requested_buffer;
++    requested_buffer.set_width(size.width.as_int());
++    requested_buffer.set_height(size.height.as_int());
++    requested_buffer.set_buffer_id(4);
++
++    EXPECT_CALL(mock_requests, allocate_buffer(_,_,_))
++        .Times(1);
++    EXPECT_CALL(mock_requests, free_buffer(package.buffer_id()))
++        .Times(1);
++
++    vault.increase_buffer_count();
++    vault.wire_transfer_inbound(requested_buffer);
++    auto b = vault.withdraw().get().buffer;
++    vault.deposit(b);
++    vault.wire_transfer_outbound(b);
++
++    b = vault.withdraw().get().buffer;
++    vault.deposit(b);
++    vault.wire_transfer_outbound(b);
++
++    b = vault.withdraw().get().buffer;
++    vault.deposit(b);
++    vault.wire_transfer_outbound(b);
++
++    b = vault.withdraw().get().buffer;
++    vault.deposit(b);
++    vault.wire_transfer_outbound(b);
++
++    vault.decrease_buffer_count();
++    vault.wire_transfer_inbound(package);
++    vault.wire_transfer_inbound(package2);
++    Mock::VerifyAndClearExpectations(&mock_requests);
++}