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

[Kevin DuBois]

optimise a a resize scenario where a buffer reallaction is not necessary. Saves a reallocation, and fixes LP: #1570765 (which was a krillin/vegeta-specific bug)

optimise a a resize scenario where a buffer reallaction is not necessary. Saves a reallocation, and fixes LP: #1570765 (which was a krillin/vegeta-specific bug)

testing:
tested on arale, krillin, frieza
checked backport of fix to 0.23 series against (1590765, 1579076, and 1584784 (frieza-only))

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3552
https://mir-jenkins.ubuntu.com/job/mir-ci/1182/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1336/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1387
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1378
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1378
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1350
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1350/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1350
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1350/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1350/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1350/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/1350
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1350/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1350
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1350/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

accidentally got stuck in "WIP" for a few days

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3552
https://mir-jenkins.ubuntu.com/job/mir-ci/1214/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1394
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1445
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1436
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/1436
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1408
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1408/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1408
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/1408/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/1408
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1408/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/1408
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1408/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1408
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/1408/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

ok

review: Approve

=== modified file 'src/client/buffer_vault.cpp'
--- src/client/buffer_vault.cpp	2016-06-06 11:51:31 +0000
+++ src/client/buffer_vault.cpp	2016-06-23 18:45:43 +0000
@@ -137,9 +137,27 @@
 
 mcl::NoTLSFuture<std::shared_ptr<mcl::MirBuffer>> mcl::BufferVault::withdraw()
 {
+    std::vector<int> free_ids;
     std::unique_lock<std::mutex> lk(mutex);
     if (disconnected_)
         BOOST_THROW_EXCEPTION(std::logic_error("server_disconnected"));
+
+    //clean up incorrectly sized buffers
+    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);
+        }
+        else
+        {
+            it++;
+        }
+    } 
+
     mcl::NoTLSPromise<std::shared_ptr<mcl::MirBuffer>> promise;
     auto it = available_buffer();
     auto future = promise.get_future();
@@ -147,6 +165,7 @@
     {
         it->second = Owner::ContentProducer;
         promise.set_value(checked_buffer_from_map(it->first));
+        lk.unlock();
     }
     else
     {
@@ -161,6 +180,9 @@
         if (allocate_buffer)
             alloc_buffer(s, format, usage);
     }
+
+    for(auto& id : free_ids)
+        free_buffer(id);
     return future;
 }
 
@@ -283,30 +305,9 @@
     set_size(lk, new_size);
 }
 
-void mcl::BufferVault::set_size(std::unique_lock<std::mutex>& lk, geometry::Size new_size)
+void mcl::BufferVault::set_size(std::unique_lock<std::mutex> const&, geometry::Size new_size)
 {
-    if (new_size == size)
-        return;
-    std::vector<int> free_ids;
     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);
-        }
-        else
-        {
-            it++;
-        }
-    } 
-    lk.unlock();
-
-    for(auto& id : free_ids)
-        free_buffer(id);
 }
 
 void mcl::BufferVault::increase_buffer_count()

=== modified file 'src/client/buffer_vault.h'
--- src/client/buffer_vault.h	2016-06-06 11:51:31 +0000
+++ src/client/buffer_vault.h	2016-06-23 18:45:43 +0000
@@ -86,7 +86,7 @@
     void free_buffer(int free_id);
     void realloc_buffer(int free_id, geometry::Size size, MirPixelFormat format, int usage);
     std::shared_ptr<MirBuffer> checked_buffer_from_map(int id);
-    void set_size(std::unique_lock<std::mutex>& lk, geometry::Size new_size);
+    void set_size(std::unique_lock<std::mutex> const& lk, geometry::Size new_size);
 
 
     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-06-06 11:51:31 +0000
+++ tests/unit-tests/client/test_buffer_vault.cpp	2016-06-23 18:45:43 +0000
@@ -327,9 +327,10 @@
     EXPECT_CALL(mock_requests, allocate_buffer(new_size,_,_));
 
     vault.wire_transfer_inbound(package.buffer_id());
+    Mock::VerifyAndClearExpectations(&mock_requests);
+
     vault.wire_transfer_inbound(package4.buffer_id());
     EXPECT_THAT(vault.withdraw().get()->size(), Eq(new_size));
-    Mock::VerifyAndClearExpectations(&mock_requests);
 }
 
 TEST_F(StartedBufferVault, withdraw_gives_only_newly_sized_buffers_after_resize)
@@ -342,20 +343,6 @@
     Mock::VerifyAndClearExpectations(&mock_requests);
 }
 
-TEST_F(StartedBufferVault, setting_size_frees_unneeded_buffers_right_away)
-{
-    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);
-    for(auto i = 0u; i < cycles; i++)
-    {
-        new_size = geom::Size(geom::Width(i), new_size.height);
-        vault.set_size(new_size);
-    }
-    Mock::VerifyAndClearExpectations(&mock_requests);
-}
-
 TEST_F(StartedBufferVault, scaling_resizes_buffers_right_away)
 {
     EXPECT_CALL(mock_requests, allocate_buffer(_,_,_))
@@ -563,10 +550,28 @@
     Mock::VerifyAndClearExpectations(&mock_requests);
 }
 
+//LP: #1590765 (more efficient allocations, and krillin/mali accommodation)
+TEST_F(StartedBufferVault, doesnt_free_buffer_until_a_newly_sized_buffer_is_called_for)
+{
+    auto i = 0u;
+    auto const num_resizes = 10u;
+    std::array<geom::Size, num_resizes> other_sizes;
+    std::generate(other_sizes.begin(), other_sizes.end(),
+        [&i, this] { return geom::Size{ size.width.as_int() + i, size.height.as_int() + i }; } );
+
+    EXPECT_CALL(mock_requests, free_buffer(_))
+        .Times(0);
+    for(auto const& size : other_sizes)
+        vault.set_size(size);
+    vault.set_size(size); //set back the original size
+
+    auto buffer_future = vault.withdraw();
+    EXPECT_THAT(buffer_future.get()->size(), Eq(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);
@@ -582,8 +587,6 @@
 
 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);