Mir

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

Proposed by Daniel van Vugt
Status: Merged
Approved by: Alan Griffiths
Approved revision: 4075
Merged at revision: 4081
Proposed branch: lp:~vanvugt/mir/fix-1671037
Merge into: lp:mir
Diff against target: 26 lines (+3/-5)
1 file modified
tests/unit-tests/test_posix_rw_mutex.cpp (+3/-5)
To merge this branch: bzr merge lp:~vanvugt/mir/fix-1671037
Reviewer Review Type Date Requested Status
Alan Griffiths Approve
Mir CI Bot continuous-integration Approve
Mir development team Pending
Review via email: mp+319297@code.launchpad.net

Commit message

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.

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

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)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

+ // else

Dead code in a comment?

review: Needs Fixing
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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).

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> 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
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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.

lp:~vanvugt/mir/fix-1671037 updated
4074. By Daniel van Vugt on 2017-03-13

Merge latest trunk

4075. By Daniel van Vugt on 2017-03-13

Shrink the diff and satisfy Alan's readability complaint.

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

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)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/unit-tests/test_posix_rw_mutex.cpp'
2--- tests/unit-tests/test_posix_rw_mutex.cpp 2016-10-20 06:56:17 +0000
3+++ tests/unit-tests/test_posix_rw_mutex.cpp 2017-03-13 04:15:45 +0000
4@@ -255,10 +255,6 @@
5 reader_changed.wait(l, [&reader_to_run, id]() { return reader_to_run == id; });
6 }
7 }
8-
9- std::this_thread::yield();
10-
11- trigger_next_reader();
12 }
13 else
14 {
15@@ -266,8 +262,10 @@
16 // some thread is waiting on an exclusive lock and we're preempted.
17 //
18 // In the latter case we need to let the waiting thread release its shared lock.
19- trigger_next_reader();
20 }
21+
22+ trigger_next_reader();
23+ std::this_thread::yield();
24 }
25 },
26 i};

Subscribers

People subscribed via source and target branches