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.

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

Updated ReadWriteSpinlock to be atomically upgrade-able from Reader to #129

Merged
merged 2 commits into from
Jun 4, 2020

Conversation

accelerated
Copy link
Contributor

Description of changes

Updated ReadWriteSpinlock to be atomically upgrade-able from Reader to Writer.

Signed-off-by: Alexander Damian [email protected]

quantum/impl/quantum_read_write_spinlock_impl.h Outdated Show resolved Hide resolved
quantum/impl/quantum_read_write_spinlock_impl.h Outdated Show resolved Hide resolved
quantum/impl/quantum_read_write_spinlock_impl.h Outdated Show resolved Hide resolved
quantum/util/impl/quantum_spinlock_util_impl.h Outdated Show resolved Hide resolved
tests/quantum_locks_tests.cpp Show resolved Hide resolved
tests/quantum_locks_tests.cpp Outdated Show resolved Hide resolved
@accelerated accelerated force-pushed the upgrade-writer branch 3 times, most recently from 0037f02 to 43f4958 Compare June 3, 2020 20:03
Copy link
Contributor

@arosenzweig3 arosenzweig3 left a comment

Choose a reason for hiding this comment

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

Looks good overall. I'm still not a fan of the semantics of a ReadGuard owning a write lock, but this looks solid.

quantum/impl/quantum_read_write_mutex_impl.h Outdated Show resolved Hide resolved
tests/quantum_locks_tests.cpp Show resolved Hide resolved
@accelerated accelerated force-pushed the upgrade-writer branch 2 times, most recently from 60b389c to 6bc5e26 Compare June 3, 2020 21:42
quantum/impl/quantum_mutex_impl.h Show resolved Hide resolved
quantum/impl/quantum_read_write_mutex_impl.h Outdated Show resolved Hide resolved
quantum/impl/quantum_read_write_mutex_impl.h Outdated Show resolved Hide resolved
quantum/impl/quantum_read_write_mutex_impl.h Show resolved Hide resolved
quantum/impl/quantum_read_write_spinlock_impl.h Outdated Show resolved Hide resolved
@accelerated accelerated force-pushed the upgrade-writer branch 2 times, most recently from a54bed5 to e96cf02 Compare June 4, 2020 13:27
quantum/impl/quantum_read_write_spinlock_impl.h Outdated Show resolved Hide resolved
quantum/impl/quantum_read_write_mutex_impl.h Outdated Show resolved Hide resolved
quantum/impl/quantum_read_write_spinlock_impl.h Outdated Show resolved Hide resolved
}

inline
bool ReadWriteMutex::ReadGuard::ownsLock() const
void ReadWriteMutex::Guard::lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a lockRead() and a lockWrite() here.

ReadWriteMutex& lock) :
_mutex(&lock),
_ownsLock(true)
bool ReadWriteMutex::Guard::tryLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

likewise tryLockRead() & tryLockWrite() instead of lock()

ReadWriteSpinLock::WriteGuard::WriteGuard(ReadWriteSpinLock& lock) :
_spinlock(lock),
_ownsLock(true)
void ReadWriteSpinLock::Guard::lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as on ReadWriteMutex::Guard; we need lockRead() & lockWrite() rather than lock().


class ReadGuard
class Guard
{
public:
/// @brief Construct this object and lock the passed-in mutex as a reader.
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete 'as a reader' here.

ReadWriteMutex::AdoptLock);
/// @note Does not block.
Guard(ReadWriteMutex& lock,
LockTraits::AdoptLock);

/// @brief Destroy this object and unlock the underlying mutex if this object owns it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rephrase this as "unlock the underlying mutex if this object still owns a lock state on it" or something similar. That is, the object doesn't really give up ownership of the Mutex object (unless we implement release() again). It always owns the Mutex, what it may or may not own is the locked state.


/// @brief Releases the write lock on the underlying mutex.
/// @brief Releases the read lock on the underlying mutex.
/// @note Also releases ownership of the underlying mutex.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment isn't quite right (I expect it's replicated on others); see comments above on destructor.

Copy link
Contributor

@arosenzweig3 arosenzweig3 left a comment

Choose a reason for hiding this comment

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

Looks good.

@accelerated accelerated merged commit 9930616 into bloomberg:master Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants