Skip to content

Commit

Permalink
Use absl::Mutex instead of custom implementation (#1356)
Browse files Browse the repository at this point in the history
Makes `common::Mutex` an alias for `absl::Mutex` and replaces the logic inside `common::MutexLocker` with `absl::MutexLock`.

A future PR can then remove these classes entirely and replace references to it across the Cartographer code base with the Abseil classes directly.
  • Loading branch information
CodeArno authored and wally-the-cartographer committed Aug 3, 2018
1 parent b841ebf commit 12e1185
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 61 deletions.
2 changes: 2 additions & 0 deletions cartographer/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ cc_library(
deps = [
":cc_protos",
"@boost//:iostreams",
"@com_google_absl//absl/base",
"@com_google_absl//absl/types:optional",
"@com_google_absl//absl/synchronization",
"@com_google_glog//:glog",
"@org_cairographics_cairo//:cairo",
"@org_ceres_solver_ceres_solver//:ceres",
Expand Down
83 changes: 25 additions & 58 deletions cartographer/common/mutex.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,83 +17,50 @@
#ifndef CARTOGRAPHER_COMMON_MUTEX_H_
#define CARTOGRAPHER_COMMON_MUTEX_H_

#include <condition_variable>
#include <mutex>

#include "absl/base/thread_annotations.h"
#include "absl/synchronization/mutex.h"
#include "cartographer/common/time.h"

namespace cartographer {
namespace common {

// Enable thread safety attributes only with clang.
// The attributes can be safely erased when compiling with other compilers.
#if defined(__SUPPORT_TS_ANNOTATION__) || defined(__clang__)
#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x))
#else
#define THREAD_ANNOTATION_ATTRIBUTE__(x) // no-op
#endif

#define CAPABILITY(x) THREAD_ANNOTATION_ATTRIBUTE__(capability(x))

#define SCOPED_CAPABILITY THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)

#define GUARDED_BY(x) THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))

#define PT_GUARDED_BY(x) THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x))

#define REQUIRES(...) \
THREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__))

#define ACQUIRE(...) \
THREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__))

#define RELEASE(...) \
THREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__))

#define EXCLUDES(...) THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__))

#define NO_THREAD_SAFETY_ANALYSIS \
THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis)
// TODO(CodeArno): Replace references in code to absl::Mutex directly.
using Mutex = absl::Mutex;

// Defines an annotated mutex that can only be locked through its scoped locker
// implementation.
class CAPABILITY("mutex") Mutex {
// A RAII class that acquires a mutex in its constructor, and
// releases it in its destructor. It also implements waiting functionality on
// conditions that get checked whenever the mutex is released.
// TODO(CodeArno): Replace MutexLocker instantiations in the codebase with
// absl::MutexLock.
class SCOPED_LOCKABLE MutexLocker {
public:
// A RAII class that acquires a mutex in its constructor, and
// releases it in its destructor. It also implements waiting functionality on
// conditions that get checked whenever the mutex is released.
class SCOPED_CAPABILITY Locker {
public:
Locker(Mutex* mutex) ACQUIRE(mutex) : mutex_(mutex), lock_(mutex->mutex_) {}
MutexLocker(Mutex* mutex) EXCLUSIVE_LOCK_FUNCTION(mutex)
: mutex_(mutex), lock_(mutex) {}

~Locker() RELEASE() {
mutex_->condition_.notify_all();
lock_.unlock();
}
~MutexLocker() UNLOCK_FUNCTION() {}

template <typename Predicate>
void Await(Predicate predicate) REQUIRES(this) {
mutex_->condition_.wait(lock_, predicate);
}
template <typename Predicate>
void Await(Predicate predicate) REQUIRES(this) {
mutex_->Await(absl::Condition(&predicate));
}

template <typename Predicate>
bool AwaitWithTimeout(Predicate predicate, common::Duration timeout)
REQUIRES(this) {
return mutex_->condition_.wait_for(lock_, timeout, predicate);
}

private:
Mutex* mutex_;
std::unique_lock<std::mutex> lock_;
};
template <typename Predicate>
bool AwaitWithTimeout(Predicate predicate, common::Duration timeout)
REQUIRES(this) {
return mutex_->AwaitWithTimeout(absl::Condition(&predicate),
absl::FromChrono(timeout));
}

private:
std::condition_variable condition_;
std::mutex mutex_;
absl::Mutex* mutex_;
absl::MutexLock lock_;
};

using MutexLocker = Mutex::Locker;

} // namespace common
} // namespace cartographer

Expand Down
2 changes: 1 addition & 1 deletion cartographer/common/thread_pool_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ namespace {
class Receiver {
public:
void Receive(int number) {
Mutex::Locker locker(&mutex_);
MutexLocker locker(&mutex_);
received_numbers_.push_back(number);
}

Expand Down
5 changes: 3 additions & 2 deletions cmake/modules/FindAbseil.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ if(NOT TARGET standalone_absl)
set(ABSEIL_LIBRARY_PATH
"${ABSEIL_PROJECT_BUILD_DIR}/absl/synchronization/${prefix}absl_synchronization${suffix}")
set(ABSEIL_DEPENDENT_LIBRARIES
"${ABSEIL_PROJECT_BUILD_DIR}/absl/debugging/${prefix}absl_symbolize${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/time/${prefix}absl_time${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/algorithm/${prefix}absl_algorithm${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/base/${prefix}absl_base${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/base/${prefix}absl_dynamic_annotations${suffix}"
Expand All @@ -41,13 +43,12 @@ if(NOT TARGET standalone_absl)
"${ABSEIL_PROJECT_BUILD_DIR}/absl/debugging/${prefix}absl_leak_check${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/debugging/${prefix}absl_stack_consumption${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/debugging/${prefix}absl_stacktrace${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/debugging/${prefix}absl_symbolize${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/memory/${prefix}absl_memory${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/meta/${prefix}absl_meta${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/numeric/${prefix}absl_int128${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/numeric/${prefix}absl_numeric${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/strings/${prefix}absl_strings${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/time/${prefix}absl_time${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/synchronization/${prefix}absl_synchronization${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/types/${prefix}absl_any${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/types/${prefix}absl_bad_any_cast${suffix}"
"${ABSEIL_PROJECT_BUILD_DIR}/absl/types/${prefix}absl_bad_optional_access${suffix}"
Expand Down

0 comments on commit 12e1185

Please sign in to comment.