Merge into development-branch : fix-1579076 : Code : Mir

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration		Needs Fixing on 2016-05-25
Cemil Azizoglu (community)		2016-05-10	Approve on 2016-05-10
Daniel van Vugt		2016-05-06	Abstain on 2016-05-10
Review via email: mp+294036@code.launchpad.net

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-05-06:

#

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

PASSED: Continuous integration, rev:3504
https://mir-jenkins.ubuntu.com/job/mir-ci/955/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1022
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1068
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1059
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1059
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1032
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1032/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1032
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1032/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1032
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1032/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1032
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1032/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1032
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1032/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-05-09:

#

Sounds good... Does this allow us to remove the hackish fix for bug 1557442? (since this MP sounds like the correct solution)

review: Needs Information

Revision history for this message

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

#

I think that 1557442 is more appropriately a duplicate of (1579076) than of 1557962, so will change accordingly.. The overallocation strategy (which could be improved to be more dynamic, but imho is not a hack) is probably still needed for 1557962

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-05-10:

#

I shall try to develop a way to test this manually soon...

review: Abstain

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-05-10:

#

Sorry, I tried and can't easily test this right now, due to too many recent changes in buffer_vault.cpp to unwind and try one thing at a time.

I am concerned we're going downhill with code quality and maturity though. This branch has been forced into lp:mir/0.23 already without review or approval, apparently in a rush to release Mir 0.23.0 while we haven't given the new NBS code time to cool down, stabilize and mature yet. This is not good practice.

Revision history for this message

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

#

> I am concerned we're going downhill with code quality and maturity though.
> This branch has been forced into lp:mir/0.23 already without review or
> approval, apparently in a rush to release Mir 0.23.0 while we haven't given
> the new NBS code time to cool down, stabilize and mature yet. This is not good
> practice.

The MP fixes a bug that was detected in full system integration. This branch should make things more stable and performant, not less.

This MP is included in the 0.23 branch already, but this is mostly just preparation work. We're blocked awaiting silo 31 landing, so things should come together at the same time.

Revision history for this message

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

#

ok

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-05-11:

#

Yes, it's an unavoidable truth that we're not going to finish finding all the bugs and build confidence in maturity till it's in production and more people are using it.

I guess my concerns are less about NBS and more about the bigger machine it is part of. We don't have anything resembling a stable long-term support Ubuntu Touch channel that only gets bug fixes. Admittedly there is a good argument that it is too early for that.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-05-11:

#

review: Needs Fixing (continuous-integration)

Revision history for this message

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

#

^lp: #1576690

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-05-11:

#

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-05-12:

#

And bug 1576690 again.

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-05-13:

#

just retriggering .. ci seems to be normal again.. at least the auth error of the trigger job is gone

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-05-19:

#

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-05-20:

#

The failure is bug 1576690 and not this branch. Try again.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-05-20:

#

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-05-23:

#

Failure is bug 1584603

Revision history for this message

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

#

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-05-25:

#

Failure is not related to this branch. It's a krillin device hiccup:

04:59:13 2016/05/25 04:59:13 error pushing: cannot stat '/var/lib/jenkins/.cache/ubuntuimages/pool/custom-013d7cf4a5015dadb787d4c4e130e774b751d28046064a2089d8b27369b71d42.tar.xz.asc': No such file or directory

Mir

Merge lp:~kdub/mir/fix-1579076 into lp:mir

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'src/client/buffer_vault.cpp'
 --- src/client/buffer_vault.cpp	2016-05-04 14:42:48 +0000
 +++ src/client/buffer_vault.cpp	2016-05-06 18:40:55 +0000
@@ -122,7 +122,7 @@
  mcl::NoTLSFuture<std::shared_ptr<mcl::Buffer>> mcl::BufferVault::withdraw()
+ {
--    std::lock_guard<std::mutex> lk(mutex);
++    std::unique_lock<std::mutex> lk(mutex);
      if (disconnected_)
          BOOST_THROW_EXCEPTION(std::logic_error("server_disconnected"));
      mcl::NoTLSPromise<std::shared_ptr<mcl::Buffer>> promise;
@@ -146,6 +146,15 @@
      else
+     {
          promises.emplace_back(std::move(promise));
++
++        auto s = size;
++        bool allocate_buffer = (current_buffer_count <  needed_buffer_count);
++        if (allocate_buffer)
++            current_buffer_count++;
++        lk.unlock();
++
++        if (allocate_buffer)
++            alloc_buffer(s, format, usage);
+     }
      return future;
+ }
@@ -198,12 +207,12 @@
+         {
              auto id = it->first;
              buffers.erase(it);
--            lk.unlock();
--
--            free_buffer(id);
              if (should_decrease_count)
                  current_buffer_count--;
--            else
++            lk.unlock();
++
++            free_buffer(id);
++            if (!should_decrease_count)
                  alloc_buffer(size, format, usage);
              return;
+         }
@@ -221,15 +230,6 @@
+     }
+ }
--//TODO: the server will currently spam us with a lot of resize messages at once,
--//      and we want to delay the IPC transactions for resize. If we could rate-limit
--//      the incoming messages, we should should consolidate the scale and size functions
--void mcl::BufferVault::set_size(geom::Size sz)
--{
--    std::lock_guard<std::mutex> lk(mutex);
--    size = sz;
--}
--
  void mcl::BufferVault::disconnected()
+ {
      std::lock_guard<std::mutex> lk(mutex);
@@ -239,17 +239,28 @@
  void mcl::BufferVault::set_scale(float scale)
+ {
++    std::unique_lock<std::mutex> lk(mutex);
++    set_size(lk, size * scale);
++}
++
++void mcl::BufferVault::set_size(geom::Size new_size)
++{
++    std::unique_lock<std::mutex> lk(mutex);
++    set_size(lk, new_size);
++}
++
++void mcl::BufferVault::set_size(std::unique_lock<std::mutex>& lk, geometry::Size new_size)
++{
++    if (new_size == size)
++        return;
      std::vector<int> free_ids;
--    std::unique_lock<std::mutex> lk(mutex);
--    auto new_size = size * scale;
--    if (new_size == size)
--        return;
      size = new_size;
      for (auto it = buffers.begin(); it != buffers.end();)
+     {
          auto buffer = checked_buffer_from_map(it->first);
          if ((it->second == Owner::Self) && (buffer->size() != size))
+         {
++            current_buffer_count--;
              free_ids.push_back(it->first);
              it = buffers.erase(it);
+         }
@@ -261,7 +272,7 @@
      lk.unlock();
      for(auto& id : free_ids)
--        realloc_buffer(id, new_size, format, usage);
++        free_buffer(id);
+ }
  void mcl::BufferVault::increase_buffer_count()
 === modified file 'src/client/buffer_vault.h'
 --- src/client/buffer_vault.h	2016-05-04 14:42:48 +0000
 +++ src/client/buffer_vault.h	2016-05-06 18:40:55 +0000
@@ -78,6 +78,8 @@
      void alloc_buffer(geometry::Size size, MirPixelFormat format, int usage);
      void free_buffer(int free_id);
      void realloc_buffer(int free_id, geometry::Size size, MirPixelFormat format, int usage);
++    void set_size(std::unique_lock<std::mutex>& lk, geometry::Size new_size);
++
      std::shared_ptr<Buffer> checked_buffer_from_map(int id);
      std::shared_ptr<ClientBufferFactory> const platform_factory;
 === modified file 'tests/unit-tests/client/test_buffer_vault.cpp'
 --- tests/unit-tests/client/test_buffer_vault.cpp	2016-05-04 12:24:20 +0000
 +++ tests/unit-tests/client/test_buffer_vault.cpp	2016-05-06 18:40:55 +0000
@@ -326,6 +326,8 @@
  TEST_F(StartedBufferVault, reallocates_incoming_buffers_of_incorrect_size_with_immediate_response)
+ {
++    vault.set_size(new_size);
++
      EXPECT_CALL(mock_requests, free_buffer(package.buffer_id()));
      EXPECT_CALL(mock_requests, allocate_buffer(new_size,_,_))
          .WillOnce(Invoke(
@@ -334,17 +336,17 @@
              vault.wire_transfer_inbound(package4.buffer_id());
          }));
--    vault.set_size(new_size);
      vault.wire_transfer_inbound(package.buffer_id());
      Mock::VerifyAndClearExpectations(&mock_requests);
+ }
  TEST_F(StartedBufferVault, reallocates_incoming_buffers_of_incorrect_size_with_delayed_response)
+ {
++    vault.set_size(new_size);
++
      EXPECT_CALL(mock_requests, free_buffer(package.buffer_id()));
      EXPECT_CALL(mock_requests, allocate_buffer(new_size,_,_));
--    vault.set_size(new_size);
      vault.wire_transfer_inbound(package.buffer_id());
      vault.wire_transfer_inbound(package4.buffer_id());
      EXPECT_THAT(vault.withdraw().get()->size(), Eq(new_size));
@@ -361,9 +363,9 @@
      Mock::VerifyAndClearExpectations(&mock_requests);
+ }
--TEST_F(StartedBufferVault, simply_setting_size_triggers_no_server_interations)
++TEST_F(StartedBufferVault, setting_size_frees_unneeded_buffers_right_away)
+ {
--    EXPECT_CALL(mock_requests, free_buffer(_)).Times(0);
++    EXPECT_CALL(mock_requests, free_buffer(_)).Times(3);
      EXPECT_CALL(mock_requests, allocate_buffer(_,_,_)).Times(0);
      auto const cycles = 30u;
      geom::Size new_size(80, 100);
@@ -450,21 +452,14 @@
      EXPECT_CALL(mock_requests, free_buffer(_))
          .Times(initial_nbuffers);
--    auto buffer = vault.withdraw().get();
      vault.set_scale(scale);
--    vault.deposit(buffer);
--    vault.wire_transfer_outbound(buffer);
--    vault.wire_transfer_inbound(package.buffer_id());
--    for(auto i = 0; i < 100; i++)
++    for(auto i = 0u; i < initial_nbuffers; i++)
+     {
          auto b = vault.withdraw().get();
--        EXPECT_THAT(b->size(), Eq(new_size));
          vault.deposit(b);
--        b->received();
--        vault.wire_transfer_outbound(b);
--        vault.wire_transfer_inbound(buffers[(i+1)%3].buffer_id());
+     }
++
      Mock::VerifyAndClearExpectations(&mock_requests);
+ }
@@ -572,6 +567,95 @@
      vault.wire_transfer_inbound(first_id);
      EXPECT_THAT(vault.withdraw().get()->rpc_id(), Ne(first_id));
--
--
++}
++
++//LP: #1579076
++TEST_F(StartedBufferVault, doesnt_let_buffers_stagnate_after_resize)
++{
++    EXPECT_CALL(mock_requests, free_buffer(_))
++        .Times(3);
++    vault.set_size(new_size);
++
++    EXPECT_CALL(mock_requests, allocate_buffer(new_size,_,_))
++        .Times(1);
++    auto buffer_future = vault.withdraw();
++    vault.wire_transfer_inbound(package4.buffer_id());
++    EXPECT_THAT(buffer_future.get()->size(), Eq(new_size));
++    Mock::VerifyAndClearExpectations(&mock_requests);
++}
++
++TEST_F(StartedBufferVault, doesnt_free_buffers_in_the_driver_after_resize)
++{
++    EXPECT_CALL(mock_requests, free_buffer(_))
++        .Times(2);
++    auto buffer = vault.withdraw().get();
++    vault.set_size(new_size);
++    Mock::VerifyAndClearExpectations(&mock_requests);
++
++    vault.deposit(buffer);
++    vault.wire_transfer_outbound(buffer);
++
++    EXPECT_CALL(mock_requests, free_buffer(_))
++        .Times(1);
++    vault.wire_transfer_inbound(buffer->rpc_id());
++    Mock::VerifyAndClearExpectations(&mock_requests);
++}
++
++TEST_F(StartedBufferVault, doesnt_free_buffers_with_content_after_resize)
++{
++    EXPECT_CALL(mock_requests, free_buffer(_))
++        .Times(2);
++    auto buffer = vault.withdraw().get();
++    vault.deposit(buffer);
++    vault.set_size(new_size);
++    Mock::VerifyAndClearExpectations(&mock_requests);
++
++    vault.wire_transfer_outbound(buffer);
++    EXPECT_CALL(mock_requests, free_buffer(_))
++        .Times(1);
++    vault.wire_transfer_inbound(buffer->rpc_id());
++    Mock::VerifyAndClearExpectations(&mock_requests);
++}
++
++TEST_F(StartedBufferVault, doesnt_free_buffers_if_size_is_the_same)
++{
++    EXPECT_CALL(mock_requests, free_buffer(_))
++        .Times(0);
++    vault.set_size(size);
++    Mock::VerifyAndClearExpectations(&mock_requests);
++}
++
++TEST_F(StartedBufferVault, delays_allocation_if_not_needed)
++{
++    vault.set_size(new_size);
++
++    EXPECT_CALL(mock_requests, allocate_buffer(new_size,_,_))
++        .Times(1)
++        .WillOnce(Invoke(
++        [&, this](geom::Size, MirPixelFormat, int)
++        {
++            vault.wire_transfer_inbound(package4.buffer_id());
++        }));
++
++    for(auto i = 0u; i < 10u; i++)
++    {
++        auto buffer = vault.withdraw().get();
++        vault.deposit(buffer);
++        buffer->received();
++        vault.wire_transfer_outbound(buffer);
++        vault.wire_transfer_inbound(buffer->rpc_id());
++    }
++}
++
++TEST_F(StartedBufferVault, can_increase_count_after_resize)
++{
++    auto num_allocations = 4u;
++    EXPECT_CALL(mock_requests, allocate_buffer(_,_,_))
++        .Times(num_allocations);
++
++    vault.increase_buffer_count();
++    vault.set_size(new_size);
++
++    for(auto i = 0u; i < num_allocations + 1; i++)
++        vault.withdraw();
+ }