Merge lp:~vanvugt/mir/fix-1671037 into lp:mir

[Daniel van Vugt]

Yield on every loop iteration because under valgrind threads are
virtualized and fair scheduling does not exist. (LP: #1671037)

Without this the test takes up to 10 minutes under valgrind and fails.
With this change it takes under 100 milliseconds to pass.

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

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

+ // else

Dead code in a comment?

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Incorrect. That comment is correct.

"// else" is to make it clear that the larger comment below it applies to the else case (which now has no code).

[Alan Griffiths (alan-griffiths)]

> Incorrect. That comment is correct.
>
> "// else" is to make it clear that the larger comment below it applies to the
> else case (which now has no code).

You might believe that is clear, I doubt anyone else will read it that way.

If the comment is needed in the else case why not simply retain the block?

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Because the block is empty. This is common practice, as I've been doing it for years in the Mir code and nobody complained before.

However it's silly that we should be spending time discussing this at all. I'll just change the style to whatever you want to get it landed.

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

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I don't see it in the Mir codebase. But that doesn't matter.

review: Approve

=== modified file 'tests/unit-tests/test_posix_rw_mutex.cpp'
--- tests/unit-tests/test_posix_rw_mutex.cpp	2016-10-20 06:56:17 +0000
+++ tests/unit-tests/test_posix_rw_mutex.cpp	2017-03-13 04:15:45 +0000
@@ -255,10 +255,6 @@
                                 reader_changed.wait(l, [&reader_to_run, id]() { return reader_to_run == id; });
                             }
                         }
-
-                        std::this_thread::yield();
-
-                        trigger_next_reader();
                     }
                     else
                     {
@@ -266,8 +262,10 @@
                         // some thread is waiting on an exclusive lock and we're preempted.
                         //
                         // In the latter case we need to let the waiting thread release its shared lock.
-                        trigger_next_reader();
                     }
+
+                    trigger_next_reader();
+                    std::this_thread::yield();
                 }
             },
             i};