From 526f87dbdb35816815625fbff4aa2455038d73ca Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Sun, 21 Jan 2018 11:34:02 -0600 Subject: [PATCH 1/8] WIP implement stand::post --- libstdc++-v3/include/experimental/executor | 49 ++++++++++++++++++++-- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/libstdc++-v3/include/experimental/executor b/libstdc++-v3/include/experimental/executor index 480d10d150450..656250811c7ec 100644 --- a/libstdc++-v3/include/experimental/executor +++ b/libstdc++-v3/include/experimental/executor @@ -1433,7 +1433,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static_assert(is_convertible<_OtherExecutor, _Executor>::value, "inner executor type must be compatible"); - // TODO move state + _M_state = std::move(__other._M_state); _M_inner_ex = std::move(__other._M_inner_ex); return *this; } @@ -1473,7 +1473,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template void - post(_Func&& __f, const _Alloc& __a) const; // TODO + post(_Func&& __f, const _Alloc& __a) const + { + if (!_M_state) + throw bad_executor(); + auto& __state = *_M_state; + unique_lock __lock(__state._M_mtx); + __state._M_tasks.push(std::move(__f)); // XXX allocator not used + if (!__state._M_scheduled) + { + __state._M_scheduled = true; + _M_inner_ex.post(_Runnable{_M_state, _M_inner_ex}); + } + } template void @@ -1485,10 +1497,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION operator==(const strand& __a, const strand& __b) { return __a._M_state == __b._M_state; } - // TODO add synchronised queue struct _State { - std::thread::id _M_running_on; + std::thread::id _M_running_on; + mutable mutex _M_mtx; + queue> _M_tasks; + bool _M_scheduled; + }; + + struct _Runnable + { + shared_ptr<_State> _M_state; + _Executor _M_inner_ex; + + void operator()() + { + function __f; + auto& __state = *_M_state; + bool __empty; + { + unique_lock __lock(__state._M_mtx); + __f = std::move(__state._M_tasks.front()); + __state._M_tasks.pop(); + } + + __f(); + + { + unique_lock __lock(__state._M_mtx); + __state._M_scheduled = !__state._M_tasks.empty(); + if (__state._M_scheduled) + _M_inner_ex.post(*this); + } + } }; shared_ptr<_State> _M_state; _Executor _M_inner_ex; From 5d6e6013317d2756109a6d2cb8533948548ede8f Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Sun, 21 Jan 2018 11:51:05 -0600 Subject: [PATCH 2/8] WIP Cleaning up strand::_Runnable implementation --- libstdc++-v3/include/experimental/executor | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/libstdc++-v3/include/experimental/executor b/libstdc++-v3/include/experimental/executor index 656250811c7ec..86263264def14 100644 --- a/libstdc++-v3/include/experimental/executor +++ b/libstdc++-v3/include/experimental/executor @@ -1519,18 +1519,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION unique_lock __lock(__state._M_mtx); __f = std::move(__state._M_tasks.front()); __state._M_tasks.pop(); + __empty = __state._M_tasks.empty(); + __state._M_scheduled = !__empty; } __f(); - { - unique_lock __lock(__state._M_mtx); - __state._M_scheduled = !__state._M_tasks.empty(); - if (__state._M_scheduled) + if (!__empty) _M_inner_ex.post(*this); - } } }; + shared_ptr<_State> _M_state; _Executor _M_inner_ex; }; From d9b6db136269e55731894eed0b781392f77cbcc3 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Sun, 21 Jan 2018 14:43:46 -0600 Subject: [PATCH 3/8] Unwinding previous change Introduced a race condition --- libstdc++-v3/include/experimental/executor | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/libstdc++-v3/include/experimental/executor b/libstdc++-v3/include/experimental/executor index 86263264def14..f5b5386f1fcc0 100644 --- a/libstdc++-v3/include/experimental/executor +++ b/libstdc++-v3/include/experimental/executor @@ -1477,13 +1477,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { if (!_M_state) throw bad_executor(); + auto& __state = *_M_state; - unique_lock __lock(__state._M_mtx); + unique_lock __lock(__state._M_mtx); __state._M_tasks.push(std::move(__f)); // XXX allocator not used if (!__state._M_scheduled) { __state._M_scheduled = true; - _M_inner_ex.post(_Runnable{_M_state, _M_inner_ex}); + _M_inner_ex.post(_Runnable(_M_state, _M_inner_ex)); } } @@ -1499,8 +1500,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION struct _State { - std::thread::id _M_running_on; - mutable mutex _M_mtx; + std::thread::id _M_running_on; + mutable mutex _M_mtx; queue> _M_tasks; bool _M_scheduled; }; @@ -1514,19 +1515,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { function __f; auto& __state = *_M_state; - bool __empty; { unique_lock __lock(__state._M_mtx); __f = std::move(__state._M_tasks.front()); __state._M_tasks.pop(); - __empty = __state._M_tasks.empty(); - __state._M_scheduled = !__empty; } __f(); - if (!__empty) + { + unique_lock __lock(__state._M_mtx); + __state._M_scheduled = !__state._M_tasks.empty(); + if (__state._M_scheduled) _M_inner_ex.post(*this); + } } }; From d3d7e99305dea9964aacb10bfd9ea6b535628e57 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Sun, 21 Jan 2018 15:42:56 -0600 Subject: [PATCH 4/8] WIP Updating strand ctors, dtor, running_in_this_thread() --- libstdc++-v3/include/experimental/executor | 43 ++++++++++++---------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/libstdc++-v3/include/experimental/executor b/libstdc++-v3/include/experimental/executor index f5b5386f1fcc0..96c841dc7e1c4 100644 --- a/libstdc++-v3/include/experimental/executor +++ b/libstdc++-v3/include/experimental/executor @@ -1366,13 +1366,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // construct / copy / destroy: - strand(); // TODO make state + strand() : _M_state(make_shared<_State>()) { } - explicit strand(_Executor __ex) : _M_inner_ex(__ex) { } // TODO make state + explicit strand(_Executor __ex) + : _M_state(make_shared<_State>()), + _M_inner_ex(__ex) { } template strand(allocator_arg_t, const _Alloc& __a, _Executor __ex) - : _M_inner_ex(__ex) { } // TODO make state + : _M_state(make_shared<_State>()), + _M_inner_ex(__ex) { } strand(const strand& __other) noexcept : _M_state(__other._M_state), _M_inner_ex(__other._M_inner_ex) { } @@ -1396,8 +1399,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static_assert(is_copy_assignable<_Executor>::value, "inner executor type must be CopyAssignable"); - // TODO lock __other - // TODO copy state + _M_state = __other._M_state; _M_inner_ex = __other._M_inner_ex; return *this; } @@ -1408,7 +1410,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static_assert(is_move_assignable<_Executor>::value, "inner executor type must be MoveAssignable"); - // TODO move state + _M_state = std::move(__other._M_state); _M_inner_ex = std::move(__other._M_inner_ex); return *this; } @@ -1420,8 +1422,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION static_assert(is_convertible<_OtherExecutor, _Executor>::value, "inner executor type must be compatible"); - // TODO lock __other - // TODO copy state + _M_state = __other._M_state; _M_inner_ex = __other._M_inner_ex; return *this; } @@ -1438,12 +1439,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return *this; } - ~strand() - { - // the task queue outlives this object if non-empty - // TODO create circular ref in queue? - } - // strand operations: inner_executor_type @@ -1452,7 +1447,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION bool running_in_this_thread() const noexcept - { return std::this_thread::get_id() == _M_state->_M_running_on; } + { return _M_state ? _M_state->running_in_this_thread() : false; } execution_context& context() const noexcept @@ -1500,10 +1495,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION struct _State { - std::thread::id _M_running_on; - mutable mutex _M_mtx; - queue> _M_tasks; - bool _M_scheduled; + mutable mutex _M_mtx; + bool _M_scheduled; + queue> _M_tasks; + std::thread::id _M_running_on; + + _State() : _M_scheduled(false) + {} + + bool running_in_this_thread() const noexcept + { + unique_lock __lock(_M_mtx); + return _M_scheduled && std::this_thread::get_id() == _M_running_on; + } }; struct _Runnable @@ -1519,6 +1523,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION unique_lock __lock(__state._M_mtx); __f = std::move(__state._M_tasks.front()); __state._M_tasks.pop(); + __state._M_running_on = std::this_thread::get_id(); } __f(); From a67f1439375211bfb7a598439541c1b238fc6cb6 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Sun, 21 Jan 2018 16:14:28 -0600 Subject: [PATCH 5/8] WIP Update _M_running_on each time _Runnable runs --- libstdc++-v3/include/experimental/executor | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libstdc++-v3/include/experimental/executor b/libstdc++-v3/include/experimental/executor index 96c841dc7e1c4..b53883a064cbe 100644 --- a/libstdc++-v3/include/experimental/executor +++ b/libstdc++-v3/include/experimental/executor @@ -1532,7 +1532,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION unique_lock __lock(__state._M_mtx); __state._M_scheduled = !__state._M_tasks.empty(); if (__state._M_scheduled) - _M_inner_ex.post(*this); + _M_inner_ex.post(std::move(*this)); } } }; From 5ea764abeabdb9c462d52ad2fe2b4c8a26e1e8f2 Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Sun, 21 Jan 2018 16:20:11 -0600 Subject: [PATCH 6/8] WIP Move post() outside of lock in strand::_Runnable --- libstdc++-v3/include/experimental/executor | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/libstdc++-v3/include/experimental/executor b/libstdc++-v3/include/experimental/executor index b53883a064cbe..b0c96beea2ed6 100644 --- a/libstdc++-v3/include/experimental/executor +++ b/libstdc++-v3/include/experimental/executor @@ -1528,12 +1528,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __f(); + bool __reschedule; { unique_lock __lock(__state._M_mtx); - __state._M_scheduled = !__state._M_tasks.empty(); - if (__state._M_scheduled) - _M_inner_ex.post(std::move(*this)); + __reschedule = __state._M_scheduled = !__state._M_tasks.empty(); } + + if (__reschedule) + _M_inner_ex.post(std::move(*this)); } }; From 3e61e0b831db0cc8c3a5bc40d462ef6f219ef02c Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Sun, 21 Jan 2018 16:46:14 -0600 Subject: [PATCH 7/8] WIP Narrow scope of lock in strand::post() Move post of runnable to underlying executor outside lock --- libstdc++-v3/include/experimental/executor | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/libstdc++-v3/include/experimental/executor b/libstdc++-v3/include/experimental/executor index b0c96beea2ed6..4c4e60335d51b 100644 --- a/libstdc++-v3/include/experimental/executor +++ b/libstdc++-v3/include/experimental/executor @@ -1473,14 +1473,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (!_M_state) throw bad_executor(); - auto& __state = *_M_state; - unique_lock __lock(__state._M_mtx); - __state._M_tasks.push(std::move(__f)); // XXX allocator not used - if (!__state._M_scheduled) + bool __schedule = false; { - __state._M_scheduled = true; - _M_inner_ex.post(_Runnable(_M_state, _M_inner_ex)); + auto& __state = *_M_state; + unique_lock __lock(__state._M_mtx); + __state._M_tasks.push(std::move(__f)); // XXX allocator not used + if (!__state._M_scheduled) { + __schedule = __state._M_scheduled = true; + } } + + if (__schedule) + _M_inner_ex.post(_Runnable(_M_state, _M_inner_ex)); } template From 47ec4ecb3dff0d6e12222c1546b80d99a2cc4edd Mon Sep 17 00:00:00 2001 From: Thomas Rodgers Date: Mon, 22 Jan 2018 11:55:04 -0600 Subject: [PATCH 8/8] Remove local state refs --- libstdc++-v3/include/experimental/executor | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/libstdc++-v3/include/experimental/executor b/libstdc++-v3/include/experimental/executor index 4c4e60335d51b..09357d71c3b80 100644 --- a/libstdc++-v3/include/experimental/executor +++ b/libstdc++-v3/include/experimental/executor @@ -1475,10 +1475,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION bool __schedule = false; { - auto& __state = *_M_state; - unique_lock __lock(__state._M_mtx); - __state._M_tasks.push(std::move(__f)); // XXX allocator not used - if (!__state._M_scheduled) { + unique_lock __lock(_M_state->_M_mtx); + _M_state->_M_tasks.push(std::move(__f)); // XXX allocator not used + if (!_M_state->_M_scheduled) { __schedule = __state._M_scheduled = true; } } @@ -1522,12 +1521,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION void operator()() { function __f; - auto& __state = *_M_state; { - unique_lock __lock(__state._M_mtx); - __f = std::move(__state._M_tasks.front()); - __state._M_tasks.pop(); - __state._M_running_on = std::this_thread::get_id(); + unique_lock __lock(_M_state->_M_mtx); + __f = std::move(_M_state->_M_tasks.front()); + _M_state->_M_tasks.pop(); + _M_state->_M_running_on = std::this_thread::get_id(); } __f();