Skip to content

Use thread-safe lock-free assignment in PackedVector::set_value#4199

Merged
oxidase merged 2 commits intomasterfrom
fix/packed_vector_datarace
Jun 27, 2017
Merged

Use thread-safe lock-free assignment in PackedVector::set_value#4199
oxidase merged 2 commits intomasterfrom
fix/packed_vector_datarace

Conversation

@oxidase
Copy link
Copy Markdown
Contributor

@oxidase oxidase commented Jun 26, 2017 •
edited by TheMarex
Loading

Issue

PR fixes #4110 and #4065 that is caused by data races in PackedVector::set_value
Lock-free compare_exchange_weak is implemented in <boost/atomic/detail/operations.hpp> as it is not possible to use atomic_compare_exchange_weak from <stdatomic.h> within C++

EDIT: the current approach requires to add Boost.Atomics as a new dependency, so other approaches that do not introduce a global vector lock are welcome

Tasklist

  • review
  • adjust for comments

@oxidase oxidase requested a review from TheMarex June 26, 2017 13:26
@TheMarex TheMarex added this to the 5.8.1 milestone Jun 26, 2017
@oxidase oxidase force-pushed the fix/packed_vector_datarace branch 2 times, most recently from 54630d5 to 19950b2 Compare June 27, 2017 08:43
daniel-j-h
daniel-j-h reviewed Jun 27, 2017
View reviewed changes
include/util/packed_vector.hpp
upper_offset[internal_index.element],
value);
// Lock-free update of the lower word
WordT local_lower_word, new_lower_word;
Copy link
Copy Markdown
Member

@daniel-j-h daniel-j-h Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WordT is a TBB typedef? Also is there a reason for not using the stdlib equivalents?

Copy link
Copy Markdown
Contributor Author

@oxidase oxidase Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WordT is an internal type alias. Please could you tell more about stdlib equivalents? So far i see only possible ways:

  • use sequential update
  • use an internal packed vector lock -> makes packed vector non-movable
  • use boost.atomic -> requires new dependency
  • use boost.interprocess atomics implementation -> outdated and only 32 bit version
  • use glib atomic's -> requires new dependency
  • use gcc __sync_bool_compare_and_swap and msvc _InterlockedCompareExchange64 -> possible, but requires proper testing
  • wait for https://isocpp.org/blog/2014/05/n4013 as_atomic
  • use c11 _Atomic -> not possible
  • use TBB atomic's -> uses TBB internal's

Copy link
Copy Markdown
Member

@daniel-j-h daniel-j-h Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urgh okay I see why you chose this solution now - thanks for the explanation! 🙇
Could you add this as a comment to the code please.

Copy link
Copy Markdown
Contributor Author

@oxidase oxidase Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniel-j-h i added a comment line about TBB atomics usage, but placed the options list in the git commit message

@oxidase oxidase force-pushed the fix/packed_vector_datarace branch from 19950b2 to 04740dc Compare June 27, 2017 09:14
TheMarex
TheMarex approved these changes Jun 27, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@TheMarex TheMarex left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using TBB internals here is preferable over the boost version. Thanks for digging. 👍 Could you also cherry-pick this to the 5.8 branch after merging?

oxidase added 2 commits June 27, 2017 12:18
@oxidase
Use thread-safe lock-free assignment in PackedVector::set_value
9f824de 
PR uses TBB internal atomic's for atomic CAS on non-atomic data

Corresponding PR #4199

Other options:

* use sequential update

* use an internal packed vector lock -> makes packed vector non-movable

* use boost.interprocess atomics implementation -> outdated and only 32 bit version

* use glib atomic's -> requires new dependency

*  wait for https://isocpp.org/blog/2014/05/n4013 as_atomic

*  use c11 _Atomic and atomic_compare_exchange_weak -> not possible to mix c++11 and c11

* use builtin functions gcc __sync_bool_compare_and_swap and msvc _InterlockedCompareExchange64 -> possible, but requires proper testing

boolean CompareAndSwapPointer(volatile * void * ptr,
                              void * new_value,
                              void * old_value) {
if defined(_MSC_VER)
   if (InterlockedCompareExchange(ptr, new_value, old_value) == old_value) return false;
   else return true;
elif (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) > 40100
   return __sync_bool_compare_and_swap(ptr, old_value, new_value);
else
  error No implementation
endif
}

* use Boost.Atomic -> requires new dependency

        WordT local_lower_word = lower_word, new_lower_word;
        do
        {
            new_lower_word = set_lower_value<WordT, T>(local_lower_word,
                                                       lower_mask[internal_index.element],
                                                       lower_offset[internal_index.element],
                                                       value);
        } while (!boost::atomics::detail::operations<sizeof(WordT), false>::compare_exchange_weak(
            lower_word,
            local_lower_word,
            new_lower_word,
            boost::memory_order_release,
            boost::memory_order_relaxed));
@oxidase
Re-enable edge-weight-updates-over-factor test
62a4556
@oxidase oxidase force-pushed the fix/packed_vector_datarace branch from 04740dc to 62a4556 Compare June 27, 2017 10:19
@oxidase oxidase merged commit 09df8d4 into master Jun 27, 2017
@oxidase oxidase deleted the fix/packed_vector_datarace branch June 27, 2017 11:02
oxidase added a commit that referenced this pull request Jun 27, 2017
@oxidase
Use thread-safe lock-free assignment in PackedVector::set_value
c8b75c9 
PR uses TBB internal atomic's for atomic CAS on non-atomic data

Corresponding PR #4199

Other options:

* use sequential update

* use an internal packed vector lock -> makes packed vector non-movable

* use boost.interprocess atomics implementation -> outdated and only 32 bit version

* use glib atomic's -> requires new dependency

*  wait for https://isocpp.org/blog/2014/05/n4013 as_atomic

*  use c11 _Atomic and atomic_compare_exchange_weak -> not possible to mix c++11 and c11

* use builtin functions gcc __sync_bool_compare_and_swap and msvc _InterlockedCompareExchange64 -> possible, but requires proper testing

boolean CompareAndSwapPointer(volatile * void * ptr,
                              void * new_value,
                              void * old_value) {
if defined(_MSC_VER)
   if (InterlockedCompareExchange(ptr, new_value, old_value) == old_value) return false;
   else return true;
elif (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) > 40100
   return __sync_bool_compare_and_swap(ptr, old_value, new_value);
else
  error No implementation
endif
}

* use Boost.Atomic -> requires new dependency

        WordT local_lower_word = lower_word, new_lower_word;
        do
        {
            new_lower_word = set_lower_value<WordT, T>(local_lower_word,
                                                       lower_mask[internal_index.element],
                                                       lower_offset[internal_index.element],
                                                       value);
        } while (!boost::atomics::detail::operations<sizeof(WordT), false>::compare_exchange_weak(
            lower_word,
            local_lower_word,
            new_lower_word,
            boost::memory_order_release,
            boost::memory_order_relaxed));
@oxidase oxidase mentioned this pull request Jun 27, 2017
Closed
oxidase added a commit that referenced this pull request Jun 27, 2017
@oxidase
Use thread-safe lock-free assignment in PackedVector::set_value
655d035 
PR uses TBB internal atomic's for atomic CAS on non-atomic data

Corresponding PR #4199

Other options:

* use sequential update

* use an internal packed vector lock -> makes packed vector non-movable

* use boost.interprocess atomics implementation -> outdated and only 32 bit version

* use glib atomic's -> requires new dependency

*  wait for https://isocpp.org/blog/2014/05/n4013 as_atomic

*  use c11 _Atomic and atomic_compare_exchange_weak -> not possible to mix c++11 and c11

* use builtin functions gcc __sync_bool_compare_and_swap and msvc _InterlockedCompareExchange64 -> possible, but requires proper testing

boolean CompareAndSwapPointer(volatile * void * ptr,
                              void * new_value,
                              void * old_value) {
if defined(_MSC_VER)
   if (InterlockedCompareExchange(ptr, new_value, old_value) == old_value) return false;
   else return true;
elif (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 + __GNUC_PATCHLEVEL__) > 40100
   return __sync_bool_compare_and_swap(ptr, old_value, new_value);
else
  error No implementation
endif
}

* use Boost.Atomic -> requires new dependency

        WordT local_lower_word = lower_word, new_lower_word;
        do
        {
            new_lower_word = set_lower_value<WordT, T>(local_lower_word,
                                                       lower_mask[internal_index.element],
                                                       lower_offset[internal_index.element],
                                                       value);
        } while (!boost::atomics::detail::operations<sizeof(WordT), false>::compare_exchange_weak(
            lower_word,
            local_lower_word,
            new_lower_word,
            boost::memory_order_release,
            boost::memory_order_relaxed));
@mjjbell mjjbell mentioned this pull request Aug 24, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daniel-j-h daniel-j-h daniel-j-h left review comments

@TheMarex TheMarex TheMarex approved these changes

Assignees

No one assigned

Labels

Bug Report Ready To Merge

Projects

None yet

Milestone

5.8.1

Development

Successfully merging this pull request may close these issues.

Traffic speed test depends on non-deterministic snapping behavior

3 participants

@oxidase @daniel-j-h @TheMarex