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

mutex.cpp, cond.cpp: Use static dispatch #3770

Merged
merged 13 commits into from
Jun 22, 2023
Merged

mutex.cpp, cond.cpp: Use static dispatch #3770

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
24922f3
`mutex.cpp`, `cond.cpp`: Use static dispatch
cpplearner Jun 12, 2023
e952e5a
Create non-polymorphic classes
cpplearner Jun 14, 2023
c3114d1
Alternative approach
cpplearner Jun 15, 2023
a212a04
Merge branch 'main' into static-dispatch
StephanTLavavej Jun 15, 2023
13f90f1
`TRANSITON` => `TRANSITION`
StephanTLavavej Jun 16, 2023
fd49a2f
When `wait` calls `wait_for`, we don't need to qualify it anymore.
StephanTLavavej Jun 16, 2023
3698785
Remove `destroy` for `stl_critical_section_win7` and `stl_condition_v…
StephanTLavavej Jun 16, 2023
5365632
Drop `stl_critical_section_win7::try_lock_for`, directly call `try_lo…
StephanTLavavej Jun 16, 2023
589f8d7
`=delete` `native_handle()` for the `mutex` family and `condition_var…
StephanTLavavej Jun 16, 2023
0578610
Remove test coverage for deleted native_handle().
StephanTLavavej Jun 17, 2023
34fdc17
We don't actually need vptr placeholders.
StephanTLavavej Jun 21, 2023
a62223f
Revert "We don't actually need vptr placeholders."
StephanTLavavej Jun 21, 2023
f3499b7
Remove native_handle_type and native_handle().
StephanTLavavej Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions stl/inc/mutex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,8 @@ public:

using native_handle_type = void*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still declare the native_handle_type even if native_handle is not usable? I guess this is technically conforming, but looks weird.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's an open question.

Copy link
Contributor

Choose a reason for hiding this comment

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

[thread.req.native]/1:

Several classes described in this Clause have members native_handle_type and native_handle. The presence of these members and their semantics is implementation-defined."

We can do whatever we like here, if we document it. I'd prefer for native_handle_type to be undefined or denote void, but we can debate it after merging this change.

Whatever we do, we need to update https://learn.microsoft.com/en-us/cpp/standard-library/mutex-class-stl?view=msvc-170#native_handle.


_NODISCARD native_handle_type native_handle() noexcept /* strengthened */ {
return _Mtx_getconcrtcs(_Mymtx());
}
// TRANSITION, native_handle() could return a pointer to the underlying SRWLOCK
StephanTLavavej marked this conversation as resolved.
Show resolved Hide resolved
_NODISCARD native_handle_type native_handle() noexcept /* strengthened */ = delete;

protected:
_NODISCARD_TRY_CHANGE_STATE bool _Verify_ownership_levels() noexcept {
Expand Down Expand Up @@ -636,8 +635,6 @@ _EXPORT_STD enum class cv_status { // names for wait returns

_EXPORT_STD class condition_variable { // class for waiting for conditions
public:
using native_handle_type = _Cnd_t;

condition_variable() noexcept /* strengthened */ {
_Cnd_init_in_situ(_Mycnd());
}
Expand Down Expand Up @@ -725,9 +722,10 @@ public:
return _Wait_until1(_Lck, _Abs_time, _Pred);
}

_NODISCARD native_handle_type native_handle() noexcept /* strengthened */ {
return _Mycnd();
}
using native_handle_type = void*;

// TRANSITION, native_handle() could return a pointer to the underlying CONDITION_VARIABLE
_NODISCARD native_handle_type native_handle() noexcept /* strengthened */ = delete;

void _Register(unique_lock<mutex>& _Lck, int* _Ready) noexcept { // register this object for release at thread exit
_Cnd_register_at_thread_exit(_Mycnd(), _Lck.release()->_Mymtx(), _Ready);
Expand Down
12 changes: 5 additions & 7 deletions stl/src/cond.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ struct _Cnd_internal_imp_t { // condition variable implementation for ConcRT
typename std::_Aligned_storage<Concurrency::details::stl_condition_variable_max_size,
Concurrency::details::stl_condition_variable_max_alignment>::type cv;

[[nodiscard]] Concurrency::details::stl_condition_variable_interface* _get_cv() noexcept {
[[nodiscard]] Concurrency::details::stl_condition_variable_win7* _get_cv() noexcept {
// get pointer to implementation
return reinterpret_cast<Concurrency::details::stl_condition_variable_interface*>(&cv);
return reinterpret_cast<Concurrency::details::stl_condition_variable_win7*>(&cv);
}
};

Expand All @@ -28,9 +28,7 @@ void _Cnd_init_in_situ(const _Cnd_t cond) { // initialize condition variable in
Concurrency::details::create_stl_condition_variable(cond->_get_cv());
}

void _Cnd_destroy_in_situ(const _Cnd_t cond) { // destroy condition variable in situ
cond->_get_cv()->destroy();
}
void _Cnd_destroy_in_situ(_Cnd_t) {} // destroy condition variable in situ

int _Cnd_init(_Cnd_t* const pcond) { // initialize
*pcond = nullptr;
Expand All @@ -53,7 +51,7 @@ void _Cnd_destroy(const _Cnd_t cond) { // clean up
}

int _Cnd_wait(const _Cnd_t cond, const _Mtx_t mtx) { // wait until signaled
const auto cs = static_cast<Concurrency::details::stl_critical_section_interface*>(_Mtx_getconcrtcs(mtx));
const auto cs = static_cast<Concurrency::details::stl_critical_section_win7*>(_Mtx_getconcrtcs(mtx));
_Mtx_clear_owner(mtx);
cond->_get_cv()->wait(cs);
_Mtx_reset_owner(mtx);
Expand All @@ -63,7 +61,7 @@ int _Cnd_wait(const _Cnd_t cond, const _Mtx_t mtx) { // wait until signaled
// wait until signaled or timeout
int _Cnd_timedwait(const _Cnd_t cond, const _Mtx_t mtx, const _timespec64* const target) {
int res = _Thrd_success;
const auto cs = static_cast<Concurrency::details::stl_critical_section_interface*>(_Mtx_getconcrtcs(mtx));
const auto cs = static_cast<Concurrency::details::stl_critical_section_win7*>(_Mtx_getconcrtcs(mtx));
if (target == nullptr) { // no target time specified, wait on mutex
_Mtx_clear_owner(mtx);
cond->_get_cv()->wait(cs);
Expand Down
8 changes: 4 additions & 4 deletions stl/src/mutex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ struct _Mtx_internal_imp_t { // ConcRT mutex
Concurrency::details::stl_critical_section_max_alignment>::type cs;
long thread_id;
int count;
Concurrency::details::stl_critical_section_interface* _get_cs() { // get pointer to implementation
return reinterpret_cast<Concurrency::details::stl_critical_section_interface*>(&cs);
[[nodiscard]] Concurrency::details::stl_critical_section_win7* _get_cs() { // get pointer to implementation
return reinterpret_cast<Concurrency::details::stl_critical_section_win7*>(&cs);
}
};

Expand All @@ -65,7 +65,7 @@ void _Mtx_init_in_situ(_Mtx_t mtx, int type) { // initialize mutex in situ

void _Mtx_destroy_in_situ(_Mtx_t mtx) { // destroy mutex in situ
_THREAD_ASSERT(mtx->count == 0, "mutex destroyed while busy");
mtx->_get_cs()->destroy();
(void) mtx;
}

int _Mtx_init(_Mtx_t* mtx, int type) { // initialize mutex
Expand Down Expand Up @@ -126,7 +126,7 @@ static int mtx_do_lock(_Mtx_t mtx, const _timespec64* target) { // lock mutex
while (now.tv_sec < target->tv_sec || now.tv_sec == target->tv_sec && now.tv_nsec < target->tv_nsec) {
// time has not expired
if (mtx->thread_id == static_cast<long>(GetCurrentThreadId())
|| mtx->_get_cs()->try_lock_for(_Xtime_diff_to_millis2(target, &now))) { // stop waiting
|| mtx->_get_cs()->try_lock()) { // stop waiting
res = WAIT_OBJECT_0;
break;
} else {
Expand Down
61 changes: 15 additions & 46 deletions stl/src/primitives.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,48 +10,23 @@

namespace Concurrency {
namespace details {
class __declspec(novtable) stl_critical_section_interface {
public:
virtual void lock() = 0;
virtual bool try_lock() = 0;
virtual bool try_lock_for(unsigned int) = 0;
virtual void unlock() = 0;
virtual void destroy() = 0;
};

class __declspec(novtable) stl_condition_variable_interface {
public:
virtual void wait(stl_critical_section_interface*) = 0;
virtual bool wait_for(stl_critical_section_interface*, unsigned int) = 0;
virtual void notify_one() = 0;
virtual void notify_all() = 0;
virtual void destroy() = 0;
};

class stl_critical_section_win7 final : public stl_critical_section_interface {
class stl_critical_section_win7 {
public:
stl_critical_section_win7() = default;

~stl_critical_section_win7() = delete;
stl_critical_section_win7(const stl_critical_section_win7&) = delete;
stl_critical_section_win7& operator=(const stl_critical_section_win7&) = delete;

void destroy() override {}

void lock() override {
void lock() {
AcquireSRWLockExclusive(&m_srw_lock);
}

bool try_lock() override {
bool try_lock() {
return TryAcquireSRWLockExclusive(&m_srw_lock) != 0;
}

bool try_lock_for(unsigned int) override {
// STL will call try_lock_for once again if this call will not succeed
return stl_critical_section_win7::try_lock();
}

void unlock() override {
void unlock() {
_Analysis_assume_lock_held_(m_srw_lock);
ReleaseSRWLockExclusive(&m_srw_lock);
}
Expand All @@ -64,47 +39,41 @@ namespace Concurrency {
SRWLOCK m_srw_lock = SRWLOCK_INIT;
};

class stl_condition_variable_win7 final : public stl_condition_variable_interface {
class stl_condition_variable_win7 {
public:
stl_condition_variable_win7() {
InitializeConditionVariable(&m_condition_variable);
}
stl_condition_variable_win7() = default;

~stl_condition_variable_win7() = delete;
stl_condition_variable_win7(const stl_condition_variable_win7&) = delete;
stl_condition_variable_win7& operator=(const stl_condition_variable_win7&) = delete;

void destroy() override {}

void wait(stl_critical_section_interface* lock) override {
if (!stl_condition_variable_win7::wait_for(lock, INFINITE)) {
void wait(stl_critical_section_win7* lock) {
if (!wait_for(lock, INFINITE)) {
std::terminate();
}
}

bool wait_for(stl_critical_section_interface* lock, unsigned int timeout) override {
return SleepConditionVariableSRW(&m_condition_variable,
static_cast<stl_critical_section_win7*>(lock)->native_handle(), timeout, 0)
!= 0;
bool wait_for(stl_critical_section_win7* lock, unsigned int timeout) {
return SleepConditionVariableSRW(&m_condition_variable, lock->native_handle(), timeout, 0) != 0;
}

void notify_one() override {
void notify_one() {
WakeConditionVariable(&m_condition_variable);
}

void notify_all() override {
void notify_all() {
WakeAllConditionVariable(&m_condition_variable);
}

private:
CONDITION_VARIABLE m_condition_variable;
CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT;
};

inline void create_stl_critical_section(stl_critical_section_interface* p) {
inline void create_stl_critical_section(stl_critical_section_win7* p) {
new (p) stl_critical_section_win7;
}

inline void create_stl_condition_variable(stl_condition_variable_interface* p) {
inline void create_stl_condition_variable(stl_condition_variable_win7* p) {
new (p) stl_condition_variable_win7;
}

Expand Down
3 changes: 0 additions & 3 deletions tests/std/tests/Dev11_1150223_shared_mutex/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,7 @@ STATIC_ASSERT(noexcept(declval<thread&>().native_handle()));
#if _HAS_CXX20
STATIC_ASSERT(noexcept(declval<jthread&>().native_handle()));
#endif // _HAS_CXX20
STATIC_ASSERT(noexcept(declval<mutex&>().native_handle()));
STATIC_ASSERT(noexcept(declval<recursive_mutex&>().native_handle()));
STATIC_ASSERT(noexcept(declval<shared_mutex&>().native_handle()));
STATIC_ASSERT(noexcept(declval<condition_variable&>().native_handle()));

// Also test mandatory and strengthened exception specification for try_lock().
STATIC_ASSERT(noexcept(declval<mutex&>().try_lock())); // strengthened
Expand Down