TaskScheduler [2/9] Scheduler Lock

base/task_scheduler/scheduler_lock_impl.cc

Index: base/task_scheduler/scheduler_lock_impl.cc
diff --git a/base/task_scheduler/scheduler_lock_impl.cc b/base/task_scheduler/scheduler_lock_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d61f63ebd69ea3252d894e069d478fa54d92e737
--- /dev/null
+++ b/base/task_scheduler/scheduler_lock_impl.cc
@@ -0,0 +1,138 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/task_scheduler/scheduler_lock_impl.h"
+
+#include <algorithm>
+#include <unordered_map>
+#include <vector>
+
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "base/synchronization/condition_variable.h"
+#include "base/threading/platform_thread.h"
+
+namespace base {
+namespace internal {
+
+namespace {
+
+class SafeAcquisitionTracker {
+ public:
+  SafeAcquisitionTracker() = default;
+
+  void RegisterLock(
+      const SchedulerLockImpl* const lock,
+      const SchedulerLockImpl* const predecessor) {
+    // Reentrant locks are unsupported.
+    DCHECK(lock != predecessor);
+    AutoLock auto_lock(metadata_lock_);
+    allowed_predecessor_map[lock] = predecessor;
+    AssertSafePredecessor(lock);
+  }
+
+  void UnregisterLock(const SchedulerLockImpl* const lock) {
+    AutoLock auto_lock(metadata_lock_);
+    allowed_predecessor_map.erase(lock);
+  }
+
+  void RecordAcquisition(const SchedulerLockImpl* const lock) {
+    AutoLock auto_lock(metadata_lock_);
+    const PlatformThreadId id = PlatformThread::CurrentId();
+    AssertSafeAcquire(id, lock);
+    acquired_locks_[id].push_back(lock);
+  }
+
+  void RecordRelease(const SchedulerLockImpl* const lock) {
+    AutoLock auto_lock(metadata_lock_);
+    const PlatformThreadId id = PlatformThread::CurrentId();
+    LockVector& thread_locks = acquired_locks_[id];
+    const auto iter =
+        std::find(thread_locks.begin(), thread_locks.end(), lock);
+    DCHECK(iter != thread_locks.end());
+    thread_locks.erase(iter);
+    if (thread_locks.empty())
+      acquired_locks_.erase(id);
+  }
+
+ private:
+  using LockVector = std::vector<const SchedulerLockImpl*>;
+  using PredecessorMap = std::unordered_map<
+      const SchedulerLockImpl*, const SchedulerLockImpl*>;
+  using AcquisitionMap =
+      std::unordered_map<base::PlatformThreadId, LockVector>;
+
+  // This asserts that the lock is safe to acquire. This means that this should
+  // be run before actually recording the acquisition.
+  void AssertSafeAcquire(PlatformThreadId id,
+                         const SchedulerLockImpl* const lock) const {
+    metadata_lock_.AssertAcquired();
+    const auto& thread_lock_pair = acquired_locks_.find(id);
+
+    // If the thread currently holds no locks, this is inherently safe.
+    if (thread_lock_pair == acquired_locks_.end())
+      return;
+
+    // Otherwise, make sure that the previous lock acquired is an allowed
+    // predecessor.
+    const SchedulerLockImpl* allowed_predecessor =
+        allowed_predecessor_map.at(lock);

[gab, 2016/02/19 16:08:03]

at() throws an exception if |lock| isn't in |allowed_predecessor_map| which
isn't a Chromium friendly way of expressing an error.

How about:

const auto allowed_predecessor_it = allowed_predecessor_map.find(lock);
DCHECK(thread_lock_pair->second.back(), *allowed_predecessor_it);

(I think it's fine to deref even if |allowed_predecessor_it| is == end()? if not
then we should DCHECK for that first)

[robliao, 2016/02/19 21:25:06]

I do actually want this to crash since it's akin to a nullptr deref. We seem to
use STL vector|map|unordered_map.at() everywhere in Chromium:

https://code.google.com/p/chromium/codesearch#search/&q=%5C.at%5C(%20-file:v8...

[gab, 2016/02/19 22:05:24]

But we could DCHECK with better information for the developer than a crash
(remember that this is debug only code trying to inform the developer of an
error).

+    DCHECK(thread_lock_pair->second.back() == allowed_predecessor);
+  }
+
+  void AssertSafePredecessor(const SchedulerLockImpl* lock) const {
+    metadata_lock_.AssertAcquired();
+    for (const SchedulerLockImpl* predecessor =
+             allowed_predecessor_map.at(lock);
+         predecessor != nullptr;
+         predecessor = allowed_predecessor_map.at(predecessor)) {

[gab, 2016/02/19 16:08:03]

Also don't use at() here.

[robliao, 2016/02/19 21:25:06]

See above.

+      if (predecessor == lock)
+        DCHECK(false);

[gab, 2016/02/19 16:08:03]

Why the change to DCHECK(false)? This is what NOTREACHED() is, no?

[robliao, 2016/02/19 21:25:06]

I got feedback in a separate review that NOTREACHED() should be a DCHECK.

Turns out, this actually does something in ChromeOS.
#if !DCHECK_IS_ON() && defined(OS_CHROMEOS)
// Implement logging of NOTREACHED() as a dedicated function to get function
// call overhead down to a minimum.
void LogErrorNotReached(const char* file, int line);
#define NOTREACHED()                                       \
  true ? ::logging::LogErrorNotReached(__FILE__, __LINE__) \
       : EAT_STREAM_PARAMETERS
#else
#define NOTREACHED() DCHECK(false)
#endif

[gab, 2016/02/19 22:05:24]

Meh, this code is only for !DCHECK_IS_ON() where this code never runs anyways.
I'd just stick to NOTREACHED(), but don't feel strongly enough if you do.

[robliao, 2016/02/19 23:51:11]

No strong opinion either way. Switched back to NOTREACHED().

+    }
+  }
+
+  // Synchronizes everything in SafeAcquisitionTracker.
+  base::Lock metadata_lock_;
+
+  PredecessorMap allowed_predecessor_map;
+  AcquisitionMap acquired_locks_;
+
+  DISALLOW_COPY_AND_ASSIGN(SafeAcquisitionTracker);
+};
+
+LazyInstance<SafeAcquisitionTracker> g_safe_acquisition_tracker =
+    LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+SchedulerLockImpl::SchedulerLockImpl() : SchedulerLockImpl(nullptr) {}
+
+SchedulerLockImpl::SchedulerLockImpl(const SchedulerLockImpl* predecessor) {
+  g_safe_acquisition_tracker.Get().RegisterLock(this, predecessor);
+}
+
+SchedulerLockImpl::~SchedulerLockImpl() {
+  g_safe_acquisition_tracker.Get().UnregisterLock(this);
+}
+
+void SchedulerLockImpl::Acquire() {
+  lock_.Acquire();
+  g_safe_acquisition_tracker.Get().RecordAcquisition(this);

[gab, 2016/02/19 16:08:03]

Should this go first to avoid locking in the death test?

Otherwise I don't understand why the test doesn't run into [1] when freeing some
locks without releasing them first after the death expectation.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

[robliao, 2016/02/19 21:25:06]

See discussion in the unit tests file:
Summarized
Non-Windows - Fork - Copy-on-write memory so from this proc, the acquires never
happened.
Windows - Relaunch - From the view of this process, the acquires also never
happened.

+}
+
+void SchedulerLockImpl::Release() {
+  lock_.Release();
+  g_safe_acquisition_tracker.Get().RecordRelease(this);
+}
+
+void SchedulerLockImpl::AssertAcquired() const {
+  lock_.AssertAcquired();
+}
+
+scoped_ptr<ConditionVariable> SchedulerLockImpl::CreateConditionVariable() {
+  return scoped_ptr<ConditionVariable>(new ConditionVariable(&lock_));
+}
+
+}  // namespace internal
+}  // base