Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use shared monitor with two interprocess_semaphore's #3648

Merged
merged 3 commits into from
Feb 24, 2017
Merged

Use shared monitor with two interprocess_semaphore's #3648

merged 3 commits into from
Feb 24, 2017

Conversation

oxidase
Copy link
Contributor

@oxidase oxidase commented Feb 3, 2017 •
edited
Loading

Issue

One possible solution for #3633 is to use interprocess_semaphore's to model a conditional variable.

PR works for linux, windows, and OSX, but hits https://svn.boost.org/trac/boost/ticket/12617 boost bug on OS X 10.11. it can be fixed by the patch.

diff --git a/include/boost/interprocess/detail/os_thread_functions.hpp b/include/boost/interprocess/detail/os_thread_functions.hpp index 291f426..bd50fd9 100644 
--- a/include/boost/interprocess/detail/os_thread_functions.hpp
+++ b/include/boost/interprocess/detail/os_thread_functions.hpp
@@ -53,7 +53,11 @@
 #     include <sys/sysctl.h>
 #  endif
 //According to the article "C/C++ tip: How to measure elapsed real time for benchmarking"
-#  if defined(CLOCK_MONOTONIC_PRECISE)   //BSD
+
+#  if (defined(macintosh) || defined(__APPLE__) || defined(__APPLE_CC__))
+#     include <mach/mach_time.h>  // mach_absolute_time, mach_timebase_info_data_t
+#     define BOOST_INTERPROCESS_MATCH_ABSOLUTE_TIME
+#  elif defined(CLOCK_MONOTONIC_PRECISE) //BSD
 #     define BOOST_INTERPROCESS_CLOCK_MONOTONIC CLOCK_MONOTONIC_PRECISE
 #  elif defined(CLOCK_MONOTONIC_RAW)     //Linux
 #     define BOOST_INTERPROCESS_CLOCK_MONOTONIC CLOCK_MONOTONIC_RAW
@@ -61,9 +65,9 @@
 #     define BOOST_INTERPROCESS_CLOCK_MONOTONIC CLOCK_HIGHRES
 #  elif defined(CLOCK_MONOTONIC)         //POSIX (AIX, BSD, Linux, Solaris)
 #     define BOOST_INTERPROCESS_CLOCK_MONOTONIC CLOCK_MONOTONIC
-#  elif !defined(CLOCK_MONOTONIC) && (defined(macintosh) || defined(__APPLE__) || defined(__APPLE_CC__))
-#     include <mach/mach_time.h>  // mach_absolute_time, mach_timebase_info_data_t
-#     define BOOST_INTERPROCESS_MATCH_ABSOLUTE_TIME
+// #  elif !defined(CLOCK_MONOTONIC) && (defined(macintosh) || defined(__APPLE__) || defined(__APPLE_CC__))
+// #     include <mach/mach_time.h>  // mach_absolute_time, mach_timebase_info_data_t
+// #     define BOOST_INTERPROCESS_MATCH_ABSOLUTE_TIME
 #  else
 #     error "No high resolution steady clock in your system, please provide a patch"
 #  endif

/cc @danpat, @TheMarex

Tasklist

  • review
  • adjust for comments

Related:
#1221, #3558

@oxidase oxidase force-pushed the refactor/shared-monitor branch from 5713355 to 981639d Compare February 3, 2017 22:03
@oxidase
Copy link
Contributor Author

oxidase commented Feb 5, 2017

at the moment the solution can lead to a deadlock, because if the waiter is killed the waiters number will not be decremented

@oxidase oxidase added this to the 5.7.0 milestone Feb 10, 2017
@oxidase oxidase force-pushed the refactor/shared-monitor branch 2 times, most recently from 5c1dbd9 to 44ddf49 Compare February 13, 2017 10:55
@oxidase
Copy link
Contributor Author

oxidase commented Feb 13, 2017

After some attempts to implement a conditional variable via semaphores and waiters counter that can handled killed waiters, just get to similar as https://github.com/boostorg/interprocess/blob/develop/include/boost/interprocess/sync/spin/condition.hpp

My suggest is not to use NIH implementation and fallback to the spinlock-based conditional variable via BOOST_INTERPROCESS_FORCE_GENERIC_EMULATION on OSX.

@oxidase
Copy link
Contributor Author

oxidase commented Feb 13, 2017 •
edited
Loading

checked boost interprocess conditional variables on windows and osx: non-robust against application kills. will fallback on windows and osx to a solution with one semaphore that generates spurious wake ups for every killed osrm-routed process.

@oxidase oxidase force-pushed the refactor/shared-monitor branch 3 times, most recently from eb504d4 to 215d7ce Compare February 14, 2017 07:34
@oxidase oxidase force-pushed the refactor/shared-monitor branch from 215d7ce to 0f2d93b Compare February 14, 2017 08:14
@oxidase
Copy link
Contributor Author

oxidase commented Feb 14, 2017

node-osrm tests also pass on OS X 10.11.6

@oxidase oxidase force-pushed the refactor/shared-monitor branch 3 times, most recently from 5e966f1 to a97d2b9 Compare February 14, 2017 13:29
TheMarex
TheMarex requested changes Feb 20, 2017
View reviewed changes
Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

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

Some questions regarding readability, approach is fine. 👍

include/storage/shared_monitor.hpp Outdated
#else
template <typename Lock> void wait(Lock &lock)
{
if (((internal().head + 1) & (buffer_size - 1)) == (internal().tail & (buffer_size - 1)))
Copy link
Member

Choose a reason for hiding this comment

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

What is the advantage of using & (buffer_size - 1) instead of % buffer_size? I'm more familiar with the latter when it comes to wrap-a-round indexing.

Wrapping the ring buffer in smaller helper functions to hide the index "magic" might improve readability.

include/storage/shared_monitor.hpp Outdated
while (internal().tail != internal().head)
{
auto index = (internal().tail++) & (buffer_size - 1);
auto semaphore = reinterpret_cast<bi::interprocess_semaphore *>(
Copy link
Member

Choose a reason for hiding this comment

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

Same here, getting the semaphore could be hidden behind a nicer interface.

include/storage/shared_monitor.hpp Outdated

std::size_t head, tail;
mutex_type mutex;
char buffer[buffer_size * sizeof(bi::interprocess_semaphore)];
Copy link
Member

Choose a reason for hiding this comment

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

This could warrant a little bit of an explanation of what we are going with these semaphore: Implementing a conditional variable using a queue of semaphores.

@oxidase oxidase force-pushed the refactor/shared-monitor branch from a97d2b9 to 8194776 Compare February 24, 2017 13:09
@oxidase
Copy link
Contributor Author

oxidase commented Feb 24, 2017

@TheMarex i have moved queue functionality into the internal data structure. It is still has to be checked on OSX

@oxidase oxidase force-pushed the refactor/shared-monitor branch from 8194776 to 928e8fd Compare February 24, 2017 13:23
@oxidase oxidase force-pushed the refactor/shared-monitor branch from 928e8fd to e56a0b9 Compare February 24, 2017 13:36
@TheMarex
Copy link
Member

@oxidase Looks good to me. 👍 Seems to work on travis, we should make @danpat let it run on his computer though. 😉

@TheMarex TheMarex merged commit cd1a73d into master Feb 24, 2017
@TheMarex TheMarex deleted the refactor/shared-monitor branch February 24, 2017 16:02
@oxidase
Copy link
Contributor Author

oxidase commented Feb 24, 2017

Checked node-osrm with the master branch -> tests with multiple Engines pass! Please don't forget to run osrm-datastore --spring-clean after building master 😺

This was referenced Feb 24, 2017
Closed
Closed
@oxidase oxidase mentioned this pull request Mar 29, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheMarex TheMarex TheMarex approved these changes

Assignees
No one assigned
Labels
Ready To Merge
Projects
None yet
Milestone
5.7.0
Development

Successfully merging this pull request may close these issues.
2 participants
@oxidase @TheMarex