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

[Kevin DuBois]

client: fix lp: #1578159 by preferring buffers that returned to the client earlier, as opposed to preferring the last-returned buffer.

fixes: LP: #1578159

Note on test change to "queue_size_scales_with_client_performance":
The nbs code, although now cycling through the 3 buffers, still will perform as double buffered, as it always has a spare buffer to work with.

client: fix lp: #1578159 by preferring buffers that returned to the client earlier, as opposed to preferring the last-returned buffer.

fixes: LP: #1578159

Note on test change to "queue_size_scales_with_client_performance":
The nbs code, although now cycling through the 3 buffers, still will perform as double buffered, as it always has a spare buffer to work with.

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

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I can't help feeling that the double find_if() search indicates we're using the wrong data structure. But OK.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

Ok.

Nit: These lines are not needed.

147 +
148 +

review: Approve

[Kevin DuBois (kdub)]

> I can't help feeling that the double find_if() search indicates we're using
> the wrong data structure. But OK.

Yeah, a queue would be better

[Kevin DuBois (kdub)]

still testing cross-device, probably should have put this as wip when proposed

[Daniel van Vugt (vanvugt)]

Sounds like the right fix. If we were preferring the latest buffer returned then we were accidentally double buffering instead of triple. Which would quantize to half frame rate more easily.

What concerns me though is that you've broken one aspect of the old test cases. In that you no longer expect minimal buffer allocation [1]:

88 - EXPECT_THAT(unique_ids_in(log), Eq(2));
89 + if (std::get<1>(GetParam()) == TestType::SubmitSemantics)
90 + EXPECT_THAT(log, NeverBlocks());
91 + if (std::get<1>(GetParam()) == TestType::ExchangeSemantics)
92 + EXPECT_THAT(unique_ids_in(log), Eq(2));

[1] https://docs.google.com/spreadsheets/d/1qf3IssN_sujygMPK2sTX2AUhGiDUBF3TMYYcSRSaSy0/pubhtml

That's a slight regression compared to BufferQueue. Although BufferQueue did not do it well to start with.

review: Abstain

[Daniel van Vugt (vanvugt)]

Come to think of it... Instead of breaking the tests, please just disable them and comment "TODO: Reintroduce efficient allocation later".

It's not a regression test if you have different expectations for the old and new ways.

review: Needs Fixing

[Kevin DuBois (kdub)]

> Sounds like the right fix. If we were preferring the latest buffer returned
> then we were accidentally double buffering instead of triple. Which would
> quantize to half frame rate more easily.

Right, but this is on design... it will scale us to double buffering when the 3rd isn't needed. Alternating which buffer is the spare buffer still has the benefits of double buffering, although there are 3 buffers involved.

> What concerns me though is that you've broken one aspect of the old test
> cases. In that you no longer expect minimal buffer allocation [1]:

Is this what the test is checking for though? The nbs stuff still has 3 buffers allocated. The test is checking the output for profiles consistent with double buffering. The change to the test is to accommodate the alternation of the 3rd buffer in and out to improve android performance.

[Kevin DuBois (kdub)]

tested nbs vs obs on phone stack on arale, flo, krillin, and should be at parity. Still digging though on what seems to be an unrelated-to-this-mp issue

[Daniel van Vugt (vanvugt)]

You're right the test is dual-purpose. Although I can't seem to find another test that covers the number of buffers touched allocation-efficiency issue. So I would prefer the test retained its dual purposes.

That said, it's not a major problem or entirely new if we have 3 buffers allocated when only 2 are in use. That's a 'regression' we could happily live with indefinitely.

review: Abstain

=== modified file 'src/client/buffer_vault.cpp'
--- src/client/buffer_vault.cpp	2016-05-03 09:49:54 +0000
+++ src/client/buffer_vault.cpp	2016-05-04 14:48:14 +0000
@@ -129,8 +129,13 @@
     auto it = std::find_if(buffers.begin(), buffers.end(),
         [this](std::pair<int, Owner> const& entry) {
             return ((entry.second == Owner::Self) &&
-                    (checked_buffer_from_map(entry.first)->size() == size));
-    });
+                    (checked_buffer_from_map(entry.first)->size() == size) &&
+                    (entry.first != last_received_id)); });
+    if (it == buffers.end())
+        it = std::find_if(buffers.begin(), buffers.end(),
+        [this](std::pair<int, Owner> const& entry) {
+            return ((entry.second == Owner::Self) &&
+                    (checked_buffer_from_map(entry.first)->size() == size)); });
 
     auto future = promise.get_future();
     if (it != buffers.end())
@@ -172,7 +177,7 @@
 void mcl::BufferVault::wire_transfer_inbound(int buffer_id)
 {
     std::unique_lock<std::mutex> lk(mutex);
-
+    last_received_id = buffer_id;
     auto buffer = checked_buffer_from_map(buffer_id);
     auto inbound_size = buffer->size();
     auto it = buffers.find(buffer_id);

=== modified file 'src/client/buffer_vault.h'
--- src/client/buffer_vault.h	2016-05-03 09:49:54 +0000
+++ src/client/buffer_vault.h	2016-05-04 14:48:14 +0000
@@ -96,6 +96,7 @@
     size_t current_buffer_count;
     size_t needed_buffer_count;
     size_t const initial_buffer_count;
+    int last_received_id = 0;
 };
 }
 }

=== modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
--- tests/integration-tests/test_buffer_scheduling.cpp	2016-05-03 09:49:54 +0000
+++ tests/integration-tests/test_buffer_scheduling.cpp	2016-05-04 14:48:14 +0000
@@ -144,7 +144,7 @@
         }
         else
         {
-            entries.emplace_back(BufferEntry{mg::BufferID{2}, 0u, Access::blocked});
+            entries.emplace_back(BufferEntry{mg::BufferID{INT_MAX}, 0u, Access::blocked});
         }
     }
 
@@ -565,6 +565,15 @@
     return std::distance(log.begin(), it);
 }
 
+MATCHER(NeverBlocks, "")
+{
+    bool never_blocks = true;
+    for(auto& e : arg)
+        never_blocks &= (e.blockage == Access::unblocked);
+    return never_blocks; 
+}
+
+
 //test infrastructure
 struct BufferScheduling : public Test, ::testing::WithParamInterface<std::tuple<int, TestType>>
 {
@@ -1454,7 +1463,12 @@
     auto log = producer->production_log();
     ASSERT_THAT(log.size(), Gt(discard));  // avoid the below erase crashing
     log.erase(log.begin(), log.begin() + discard);
-    EXPECT_THAT(unique_ids_in(log), Eq(2));
+
+    if (std::get<1>(GetParam()) == TestType::SubmitSemantics)
+        EXPECT_THAT(log, NeverBlocks());
+    if (std::get<1>(GetParam()) == TestType::ExchangeSemantics)
+        EXPECT_THAT(unique_ids_in(log), Eq(2));
+
     producer->reset_log();
 
     //put server-side pressure on the buffer count
@@ -1485,7 +1499,10 @@
     // Expect double-buffers as the steady state for fast clients
     log = producer->production_log();
     log.erase(log.begin(), log.begin() + discard);
-    EXPECT_THAT(unique_ids_in(log), Eq(2));
+    if (std::get<1>(GetParam()) == TestType::SubmitSemantics)
+        EXPECT_THAT(log, NeverBlocks());
+    if (std::get<1>(GetParam()) == TestType::ExchangeSemantics)
+        EXPECT_THAT(unique_ids_in(log), Eq(2));
 }
 
 //NOTE: compositors need 2 buffers in overlay/bypass cases, as they 
@@ -1532,6 +1549,24 @@
     EXPECT_THAT(snaps, Each(production_log.back().id));
 }
 
+//LP: #1578159
+//If given the choice best to prefer buffers that the compositor used furthest in the past
+//so that we avert any waits on synchronization internal to the buffers or platform.
+TEST_P(WithThreeOrMoreBuffers, prefers_fifo_ordering_when_distributing_buffers_to_driver)
+{ 
+    producer->produce();
+    producer->produce();
+    consumer->consume();
+    consumer->consume();
+    producer->produce();
+
+    auto production_log = producer->production_log();
+    auto consumption_log = consumer->consumption_log();
+    ASSERT_THAT(production_log, Not(IsEmpty()));
+    ASSERT_THAT(consumption_log, Not(IsEmpty()));
+    EXPECT_THAT(production_log.back().id, Ne(consumption_log.front().id));
+}
+
 int const max_buffers_to_test{5};
 INSTANTIATE_TEST_CASE_P(
     BufferScheduling,

=== modified file 'tests/unit-tests/client/test_buffer_vault.cpp'
--- tests/unit-tests/client/test_buffer_vault.cpp	2016-05-03 09:49:54 +0000
+++ tests/unit-tests/client/test_buffer_vault.cpp	2016-05-04 14:48:14 +0000
@@ -554,3 +554,24 @@
     vault.wire_transfer_inbound(package2.buffer_id());
     Mock::VerifyAndClearExpectations(&mock_requests);
 }
+
+//LP: #1578159
+TEST_F(StartedBufferVault, prefers_buffers_returned_further_in_the_past)
+{
+    auto buffer = vault.withdraw().get();
+    vault.deposit(buffer);
+    vault.wire_transfer_outbound(buffer);
+    auto first_id = buffer->rpc_id();
+
+    buffer = vault.withdraw().get();
+    vault.deposit(buffer);
+    vault.wire_transfer_outbound(buffer);
+    auto second_id = buffer->rpc_id();
+
+    vault.wire_transfer_inbound(second_id);
+    vault.wire_transfer_inbound(first_id);
+
+    EXPECT_THAT(vault.withdraw().get()->rpc_id(), Ne(first_id));
+
+
+}