Mir

Merge lp://qastaging/~raof/mir/scratch-threadsafe-list-itch into lp://qastaging/mir

  1. scratch-threadsafe-list-itch
  2. Merge into development-branch
Proposed by Chris Halse Rogers
Status: Work in progress
Proposed branch: lp://qastaging/~raof/mir/scratch-threadsafe-list-itch
Merge into: lp://qastaging/mir
Diff against target: 294 lines (+81/-100)
3 files modified
src/include/common/mir/thread_safe_list.h (+73/-98)
tests/unit-tests/scene/test_timeout_application_not_responding_detector.cpp (+2/-0)
tests/unit-tests/test_thread_safe_list.cpp (+6/-2)
To merge this branch: bzr merge lp://qastaging/~raof/mir/scratch-threadsafe-list-itch
Reviewer Review Type Date Requested Status
Alan Griffiths Needs Fixing
Alexandros Frantzis (community) Needs Information
Andreas Pokorny (community) Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
Review via email: mp+275157@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2015-10-21.

Commit message

Simplify ThreadSafeList (by relaxing an invariant).

It's almost never a good idea to modify the list as you're iterating over it.
Where it *is* sensible, being able to remove yourself from subsequent runs is good enough."

Description of the change

Scratch my threadsafe-list itch.Simplify ThreadSafeList (by relaxing an invariant).

I find this substantially easier to reason about, and it's almost certainly faster for the common case of iterate-often, mutate seldom.

To post a comment you must log in.
Revision history for this message
Chris Halse Rogers (raof) wrote : Posted in a previous version of this proposal

Bah! Unrelated changes!

Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote : Posted in a previous version of this proposal

I think you could avoid a copy in the mutating functions by testing use_count == 1 after acquiring the lock.

lgtm.

review: Needs Information
Revision history for this message
Chris Halse Rogers (raof) wrote :

There you go; now with optimised no-contention path.

Reply
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:3045
http://jenkins.qa.ubuntu.com/job/mir-ci/5353/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4566/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3473
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4508/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/466/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1507
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1507/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/466
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/466/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4509/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/467/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5353/rebuild

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

looks sensible

review: Approve
Reply
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

I don't recall the details, but I think that we had a good reason for wanting fine-granularity locking. Alan?

review: Needs Information
Reply
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Not accessing an element after it has been removed is an important guarantee and was tested by ThreadSafeListTest, can_remove_unused_element_while_iterating_different_element and ThreadSafeListTest.can_remove_unused_element_while_different_element_is_used_in_different_thread.

At a first look it seems that after the changes these tests no longer checks that the iteration (the loop in the first test and thread t in the second) does not access element2 after it has been removed.

review: Needs Fixing
Reply
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

For a history of why ThreadSafeList has the (quite irritating) invariants it does see: http://accu.org/index.php/journals/2040

Reply
Revision history for this message
Chris Halse Rogers (raof) wrote :

Ah, yes, that's right. There's this little gem in surface_input_dispatcher.cpp, among others, that this doesn't deal with.

mi::SurfaceInputDispatcher::SurfaceInputDispatcher(std::shared_ptr<mi::Scene> const& scene)
    : scene(scene),
      started(false)
{
    scene_observer = std::make_shared<InputDispatcherSceneObserver>([this](ms::Surface* s){su
rface_removed(s);});
    scene->add_observer(scene_observer);
}

mi::SurfaceInputDispatcher::~SurfaceInputDispatcher()
{
    scene->remove_observer(scene_observer);
}

Reply

Unmerged revisions

3045. By Chris Halse Rogers

Optimise the no-contention case for ThreadSafeList mutation

3044. By Chris Halse Rogers

Simplify ThreadSafeList (by relaxing an invariant).

It's almost never a good idea to modify the list as you're iterating over it.
Where it *is* sensible, being able to remove yourself from subsequent runs
is good enough.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches