Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1706123002:
TaskScheduler [2/9] Scheduler Lock (Closed)
DescriptionTaskScheduler [2/9] Scheduler Lock
The task scheduler lock helps ensure correctness of locks by verifying that
we cannot create lock cycles and checking to make sure that the locks acquired
match the locks expected to be acquired in DCHECK_IS_ON() builds.
The implementation will DCHECK if an unexpected lock is acquired.
When DCHECK_IS_ON() is not set, SchedulerLock is a regular lock.
This change is a subset of https://codereview.chromium.org/1698183005/
BUG=553459
Committed: https://crrev.com/b6f61ce5c35e985cf07279ea25fd846b0b9d6f9d
Cr-Commit-Position: refs/heads/master@{#381075}
Patch Set 1 #
Total comments: 33
Patch Set 2 : CR Feedback #
Total comments: 41
Patch Set 3 : CR Feedback #
Total comments: 4
Patch Set 4 : Thread Local Acquired Lock Vector and CR Feedback #
Total comments: 27
Patch Set 5 : CR Feedback #Patch Set 6 : thread_local -> thread_local_storage ( Bug http://crbug.com/588824) #
Total comments: 4
Patch Set 7 : Nits #Patch Set 8 : Standardize Tests on SimpleThread #
Total comments: 12
Patch Set 9 : CR Feedback #Patch Set 10 : Check GTEST_HAS_DEATH_TEST and Disable Death Tests on Android #
Total comments: 10
Patch Set 11 : Move most DCHECKs to DCHECK_(EQ|NE) #
Total comments: 1
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 97 (36 generated)
Description was changed from ========== Task Scheduler Lock The task scheduler lock helps ensure correctness of locks by ensuring that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock becomes a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== Task Scheduler Lock The task scheduler lock helps ensure correctness of locks by ensuring that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock becomes a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
Description was changed from ========== Task Scheduler Lock The task scheduler lock helps ensure correctness of locks by ensuring that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock becomes a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [2/8] Scheduler Lock The task scheduler lock helps ensure correctness of locks by verifying that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock becomes a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Was this change uploaded from s_1_scheduler_lock with s_0_task_traits as upstream? Feels that's what we should do instead of syncing and pointing at master. That way we have dependents/dependees highlighted automagically by rietveld. Syncing + pointing at master is typically better but in this scenario no one is touching this code but us so until we have to sync because of conflict, I'd rather we don't (until the 8 impl CLs go in, then we can drop the private repo and move everything to a real chrome checkout). WDYT?
On 2016/02/18 13:24:17, gab wrote: > Was this change uploaded from s_1_scheduler_lock with s_0_task_traits as > upstream? Feels that's what we should do instead of syncing and pointing at > master. That way we have dependents/dependees highlighted automagically by > rietveld. > > Syncing + pointing at master is typically better but in this scenario no one is > touching this code but us so until we have to sync because of conflict, I'd > rather we don't (until the 8 impl CLs go in, then we can drop the private repo > and move everything to a real chrome checkout). > > WDYT? Note: I'm mostly trying to avoid us having to move which revision we're based on throughout these 8 CLs so that any one of us can merge all changes from previous branches without incurring changes from master.
On 2016/02/18 15:56:46, gab wrote: > On 2016/02/18 13:24:17, gab wrote: > > Was this change uploaded from s_1_scheduler_lock with s_0_task_traits as > > upstream? Feels that's what we should do instead of syncing and pointing at > > master. That way we have dependents/dependees highlighted automagically by > > rietveld. > > > > Syncing + pointing at master is typically better but in this scenario no one > is > > touching this code but us so until we have to sync because of conflict, I'd > > rather we don't (until the 8 impl CLs go in, then we can drop the private repo > > and move everything to a real chrome checkout). > > > > WDYT? > > Note: I'm mostly trying to avoid us having to move which revision we're based on > throughout these 8 CLs so that any one of us can merge all changes from previous > branches without incurring changes from master. On a similar note, can you push updates to s_0_task_traits so that other branches can be based on what landed?
On 2016/02/18 16:06:00, gab wrote: > On 2016/02/18 15:56:46, gab wrote: > > On 2016/02/18 13:24:17, gab wrote: > > > Was this change uploaded from s_1_scheduler_lock with s_0_task_traits as > > > upstream? Feels that's what we should do instead of syncing and pointing at > > > master. That way we have dependents/dependees highlighted automagically by > > > rietveld. > > > > > > Syncing + pointing at master is typically better but in this scenario no one > > is > > > touching this code but us so until we have to sync because of conflict, I'd > > > rather we don't (until the 8 impl CLs go in, then we can drop the private > repo > > > and move everything to a real chrome checkout). > > > > > > WDYT? > > > > Note: I'm mostly trying to avoid us having to move which revision we're based > on > > throughout these 8 CLs so that any one of us can merge all changes from > previous > > branches without incurring changes from master. > > On a similar note, can you push updates to s_0_task_traits so that other > branches can be based on what landed? As for getting the base URL right, all that matters is that the chain of upstreams land at origin/master. For Francois' CLs I had him build a chain of origin/master -> s_master -> s_0_task_traits -> s_1_scheduler_lock -> ... and all the uploads ended up with the right base URL (+ bonus with rietveld linking to dependent/dependee on each CL)
Description was changed from ========== TaskScheduler [2/8] Scheduler Lock The task scheduler lock helps ensure correctness of locks by verifying that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock becomes a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [2/8] Scheduler Lock The task scheduler lock helps ensure correctness of locks by verifying that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock is a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Running to a series a meeting, here's a first pass, looked at everything but the unittest. Thanks! Gab https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_lock.h (right): https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:19: Add a meta comment here on how this class should be used (e.g. both constructors and CreateConditionVariable()). I think that's better than adding comments on each method below since they're defined twice + CreateConditionVariable() is only defined here for the non-impl. So I'd say meta comment here and then no/minimal comments below and in impl header (as you have now). Also mention in that meta-comment that loops in predecessor chains are prevented by the predecessor only being registerable at construction time. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:27: #else #else // DCHECK_IS_ON() https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:38: #endif #endif // DCHECK_IS_ON() https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:47: lock_.Release(); Add lock_.AssertAcquired(); above. (to match AutoLock's impl: https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...) https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:52: DISALLOW_COPY_AND_ASSIGN(AutoSchedulerLock); Chromium style is typically to have an empty line above DISALLOW_COPY_AND_ASSIGN macro https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:24: return base::Singleton<SafeAcquisitionTracker>::get(); Instead of a Singleton, can we use a LazyInstance::Leaky? e.g. below this class definition add: LazyInstance<SafeAcquisitionTracker>::Leaky safe_acquisition_tracker = LAZY_INSTANCE_INITIALIZER; One of the major differences is that base::Singleton is a global registration -- although no one else could get it per not having a the SafeAcquisitionTracker type definition -- whereas LazyInstance keeps everything local for real. Plus Singleton is generally frowned upon in and not necessary here I think. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:30: // Reentrant locks are unexpected. s/unexpected/unsupported/ https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:31: CHECK(lock != predecessor); DCHECK (equivalent since this code ever runs in DCHECK-enabled mode but avoids giving a sense to the reader that it does run in non-DCHECK-mode) https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:47: thread_locks.push_back(lock); Merge above two lines into: acquired_locks_[id].push_back(lock); ? I guess with the two lines it's more explicit and closer to RecordRelease(). Given that, I'm okay either way, WDYT? https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:66: std::unordered_map<base::PlatformThreadId, LockVector>; How about using ThreadLocalStorage here instead of a map with a PlatformThreadId as key? Would that allow less |metadata_lock_| locking in this class? https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:70: void AssertSafeAcquire(PlatformThreadId id, Add a comment + mention that this must be called before |lock| is registered in |acquired_locks_|. (or just inline this in RecordAcquisition since it's its only caller) https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:75: // If the thread hasn't ever acquired a lock, this is inherently safe. // If the thread doesn't currently own a lock, this is inherently safe. (i.e. "hasn't ever" is pushing it, it could have held some in the past) https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:92: NOTREACHED(); So is this method only verifying that we do not get a bidirectional registration? If so this is impossible per the registration happening at construction time. So I think it's safe to remove this method and instead specify this in the API meta comment as already mentioned above. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:99: PredecessorMap allowed_predecessors_; Whenever I saw this in code the plural form made me think that each lock had multiple predecessors instead of this being a map of multiple one-off predecessor registrations. How about |allowed_predecessor_map| or |predecessor_registrations|? https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:107: SchedulerLockImpl::SchedulerLockImpl() : SchedulerLockImpl(nullptr) {} Seems this will call RegisterLock with a nullptr predecessor, should that method check for nullptr or do you think it's not a big deal to register a null predecessor regardless?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Thanks for the comments! > Was this change uploaded from s_1_scheduler_lock with s_0_task_traits as upstream? Feels that's what we should do instead of syncing and pointing at master. That way we have dependents/dependees highlighted automagically by rietveld. This change was uploaded against Chromium Head because there have been changes to build files in the meantime. I went ahead and chained this to 1702813002. It's just metadata, so you can upload against head and then have an upstream branch that points to the previous CL. (You do have to do some git config updating to get this to work, but it seems to work fine). > On a similar note, can you push updates to s_0_task_traits so that other branches can be based on what landed? Done https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_lock.h (right): https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:19: On 2016/02/18 16:58:06, gab wrote: > Add a meta comment here on how this class should be used (e.g. both constructors > and CreateConditionVariable()). I think that's better than adding comments on > each method below since they're defined twice + CreateConditionVariable() is > only defined here for the non-impl. > > So I'd say meta comment here and then no/minimal comments below and in impl > header (as you have now). > > Also mention in that meta-comment that loops in predecessor chains are prevented > by the predecessor only being registerable at construction time. Done, except for the loops part. The unit tests actually constructs some loops. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:27: #else On 2016/02/18 16:58:06, gab wrote: > #else // DCHECK_IS_ON() Done. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:38: #endif On 2016/02/18 16:58:06, gab wrote: > #endif // DCHECK_IS_ON() Done. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:47: lock_.Release(); On 2016/02/18 16:58:06, gab wrote: > Add > > lock_.AssertAcquired(); > > above. > > (to match AutoLock's impl: > https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...) Done. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock.h:52: DISALLOW_COPY_AND_ASSIGN(AutoSchedulerLock); On 2016/02/18 16:58:06, gab wrote: > Chromium style is typically to have an empty line above DISALLOW_COPY_AND_ASSIGN > macro Done. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:24: return base::Singleton<SafeAcquisitionTracker>::get(); On 2016/02/18 16:58:06, gab wrote: > Instead of a Singleton, can we use a LazyInstance::Leaky? > > e.g. below this class definition add: > > LazyInstance<SafeAcquisitionTracker>::Leaky safe_acquisition_tracker = > LAZY_INSTANCE_INITIALIZER; > > One of the major differences is that base::Singleton is a global registration -- > although no one else could get it per not having a the SafeAcquisitionTracker > type definition -- whereas LazyInstance keeps everything local for real. Plus > Singleton is generally frowned upon in and not necessary here I think. Works for me. Done. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:30: // Reentrant locks are unexpected. On 2016/02/18 16:58:06, gab wrote: > s/unexpected/unsupported/ Done. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:31: CHECK(lock != predecessor); On 2016/02/18 16:58:06, gab wrote: > DCHECK > > (equivalent since this code ever runs in DCHECK-enabled mode but avoids giving a > sense to the reader that it does run in non-DCHECK-mode) Done. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:47: thread_locks.push_back(lock); On 2016/02/18 16:58:06, gab wrote: > Merge above two lines into: > > acquired_locks_[id].push_back(lock); > > ? > > I guess with the two lines it's more explicit and closer to RecordRelease(). > Given that, I'm okay either way, WDYT? Symmetry is nice, but it's still the same code. Merged. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:66: std::unordered_map<base::PlatformThreadId, LockVector>; On 2016/02/18 16:58:06, gab wrote: > How about using ThreadLocalStorage here instead of a map with a PlatformThreadId > as key? Would that allow less |metadata_lock_| locking in this class? We would only be able to save on the release. Acquisitions still need to check the allowed_predecessor_map. I'm not sure how much this buys us for an extra gotcha in the class. Releases should run fairly quickly. acquired locks lookups should be O(1) and the number of locks held by a thread should be very small. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:70: void AssertSafeAcquire(PlatformThreadId id, On 2016/02/18 16:58:06, gab wrote: > Add a comment + mention that this must be called before |lock| is registered in > |acquired_locks_|. > > (or just inline this in RecordAcquisition since it's its only caller) This keeps both RecordAcquisition and RecordRelease as booking. Added the comment. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:75: // If the thread hasn't ever acquired a lock, this is inherently safe. On 2016/02/18 16:58:06, gab wrote: > // If the thread doesn't currently own a lock, this is inherently safe. > > (i.e. "hasn't ever" is pushing it, it could have held some in the past) Fixed. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:92: NOTREACHED(); On 2016/02/18 16:58:06, gab wrote: > So is this method only verifying that we do not get a bidirectional > registration? If so this is impossible per the registration happening at > construction time. > > So I think it's safe to remove this method and instead specify this in the API > meta comment as already mentioned above. There's actually a unit test for this :-). It is indeed possible. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:99: PredecessorMap allowed_predecessors_; On 2016/02/18 16:58:07, gab wrote: > Whenever I saw this in code the plural form made me think that each lock had > multiple predecessors instead of this being a map of multiple one-off > predecessor registrations. > > How about |allowed_predecessor_map| or |predecessor_registrations|? Went with allowed_predecessor_map. https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:107: SchedulerLockImpl::SchedulerLockImpl() : SchedulerLockImpl(nullptr) {} On 2016/02/18 16:58:06, gab wrote: > Seems this will call RegisterLock with a nullptr predecessor, should that method > check for nullptr or do you think it's not a big deal to register a null > predecessor regardless? In invariant here is that all locks are registered with a predecessor, allowing us to form a predecessor chain. nullptr forms a nice end to the chain.
Patchset #3 (id:120001) has been deleted
On 2016/02/18 21:59:37, robliao wrote: > Thanks for the comments! > > > Was this change uploaded from s_1_scheduler_lock with s_0_task_traits as > upstream? Feels that's what we should do instead of syncing and pointing at > master. That way we have dependents/dependees highlighted automagically by > rietveld. > > This change was uploaded against Chromium Head because there have been changes > to build files in the meantime. Ah okay, in cases where that's needed I think we should move s_master up (and only in those cases -- i.e. as little as possible, but that's why I made it separate from a_master) and then run the mega merge command I sent up until the branch you care about + `git push lucky`. It doesn't affect me/fdoray until we "sync" (fetch and merge) LL anyways. Basically I think we should all use the same chain, and push changes to it, until we reach the top. If we start using different bases it's going to be a mess. Thanks! > > I went ahead and chained this to 1702813002. It's just metadata, so you can > upload against head and then have an upstream branch that points to the previous > CL. (You do have to do some git config updating to get this to work, but it > seems to work fine). > > > On a similar note, can you push updates to s_0_task_traits so that other > branches can be based on what landed? > Done https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:66: std::unordered_map<base::PlatformThreadId, LockVector>; On 2016/02/18 21:59:37, robliao wrote: > On 2016/02/18 16:58:06, gab wrote: > > How about using ThreadLocalStorage here instead of a map with a > PlatformThreadId > > as key? Would that allow less |metadata_lock_| locking in this class? > > We would only be able to save on the release. Acquisitions still need to check > the allowed_predecessor_map. > > I'm not sure how much this buys us for an extra gotcha in the class. Releases > should run fairly quickly. acquired locks lookups should be O(1) and the number > of locks held by a thread should be very small. To avoid this being a "gotcha -- only lock here not there" we could make the lock specific to the allowed_predecessor_map. It just feels to me ThreadLocalStorage is meant for exactly this (thread specific state) and that using a thread-safe map for which every thread only accesses its own entry is wrong. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:80: allowed_predecessor_map.at(lock); 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) https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:89: predecessor = allowed_predecessor_map.at(predecessor)) { Also don't use at() here. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:91: DCHECK(false); Why the change to DCHECK(false)? This is what NOTREACHED() is, no? https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:121: g_safe_acquisition_tracker.Get().RecordAcquisition(this); 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... https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.h (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.h:22: // See scheduler_lock.h for full DCHECK details. s/full DCHECK details/details/ ? (i.e. there are more details than just DCHECKs in scheduler_lock.h; e.g., method comments) https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:18: class BasicLockTestThread : public PlatformThread::Delegate { Meta comment on what this does. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:33: PlatformThread::Sleep(TimeDelta::FromMilliseconds(rand() % 20)); Use base/rand_util.h instead of stdlib's rand() https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:61: void WaitForLockAcquition() { WaitForLockAcquisition ^ https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:73: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:104: EXPECT_GE(thread.acquired(), 20); Why GE and not EQ? (if EQ, put constant on LHS, per the macros's (expected, actual) definition) https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:117: SchedulerLock lock_predecessor; s/lock_predecessor/other_lock/ (since it's not actually a predecessor?) https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:119: EXPECT_DEBUG_DEATH({ Cool :-)!! Didn't know about EXPECT_DEBUG_DEATH! But it seems to be defined based on NDEBUG [1], so won't it fail in Release builds with dcheck_always_on? Perhaps we'll need some kind of include trick like: #if DCHECK_IS_ON() && defined(NDEBUG) // EXPECT_DEBUG_DEATH macros are defined based on NDEBUG, but they should // trigger as well in DCHECK_ALWAYS_ON Release builds. #undef NDEBUG #include "testing/gtest/include/gtest/gtest.h" #define NDEBUG #else // defined(DCHECK_ALWAYS_ON) && defined(NDEBUG) #include "testing/gtest/include/gtest/gtest.h" #endif // defined(DCHECK_ALWAYS_ON) && defined(NDEBUG) [1] https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:151: SchedulerLock lock3(&lock2); Looks like this test only needs two locks. Let's keep each test to the minimal state required. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:154: lock3.Acquire(); I'd prefer if this was the last statement in the EXPECT_DEBUG_DEATH scope as it's otherwise unclear that this is the one expected to cause the failure. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:155: lock1.Release(); In non-dcheck builds you have to Release lock3 or undefined behaviour ensues (deleting an owned CRITICAL_SECTION is undefined [1] and a pthread_mutex will fail [2]). So how about taking this out into: #if !DCHECK_IS_ON() lock3.Release(); #endif lock1.Release(); [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682552(v=vs.85).aspx [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_mutex_destro... https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:179: Think we also need thread.ContinueAcquireLock(); to tell the thread when it should try to acquire the lock. i.e. the real test here is that we can grab lock2 before lock1 iff lock2 is not obtained on the same thread at lock1. Suggest adding a comment here like: // TaskSchedulerLock.AcquireMultipleLocksOutOfOrder verified that // lock2 couldn't be obtained before lock1 on the same thread, but // it should be okay on different threads. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:207: Ah interesting :-), thought taking predecessor as constructor prevented cycles altogether but I guess it's possible this way. Looks like silly code, but no harm testing for it since it's only DCHECK code. With that in mind I think we should also test non-immediate cycles (i.e. more than 2 elements) as those are probably more likely (hope nobody would intentionally write the above two... though I guess they could occur by typo or something).
https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:80: allowed_predecessor_map.at(lock); On 2016/02/19 16:08:03, gab wrote: > 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) 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... https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:89: predecessor = allowed_predecessor_map.at(predecessor)) { On 2016/02/19 16:08:03, gab wrote: > Also don't use at() here. See above. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:91: DCHECK(false); On 2016/02/19 16:08:03, gab wrote: > Why the change to DCHECK(false)? This is what NOTREACHED() is, no? 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 https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:121: g_safe_acquisition_tracker.Get().RecordAcquisition(this); On 2016/02/19 16:08:03, gab wrote: > 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... 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. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.h (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.h:22: // See scheduler_lock.h for full DCHECK details. On 2016/02/19 16:08:03, gab wrote: > s/full DCHECK details/details/ ? (i.e. there are more details than just DCHECKs > in scheduler_lock.h; e.g., method comments) Fair enough. Done. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:18: class BasicLockTestThread : public PlatformThread::Delegate { On 2016/02/19 16:08:04, gab wrote: > Meta comment on what this does. Done. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:33: PlatformThread::Sleep(TimeDelta::FromMilliseconds(rand() % 20)); On 2016/02/19 16:08:04, gab wrote: > Use base/rand_util.h instead of stdlib's rand() Done. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:61: void WaitForLockAcquition() { On 2016/02/19 16:08:03, gab wrote: > WaitForLockAcquisition > ^ Done. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:73: }; On 2016/02/19 16:08:03, gab wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:104: EXPECT_GE(thread.acquired(), 20); On 2016/02/19 16:08:03, gab wrote: > Why GE and not EQ? (if EQ, put constant on LHS, per the macros's (expected, > actual) definition) The original lock test had a try, so there was a case where this would fail. We don't have try, so now updated to EQ. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:117: SchedulerLock lock_predecessor; On 2016/02/19 16:08:03, gab wrote: > s/lock_predecessor/other_lock/ (since it's not actually a predecessor?) Changed to lock1 and lock2 to match below. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:119: EXPECT_DEBUG_DEATH({ On 2016/02/19 16:08:03, gab wrote: > Cool :-)!! Didn't know about EXPECT_DEBUG_DEATH! > > But it seems to be defined based on NDEBUG [1], so won't it fail in Release > builds with dcheck_always_on? > > Perhaps we'll need some kind of include trick like: > > #if DCHECK_IS_ON() && defined(NDEBUG) > // EXPECT_DEBUG_DEATH macros are defined based on NDEBUG, but they should > // trigger as well in DCHECK_ALWAYS_ON Release builds. > #undef NDEBUG > #include "testing/gtest/include/gtest/gtest.h" > #define NDEBUG > #else // defined(DCHECK_ALWAYS_ON) && defined(NDEBUG) > #include "testing/gtest/include/gtest/gtest.h" > #endif // defined(DCHECK_ALWAYS_ON) && defined(NDEBUG) > > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... Turns out Release builds for tests don't have DCHECKs! Toss in a DCHECK(false) and it doesn't trigger. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:151: SchedulerLock lock3(&lock2); On 2016/02/19 16:08:03, gab wrote: > Looks like this test only needs two locks. Let's keep each test to the minimal > state required. This is actually a little different from AcquireNonPredecessor, which is the two lock case. This tests to make sure that you don't automatically get lock1 by transitivity. Renamed the test https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:154: lock3.Acquire(); On 2016/02/19 16:08:04, gab wrote: > I'd prefer if this was the last statement in the EXPECT_DEBUG_DEATH scope as > it's otherwise unclear that this is the one expected to cause the failure. Done. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:155: lock1.Release(); On 2016/02/19 16:08:03, gab wrote: > In non-dcheck builds you have to Release lock3 or undefined behaviour ensues > (deleting an owned CRITICAL_SECTION is undefined [1] and a pthread_mutex will > fail [2]). > > So how about taking this out into: > > #if !DCHECK_IS_ON() > lock3.Release(); > #endif > lock1.Release(); > > [1] > https://msdn.microsoft.com/en-us/library/windows/desktop/ms682552(v=vs.85).aspx > > [2] > http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_mutex_destro... Turns out, this is okay due to forking on non-Windows platforms. On Windows platforms, a whole new process gets spun up and the specific test is reexecuted with some special flags (filter to this test, pass a death test flag). So in this process, the body of EXPECT_DEBUG_DEATH never happens. This also explains why the older form of the test worked on Windows as the entire test was rerun instead of forking at this point. https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/... https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:179: On 2016/02/19 16:08:03, gab wrote: > Think we also need > thread.ContinueAcquireLock(); > to tell the thread when it should try to acquire the lock. > > i.e. the real test here is that we can grab lock2 before lock1 iff lock2 is not > obtained on the same thread at lock1. > > Suggest adding a comment here like: > > // TaskSchedulerLock.AcquireMultipleLocksOutOfOrder verified that > // lock2 couldn't be obtained before lock1 on the same thread, but > // it should be okay on different threads. Clarified the test and added explicit locking order. We can approximate ContinueAcquireLock by waiting on when we create the thread. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:207: On 2016/02/19 16:08:04, gab wrote: > Ah interesting :-), thought taking predecessor as constructor prevented cycles > altogether but I guess it's possible this way. Looks like silly code, but no > harm testing for it since it's only DCHECK code. > > With that in mind I think we should also test non-immediate cycles (i.e. more > than 2 elements) as those are probably more likely (hope nobody would > intentionally write the above two... though I guess they could occur by typo or > something). Done.
lgtm % TLS ping and nits (feel free to send it to OWNERS by EOD -- will be somewhat available on hangouts for a few hours for discussion) https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:66: std::unordered_map<base::PlatformThreadId, LockVector>; On 2016/02/19 16:08:02, gab wrote: > On 2016/02/18 21:59:37, robliao wrote: > > On 2016/02/18 16:58:06, gab wrote: > > > How about using ThreadLocalStorage here instead of a map with a > > PlatformThreadId > > > as key? Would that allow less |metadata_lock_| locking in this class? > > > > We would only be able to save on the release. Acquisitions still need to check > > the allowed_predecessor_map. > > > > I'm not sure how much this buys us for an extra gotcha in the class. Releases > > should run fairly quickly. acquired locks lookups should be O(1) and the > number > > of locks held by a thread should be very small. > > To avoid this being a "gotcha -- only lock here not there" we could make the > lock specific to the allowed_predecessor_map. It just feels to me > ThreadLocalStorage is meant for exactly this (thread specific state) and that > using a thread-safe map for which every thread only accesses its own entry is > wrong. ping ^^ https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:80: allowed_predecessor_map.at(lock); On 2016/02/19 21:25:06, robliao wrote: > On 2016/02/19 16:08:03, gab wrote: > > 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) > > 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... 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). https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:91: DCHECK(false); On 2016/02/19 21:25:06, robliao wrote: > On 2016/02/19 16:08:03, gab wrote: > > Why the change to DCHECK(false)? This is what NOTREACHED() is, no? > > 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 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. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:119: EXPECT_DEBUG_DEATH({ On 2016/02/19 21:25:06, robliao wrote: > On 2016/02/19 16:08:03, gab wrote: > > Cool :-)!! Didn't know about EXPECT_DEBUG_DEATH! > > > > But it seems to be defined based on NDEBUG [1], so won't it fail in Release > > builds with dcheck_always_on? > > > > Perhaps we'll need some kind of include trick like: > > > > #if DCHECK_IS_ON() && defined(NDEBUG) > > // EXPECT_DEBUG_DEATH macros are defined based on NDEBUG, but they should > > // trigger as well in DCHECK_ALWAYS_ON Release builds. > > #undef NDEBUG > > #include "testing/gtest/include/gtest/gtest.h" > > #define NDEBUG > > #else // defined(DCHECK_ALWAYS_ON) && defined(NDEBUG) > > #include "testing/gtest/include/gtest/gtest.h" > > #endif // defined(DCHECK_ALWAYS_ON) && defined(NDEBUG) > > > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... > > Turns out Release builds for tests don't have DCHECKs! Toss in a DCHECK(false) > and it doesn't trigger. Hmmm okay (they probably should and I remember a discussion that someone wanted to add them). Either way we need to support it (my dev build is Release+dcheck_always_on). https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:151: SchedulerLock lock3(&lock2); On 2016/02/19 21:25:06, robliao wrote: > On 2016/02/19 16:08:03, gab wrote: > > Looks like this test only needs two locks. Let's keep each test to the minimal > > state required. > > This is actually a little different from AcquireNonPredecessor, which is the two > lock case. > This tests to make sure that you don't automatically get lock1 by transitivity. > > Renamed the test Ah I see then I think we also need an "out-of-order" test (i.e. lock1, lock2(&lock1), acquire(lock2), acquire(lock1), explode). https://codereview.chromium.org/1706123002/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.h (right): https://codereview.chromium.org/1706123002/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.h:22: // See scheduler_lock.h for jdetails. s/jdetails/details/ https://codereview.chromium.org/1706123002/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:174: } What is this now testing that the next two tests aren't? Looks like it will undeterministically alternate between being either?
Patchset #4 (id:160001) has been deleted
It would be nice if there were resolved/unresolved bits for the comments! https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_lock_impl.cc:66: std::unordered_map<base::PlatformThreadId, LockVector>; On 2016/02/19 22:05:24, gab wrote: > On 2016/02/19 16:08:02, gab wrote: > > On 2016/02/18 21:59:37, robliao wrote: > > > On 2016/02/18 16:58:06, gab wrote: > > > > How about using ThreadLocalStorage here instead of a map with a > > > PlatformThreadId > > > > as key? Would that allow less |metadata_lock_| locking in this class? > > > > > > We would only be able to save on the release. Acquisitions still need to > check > > > the allowed_predecessor_map. > > > > > > I'm not sure how much this buys us for an extra gotcha in the class. > Releases > > > should run fairly quickly. acquired locks lookups should be O(1) and the > > number > > > of locks held by a thread should be very small. > > > > To avoid this being a "gotcha -- only lock here not there" we could make the > > lock specific to the allowed_predecessor_map. It just feels to me > > ThreadLocalStorage is meant for exactly this (thread specific state) and that > > using a thread-safe map for which every thread only accesses its own entry is > > wrong. > > ping ^^ Oops. Missed this one. Done. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:91: DCHECK(false); On 2016/02/19 22:05:24, gab wrote: > On 2016/02/19 21:25:06, robliao wrote: > > On 2016/02/19 16:08:03, gab wrote: > > > Why the change to DCHECK(false)? This is what NOTREACHED() is, no? > > > > 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 > > 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. No strong opinion either way. Switched back to NOTREACHED(). https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:119: EXPECT_DEBUG_DEATH({ On 2016/02/19 22:05:24, gab wrote: > On 2016/02/19 21:25:06, robliao wrote: > > On 2016/02/19 16:08:03, gab wrote: > > > Cool :-)!! Didn't know about EXPECT_DEBUG_DEATH! > > > > > > But it seems to be defined based on NDEBUG [1], so won't it fail in Release > > > builds with dcheck_always_on? > > > > > > Perhaps we'll need some kind of include trick like: > > > > > > #if DCHECK_IS_ON() && defined(NDEBUG) > > > // EXPECT_DEBUG_DEATH macros are defined based on NDEBUG, but they should > > > // trigger as well in DCHECK_ALWAYS_ON Release builds. > > > #undef NDEBUG > > > #include "testing/gtest/include/gtest/gtest.h" > > > #define NDEBUG > > > #else // defined(DCHECK_ALWAYS_ON) && defined(NDEBUG) > > > #include "testing/gtest/include/gtest/gtest.h" > > > #endif // defined(DCHECK_ALWAYS_ON) && defined(NDEBUG) > > > > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... > > > > Turns out Release builds for tests don't have DCHECKs! Toss in a DCHECK(false) > > and it doesn't trigger. > > Hmmm okay (they probably should and I remember a discussion that someone wanted > to add them). Either way we need to support it (my dev build is > Release+dcheck_always_on). The right fix would be to update the EXPECT_DEBUG_DEATH or introduce EXPECT_DCHECK_DEATH. https://codereview.chromium.org/1706123002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:151: SchedulerLock lock3(&lock2); On 2016/02/19 22:05:24, gab wrote: > On 2016/02/19 21:25:06, robliao wrote: > > On 2016/02/19 16:08:03, gab wrote: > > > Looks like this test only needs two locks. Let's keep each test to the > minimal > > > state required. > > > > This is actually a little different from AcquireNonPredecessor, which is the > two > > lock case. > > This tests to make sure that you don't automatically get lock1 by > transitivity. > > > > Renamed the test > > Ah I see then I think we also need an "out-of-order" test (i.e. lock1, > lock2(&lock1), acquire(lock2), acquire(lock1), explode). Done. https://codereview.chromium.org/1706123002/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.h (right): https://codereview.chromium.org/1706123002/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.h:22: // See scheduler_lock.h for jdetails. On 2016/02/19 22:05:24, gab wrote: > s/jdetails/details/ Done. https://codereview.chromium.org/1706123002/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:174: } On 2016/02/19 22:05:24, gab wrote: > What is this now testing that the next two tests aren't? Looks like it will > undeterministically alternate between being either? This tests the absence of any predecessor relationship. It doesn't matter which lock is acquired first as long as they're on different threads.
lgtm with 2 nits https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock.h (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock.h:8: #include <vector> not needed here https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:69: std::unordered_map<base::PlatformThreadId, LockVector>; not used anymore
https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:42: AutoLock auto_lock(metadata_lock_); Move AutoLock into AssertSafeAcquire (on the line where there is an AssertAcquired()). https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:52: AutoLock auto_lock(metadata_lock_); No need to lock, right? https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:53: LockVector* acquired_locks = tls_acquired_locks_.Get(); DCHECK(acquired_locks); https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:60: delete acquired_locks; Assuming this thread remains active, it will soon grab another lock so instead of causing churn by deleting and re-allocating the LockVector. How about we instead only clear it in ~SchedulerLockImp(), i.e. when the WorkerThread is terminated. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:68: using AcquisitionMap = rm unused type https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:81: // We're getting ready to look at the allowed_predecessor_map_. When the line below is changed to an AutoLock as suggested above and the lock renamed to be predecessor specific, I don't think a comment will be required here as it will be implicit. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:104: return const_cast<SafeAcquisitionTracker*>(this)->tls_acquired_locks_.Get(); I'd prefer to inline this at single call site, otherwise there is an extra indirection for the reader for a one-liner. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:108: Lock metadata_lock_; // Synchronized access to |allowed_predecessor_map_|. Lock allowed_predecessor_lock_; // A map of allowed SchedulerLockImpl predecessors. PredecessorMap allowed_predecessor_map_; // A thread-local vector of currently acquired locks. ThreadLocalPointer<LockVector> tls_acquired_locks_; (i.e. rename the lock + suggestion for comments)
https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock.h (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock.h:8: #include <vector> On 2016/02/22 16:45:21, fdoray wrote: > not needed here Done. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:42: AutoLock auto_lock(metadata_lock_); On 2016/02/22 16:54:12, gab wrote: > Move AutoLock into AssertSafeAcquire (on the line where there is an > AssertAcquired()). Done. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:52: AutoLock auto_lock(metadata_lock_); On 2016/02/22 16:54:12, gab wrote: > No need to lock, right? Indeed. It's all TLS only. Removed. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:53: LockVector* acquired_locks = tls_acquired_locks_.Get(); On 2016/02/22 16:54:12, gab wrote: > DCHECK(acquired_locks); DCHECK isn't necessary here as we'll simply crash on the next line. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:60: delete acquired_locks; On 2016/02/22 16:54:12, gab wrote: > Assuming this thread remains active, it will soon grab another lock so instead > of causing churn by deleting and re-allocating the LockVector. How about we > instead only clear it in ~SchedulerLockImp(), i.e. when the WorkerThread is > terminated. The destruction of a lock does not necessarily correspond to an opportunity to clean up the single thread local state because a lock may be acquired on multiple threads. We would need to clean up the vectors for those other threads as well. We could create a map to hold all of these extra vectors and then verify that the threads are alive before destroying the vector, but then we're back to a non TLSed map. What we really want is to destroy the vector on the destruction of a real thread since that's where the TLS lives. We could modify ThreadLocalPointer to forward on a destructor call to do this on our behalf if that sounds appropriate to you. Alternatively, we can see if this alloc/free really causes a performance issue, and if it does, then we perform the ThreadLocalPointer change. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:68: using AcquisitionMap = On 2016/02/22 16:54:12, gab wrote: > rm unused type Done. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:69: std::unordered_map<base::PlatformThreadId, LockVector>; On 2016/02/22 16:45:21, fdoray wrote: > not used anymore Done. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:81: // We're getting ready to look at the allowed_predecessor_map_. On 2016/02/22 16:54:12, gab wrote: > When the line below is changed to an AutoLock as suggested above and the lock > renamed to be predecessor specific, I don't think a comment will be required > here as it will be implicit. Done. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:104: return const_cast<SafeAcquisitionTracker*>(this)->tls_acquired_locks_.Get(); On 2016/02/22 16:54:12, gab wrote: > I'd prefer to inline this at single call site, otherwise there is an extra > indirection for the reader for a one-liner. Done. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:108: Lock metadata_lock_; On 2016/02/22 16:54:12, gab wrote: > // Synchronized access to |allowed_predecessor_map_|. > Lock allowed_predecessor_lock_; > > // A map of allowed SchedulerLockImpl predecessors. > PredecessorMap allowed_predecessor_map_; > > // A thread-local vector of currently acquired locks. > ThreadLocalPointer<LockVector> tls_acquired_locks_; > > > (i.e. rename the lock + suggestion for comments) Done.
https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:53: LockVector* acquired_locks = tls_acquired_locks_.Get(); On 2016/02/22 18:38:09, robliao wrote: > On 2016/02/22 16:54:12, gab wrote: > > DCHECK(acquired_locks); > > DCHECK isn't necessary here as we'll simply crash on the next line. Right but DCHECK explicitly documents expectations whereas crash is unexpected. Hence I strongly prefer DCHECK here. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:60: delete acquired_locks; On 2016/02/22 18:38:09, robliao wrote: > On 2016/02/22 16:54:12, gab wrote: > > Assuming this thread remains active, it will soon grab another lock so instead > > of causing churn by deleting and re-allocating the LockVector. How about we > > instead only clear it in ~SchedulerLockImp(), i.e. when the WorkerThread is > > terminated. > > The destruction of a lock does not necessarily correspond to an opportunity to > clean up the single thread local state because a lock may be acquired on > multiple threads. We would need to clean up the vectors for those other threads > as well. > > We could create a map to hold all of these extra vectors and then verify that > the threads are alive before destroying the vector, but then we're back to a non > TLSed map. > > What we really want is to destroy the vector on the destruction of a real thread > since that's where the TLS lives. We could modify ThreadLocalPointer to forward > on a destructor call to do this on our behalf if that sounds appropriate to you. > > Alternatively, we can see if this alloc/free really causes a performance issue, > and if it does, then we perform the ThreadLocalPointer change. Duh of course, I did mean on destruction of thread but then somehow thought very incorrectly that we could rely on lock destruction instead of introducing a notification (clearly this makes no sense so not sure why I thought that!). How about a static SchedulerLock::OnThreadDestruction()? I'd rather not wait until we measure the impact, this feels suddle and hard to measure but we have an easy way to make it better. WDYT?
https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:53: LockVector* acquired_locks = tls_acquired_locks_.Get(); On 2016/02/22 19:56:47, gab wrote: > On 2016/02/22 18:38:09, robliao wrote: > > On 2016/02/22 16:54:12, gab wrote: > > > DCHECK(acquired_locks); > > > > DCHECK isn't necessary here as we'll simply crash on the next line. > > Right but DCHECK explicitly documents expectations whereas crash is unexpected. > Hence I strongly prefer DCHECK here. I used to be on the DCHECK pointers side as well until someone pointed out to me in a CR that a DCHECK right before a pointer dereference doesn't really add value to the code. By immediately dereferencing the pointer after you get it, you already document that you expect this pointer to be good. The DCHECK isn't adding any value because on DCHECK_IS_ON builds, once you assert, you immediately crash on the next line after you continue the DCHECK. On Release builds, you simply crash. The DCHECK then becomes an unnecessarily line. A DCHECK of a pointer provides value in cases when the pointer isn't immediately dereferenced but is dereferenced outside of the containing function. Constructors that accept pointers for later use are excellent places for a DCHECK of a pointer. https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:60: delete acquired_locks; On 2016/02/22 19:56:47, gab wrote: > On 2016/02/22 18:38:09, robliao wrote: > > On 2016/02/22 16:54:12, gab wrote: > > > Assuming this thread remains active, it will soon grab another lock so > instead > > > of causing churn by deleting and re-allocating the LockVector. How about we > > > instead only clear it in ~SchedulerLockImp(), i.e. when the WorkerThread is > > > terminated. > > > > The destruction of a lock does not necessarily correspond to an opportunity to > > clean up the single thread local state because a lock may be acquired on > > multiple threads. We would need to clean up the vectors for those other > threads > > as well. > > > > We could create a map to hold all of these extra vectors and then verify that > > the threads are alive before destroying the vector, but then we're back to a > non > > TLSed map. > > > > What we really want is to destroy the vector on the destruction of a real > thread > > since that's where the TLS lives. We could modify ThreadLocalPointer to > forward > > on a destructor call to do this on our behalf if that sounds appropriate to > you. > > > > Alternatively, we can see if this alloc/free really causes a performance > issue, > > and if it does, then we perform the ThreadLocalPointer change. > > Duh of course, I did mean on destruction of thread but then somehow thought very > incorrectly that we could rely on lock destruction instead of introducing a > notification (clearly this makes no sense so not sure why I thought that!). > > How about a static SchedulerLock::OnThreadDestruction()? > > I'd rather not wait until we measure the impact, this feels suddle and hard to > measure but we have an easy way to make it better. WDYT? The right way to do this is to introduce the destructor that already runs with the TLS slot. I'll go ahead and add that in.
https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:53: LockVector* acquired_locks = tls_acquired_locks_.Get(); On 2016/02/22 20:22:51, robliao wrote: > On 2016/02/22 19:56:47, gab wrote: > > On 2016/02/22 18:38:09, robliao wrote: > > > On 2016/02/22 16:54:12, gab wrote: > > > > DCHECK(acquired_locks); > > > > > > DCHECK isn't necessary here as we'll simply crash on the next line. > > > > Right but DCHECK explicitly documents expectations whereas crash is > unexpected. > > Hence I strongly prefer DCHECK here. > > I used to be on the DCHECK pointers side as well until someone pointed out to me > in a CR that a DCHECK right before a pointer dereference doesn't really add > value to the code. > > By immediately dereferencing the pointer after you get it, you already document > that you expect this pointer to be good. The DCHECK isn't adding any value > because on DCHECK_IS_ON builds, once you assert, you immediately crash on the > next line after you continue the DCHECK. On Release builds, you simply crash. > The DCHECK then becomes an unnecessarily line. > > A DCHECK of a pointer provides value in cases when the pointer isn't immediately > dereferenced but is dereferenced outside of the containing function. > Constructors that accept pointers for later use are excellent places for a > DCHECK of a pointer. To me it acts as explicit documentation whereas immediate deref is implicit documentation (i.e. could be a developer/reviewer inattention whereas DCHECK can't be accidental). If the DCHECK hits, the developer immediately knows it's wrong. If it crashes on deref, the developer has to read the rest of the code to figure out whether this should never be the case. tl;dr; to me a DCHECK is documentation that says "we're now going to deref right away because this should never be null per the impl" without the reader having to wonder whether that's true.
https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:60: delete acquired_locks; On 2016/02/22 20:22:51, robliao wrote: > On 2016/02/22 19:56:47, gab wrote: > > On 2016/02/22 18:38:09, robliao wrote: > > > On 2016/02/22 16:54:12, gab wrote: > > > > Assuming this thread remains active, it will soon grab another lock so > > instead > > > > of causing churn by deleting and re-allocating the LockVector. How about > we > > > > instead only clear it in ~SchedulerLockImp(), i.e. when the WorkerThread > is > > > > terminated. > > > > > > The destruction of a lock does not necessarily correspond to an opportunity > to > > > clean up the single thread local state because a lock may be acquired on > > > multiple threads. We would need to clean up the vectors for those other > > threads > > > as well. > > > > > > We could create a map to hold all of these extra vectors and then verify > that > > > the threads are alive before destroying the vector, but then we're back to a > > non > > > TLSed map. > > > > > > What we really want is to destroy the vector on the destruction of a real > > thread > > > since that's where the TLS lives. We could modify ThreadLocalPointer to > > forward > > > on a destructor call to do this on our behalf if that sounds appropriate to > > you. > > > > > > Alternatively, we can see if this alloc/free really causes a performance > > issue, > > > and if it does, then we perform the ThreadLocalPointer change. > > > > Duh of course, I did mean on destruction of thread but then somehow thought > very > > incorrectly that we could rely on lock destruction instead of introducing a > > notification (clearly this makes no sense so not sure why I thought that!). > > > > How about a static SchedulerLock::OnThreadDestruction()? > > > > I'd rather not wait until we measure the impact, this feels suddle and hard to > > measure but we have an easy way to make it better. WDYT? > > The right way to do this is to introduce the destructor that already runs with > the TLS slot. I'll go ahead and add that in. Looks like I incorrectly assumed that this API was using the ThreadLocalStorage API, but it turns out this API was supposed to supersede that API, but it looks like it was never removed! https://chromium.googlesource.com/chromium/src/+/83a05ddf63cc5920ad04b0a6936f... This will require a bit more cleanup than I expected.
Patchset #6 (id:220001) has been deleted
Patchset #6 (id:240001) has been deleted
Patchset #6 (id:260001) has been deleted
https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/180001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:53: LockVector* acquired_locks = tls_acquired_locks_.Get(); On 2016/02/22 20:36:31, gab wrote: > On 2016/02/22 20:22:51, robliao wrote: > > On 2016/02/22 19:56:47, gab wrote: > > > On 2016/02/22 18:38:09, robliao wrote: > > > > On 2016/02/22 16:54:12, gab wrote: > > > > > DCHECK(acquired_locks); > > > > > > > > DCHECK isn't necessary here as we'll simply crash on the next line. > > > > > > Right but DCHECK explicitly documents expectations whereas crash is > > unexpected. > > > Hence I strongly prefer DCHECK here. > > > > I used to be on the DCHECK pointers side as well until someone pointed out to > me > > in a CR that a DCHECK right before a pointer dereference doesn't really add > > value to the code. > > > > By immediately dereferencing the pointer after you get it, you already > document > > that you expect this pointer to be good. The DCHECK isn't adding any value > > because on DCHECK_IS_ON builds, once you assert, you immediately crash on the > > next line after you continue the DCHECK. On Release builds, you simply crash. > > The DCHECK then becomes an unnecessarily line. > > > > A DCHECK of a pointer provides value in cases when the pointer isn't > immediately > > dereferenced but is dereferenced outside of the containing function. > > Constructors that accept pointers for later use are excellent places for a > > DCHECK of a pointer. > > To me it acts as explicit documentation whereas immediate deref is implicit > documentation (i.e. could be a developer/reviewer inattention whereas DCHECK > can't be accidental). > > If the DCHECK hits, the developer immediately knows it's wrong. If it crashes on > deref, the developer has to read the rest of the code to figure out whether this > should never be the case. > > tl;dr; to me a DCHECK is documentation that says "we're now going to deref right > away because this should never be null per the impl" without the reader having > to wonder whether that's true. Discussed offline and rendered moot with the allowance for empty vectors.
lgtm % nits, thanks! https://codereview.chromium.org/1706123002/diff/280001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:24: SafeAcquisitionTracker() : tls_acquired_locks_(DestroyLockVector) {} Shouldn't this be &DestroyLockVector? FWICT @ [1] this takes a pointer to a method. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... https://codereview.chromium.org/1706123002/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:94: static void DestroyLockVector(void* value) { How about HandleTLSLockVectorDestruction()? DestroyLockVector() feels a bit too generic for its purpose IMO.
https://codereview.chromium.org/1706123002/diff/280001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:24: SafeAcquisitionTracker() : tls_acquired_locks_(DestroyLockVector) {} On 2016/02/22 21:52:10, gab wrote: > Shouldn't this be &DestroyLockVector? FWICT @ [1] this takes a pointer to a > method. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... Indeed it should. C++ is pretty permissive with regards to the & operator and function pointers. https://codereview.chromium.org/1706123002/diff/280001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:94: static void DestroyLockVector(void* value) { On 2016/02/22 21:52:10, gab wrote: > How about HandleTLSLockVectorDestruction()? DestroyLockVector() feels a bit too > generic for its purpose IMO. Went with OnTLSDestroy since this is called on the destruction of the TLS.
robliao@chromium.org changed reviewers: + jam@chromium.org
jam: Please review this CL. Thanks!
jam@chromium.org changed reviewers: + brettw@chromium.org
+brettw as well for his thoughts (original base owner, and familiar with this work). btw please distribute changes across us (or other reviewers), since brett is back now. With my skeptical hat on, I have to ask if we really need this. a lot of other places in the code use multiple locks and we haven't had the need to introduce yet another locking wrappers to the codebase. Especially since TSAN already provides this deadlock detection for you.
On 2016/02/22 23:33:16, jam wrote: > +brettw as well for his thoughts (original base owner, and familiar with this > work). btw please distribute changes across us (or other reviewers), since brett > is back now. > > With my skeptical hat on, I have to ask if we really need this. a lot of other > places in the code use multiple locks and we haven't had the need to introduce > yet another locking wrappers to the codebase. Especially since TSAN already > provides this deadlock detection for you. We have a well defined locking model for the scheduler and the goal of the scheduler lock is to enforce that. During development, we actually found a lock violation in our unit tests that was caught by acquisition tracking. If we need to add any more synchronization, the SchedulerLock will help us ensure that this synchronization is safe in an immediate manner.
On 2016/02/22 23:33:16, jam wrote: > +brettw as well for his thoughts (original base owner, and familiar with this > work). btw please distribute changes across us (or other reviewers), since brett > is back now. > > With my skeptical hat on, I have to ask if we really need this. a lot of other > places in the code use multiple locks and we haven't had the need to introduce > yet another locking wrappers to the codebase. Especially since TSAN already > provides this deadlock detection for you. While the scheduler is trying hard to not hold two locks at once, it has to for some use cases. There also isn't always a clear hierarchy as callbacks allow leaf components to call into parent components. We are fairly convinced the current implementation is correct for now (uses a sane locking hierarchy) but we decided that introducing lock checking would be safer after we walked in a tricky corner case that could actually have caused a deadlock a few weeks ago (now fixed). We figured it would serve both to prove the current implementation is correct as well as prevent future regressions. And furthermore, it acts as documentation of the locking hierarchy to an eventual reader. Does TSAN cover deadlock possibilities? I thought it only detected race conditions. Where are the TSAN bots? I don't seem them on CQ nor on waterfall. All I can find are "TSAN v2 Linux" bots on the Memory FYI waterfall, are we happy to rely on those?
On 2016/02/22 23:51:51, gab wrote: > On 2016/02/22 23:33:16, jam wrote: > > +brettw as well for his thoughts (original base owner, and familiar with this > > work). btw please distribute changes across us (or other reviewers), since > brett > > is back now. > > > > With my skeptical hat on, I have to ask if we really need this. a lot of other > > places in the code use multiple locks and we haven't had the need to introduce > > yet another locking wrappers to the codebase. Especially since TSAN already > > provides this deadlock detection for you. > > While the scheduler is trying hard to not hold two locks at once, it has to for > some use cases. There also isn't always a clear hierarchy as callbacks allow > leaf components to call into parent components. > > We are fairly convinced the current implementation is correct for now (uses a > sane locking hierarchy) but we decided that introducing lock checking would be > safer after we walked in a tricky corner case that could actually have caused a > deadlock a few weeks ago (now fixed). > > We figured it would serve both to prove the current implementation is correct as > well as prevent future regressions. And furthermore, it acts as documentation of > the locking hierarchy to an eventual reader. > > Does TSAN cover deadlock possibilities? yep, see https://www.chromium.org/developers/testing/threadsanitizer-tsan-v2 https://github.com/google/sanitizers/wiki/ThreadSanitizerDeadlockDetector > I thought it only detected race > conditions. Where are the TSAN bots? I don't seem them on CQ nor on waterfall. > All I can find are "TSAN v2 Linux" bots on the Memory FYI waterfall, correct that's where they are. there are also non-default CQ bots. > are we > happy to rely on those? The first question I have is do you think this code will change and you'll need this? The second question is, given that this code in base/, it's easy for anyone else to start using this and then even if you remove your usage you won't be able to remove it. so introducing any new locking class in base/ seems like it should have a review from base/owners.
gab@chromium.org changed reviewers: + thakis@chromium.org
+Nico who will be our base reviewer for these 8 CLs. @Nico see discussion below. On 2016/02/22 23:59:13, jam wrote: > On 2016/02/22 23:51:51, gab wrote: > > On 2016/02/22 23:33:16, jam wrote: > > > +brettw as well for his thoughts (original base owner, and familiar with > this > > > work). btw please distribute changes across us (or other reviewers), since > > brett > > > is back now. > > > > > > With my skeptical hat on, I have to ask if we really need this. a lot of > other > > > places in the code use multiple locks and we haven't had the need to > introduce > > > yet another locking wrappers to the codebase. Especially since TSAN already > > > provides this deadlock detection for you. > > > > While the scheduler is trying hard to not hold two locks at once, it has to > for > > some use cases. There also isn't always a clear hierarchy as callbacks allow > > leaf components to call into parent components. > > > > We are fairly convinced the current implementation is correct for now (uses a > > sane locking hierarchy) but we decided that introducing lock checking would be > > safer after we walked in a tricky corner case that could actually have caused > a > > deadlock a few weeks ago (now fixed). > > > > We figured it would serve both to prove the current implementation is correct > as > > well as prevent future regressions. And furthermore, it acts as documentation > of > > the locking hierarchy to an eventual reader. > > > > Does TSAN cover deadlock possibilities? > > yep, see > https://www.chromium.org/developers/testing/threadsanitizer-tsan-v2 > https://github.com/google/sanitizers/wiki/ThreadSanitizerDeadlockDetector > > > I thought it only detected race > > conditions. Where are the TSAN bots? I don't seem them on CQ nor on waterfall. > > All I can find are "TSAN v2 Linux" bots on the Memory FYI waterfall, > > correct that's where they are. there are also non-default CQ bots. > > > are we > > happy to rely on those? > > The first question I have is do you think this code will change and you'll need > this? Yes it probably will as we expand to support more TaskTraits. One example is timers which are #1 request on our list to eventually support the renderer process' use case. This means delayed task on the multi-threaded non-background side of things which implies more locks. > The second question is, given that this code in base/, it's easy for anyone else > to start using this and then even if you remove your usage you won't be able to > remove it. so introducing any new locking class in base/ seems like it should > have a review from base/owners. We explicitly put it in base::internal:: and named it SchedulerLock to make it clear it was specific to this impl. I doubt anyone would think it's the right thing to use this in some random place outside of base/task_scheduler. In fact SchedulerLock isn't even a BASE_EXPORT so this is even less likely (SchedulerLockImpl has to be for unit_tests sadly).
On 2016/02/23 01:16:26, gab wrote: > +Nico who will be our base reviewer for these 8 CLs. > > @Nico see discussion below. > > On 2016/02/22 23:59:13, jam wrote: > > On 2016/02/22 23:51:51, gab wrote: > > > On 2016/02/22 23:33:16, jam wrote: > > > > +brettw as well for his thoughts (original base owner, and familiar with > > this > > > > work). btw please distribute changes across us (or other reviewers), since > > > brett > > > > is back now. > > > > > > > > With my skeptical hat on, I have to ask if we really need this. a lot of > > other > > > > places in the code use multiple locks and we haven't had the need to > > introduce > > > > yet another locking wrappers to the codebase. Especially since TSAN > already > > > > provides this deadlock detection for you. > > > > > > While the scheduler is trying hard to not hold two locks at once, it has to > > for > > > some use cases. There also isn't always a clear hierarchy as callbacks allow > > > leaf components to call into parent components. > > > > > > We are fairly convinced the current implementation is correct for now (uses > a > > > sane locking hierarchy) but we decided that introducing lock checking would > be > > > safer after we walked in a tricky corner case that could actually have > caused > > a > > > deadlock a few weeks ago (now fixed). > > > > > > We figured it would serve both to prove the current implementation is > correct > > as > > > well as prevent future regressions. And furthermore, it acts as > documentation > > of > > > the locking hierarchy to an eventual reader. > > > > > > Does TSAN cover deadlock possibilities? > > > > yep, see > > https://www.chromium.org/developers/testing/threadsanitizer-tsan-v2 > > https://github.com/google/sanitizers/wiki/ThreadSanitizerDeadlockDetector > > > > > I thought it only detected race > > > conditions. Where are the TSAN bots? I don't seem them on CQ nor on > waterfall. > > > All I can find are "TSAN v2 Linux" bots on the Memory FYI waterfall, > > > > correct that's where they are. there are also non-default CQ bots. > > > > > are we > > > happy to rely on those? > > > > The first question I have is do you think this code will change and you'll > need > > this? > > Yes it probably will as we expand to support more TaskTraits. One example is > timers which are #1 request on our list to eventually support the renderer > process' use case. This means delayed task on the multi-threaded non-background > side of things which implies more locks. > > > The second question is, given that this code in base/, it's easy for anyone > else > > to start using this and then even if you remove your usage you won't be able > to > > remove it. so introducing any new locking class in base/ seems like it should > > have a review from base/owners. > > We explicitly put it in base::internal:: and named it SchedulerLock to make it > clear it was specific to this impl. I doubt anyone would think it's the right > thing to use this in some random place outside of base/task_scheduler. > > In fact SchedulerLock isn't even a BASE_EXPORT so this is even less likely > (SchedulerLockImpl has to be for unit_tests sadly). Ping for thakis. Thanks!
gab@chromium.org changed reviewers: + danakj@chromium.org
+Dana, as just discussed :-), thanks! On 2016/02/25 18:02:27, robliao wrote: > On 2016/02/23 01:16:26, gab wrote: > > +Nico who will be our base reviewer for these 8 CLs. > > > > @Nico see discussion below. > > > > On 2016/02/22 23:59:13, jam wrote: > > > On 2016/02/22 23:51:51, gab wrote: > > > > On 2016/02/22 23:33:16, jam wrote: > > > > > +brettw as well for his thoughts (original base owner, and familiar with > > > this > > > > > work). btw please distribute changes across us (or other reviewers), > since > > > > brett > > > > > is back now. > > > > > > > > > > With my skeptical hat on, I have to ask if we really need this. a lot of > > > other > > > > > places in the code use multiple locks and we haven't had the need to > > > introduce > > > > > yet another locking wrappers to the codebase. Especially since TSAN > > already > > > > > provides this deadlock detection for you. > > > > > > > > While the scheduler is trying hard to not hold two locks at once, it has > to > > > for > > > > some use cases. There also isn't always a clear hierarchy as callbacks > allow > > > > leaf components to call into parent components. > > > > > > > > We are fairly convinced the current implementation is correct for now > (uses > > a > > > > sane locking hierarchy) but we decided that introducing lock checking > would > > be > > > > safer after we walked in a tricky corner case that could actually have > > caused > > > a > > > > deadlock a few weeks ago (now fixed). > > > > > > > > We figured it would serve both to prove the current implementation is > > correct > > > as > > > > well as prevent future regressions. And furthermore, it acts as > > documentation > > > of > > > > the locking hierarchy to an eventual reader. > > > > > > > > Does TSAN cover deadlock possibilities? > > > > > > yep, see > > > https://www.chromium.org/developers/testing/threadsanitizer-tsan-v2 > > > https://github.com/google/sanitizers/wiki/ThreadSanitizerDeadlockDetector > > > > > > > I thought it only detected race > > > > conditions. Where are the TSAN bots? I don't seem them on CQ nor on > > waterfall. > > > > All I can find are "TSAN v2 Linux" bots on the Memory FYI waterfall, > > > > > > correct that's where they are. there are also non-default CQ bots. > > > > > > > are we > > > > happy to rely on those? > > > > > > The first question I have is do you think this code will change and you'll > > need > > > this? > > > > Yes it probably will as we expand to support more TaskTraits. One example is > > timers which are #1 request on our list to eventually support the renderer > > process' use case. This means delayed task on the multi-threaded > non-background > > side of things which implies more locks. > > > > > The second question is, given that this code in base/, it's easy for anyone > > else > > > to start using this and then even if you remove your usage you won't be able > > to > > > remove it. so introducing any new locking class in base/ seems like it > should > > > have a review from base/owners. > > > > We explicitly put it in base::internal:: and named it SchedulerLock to make it > > clear it was specific to this impl. I doubt anyone would think it's the right > > thing to use this in some random place outside of base/task_scheduler. > > > > In fact SchedulerLock isn't even a BASE_EXPORT so this is even less likely > > (SchedulerLockImpl has to be for unit_tests sadly). > > Ping for thakis. Thanks!
robliao@chromium.org changed reviewers: - brettw@chromium.org, jam@chromium.org, thakis@chromium.org
On 2016/03/03 00:38:57, gab wrote: > +Dana, as just discussed :-), thanks! > > On 2016/02/25 18:02:27, robliao wrote: > > On 2016/02/23 01:16:26, gab wrote: > > > +Nico who will be our base reviewer for these 8 CLs. > > > > > > @Nico see discussion below. > > > > > > On 2016/02/22 23:59:13, jam wrote: > > > > On 2016/02/22 23:51:51, gab wrote: > > > > > On 2016/02/22 23:33:16, jam wrote: > > > > > > +brettw as well for his thoughts (original base owner, and familiar > with > > > > this > > > > > > work). btw please distribute changes across us (or other reviewers), > > since > > > > > brett > > > > > > is back now. > > > > > > > > > > > > With my skeptical hat on, I have to ask if we really need this. a lot > of > > > > other > > > > > > places in the code use multiple locks and we haven't had the need to > > > > introduce > > > > > > yet another locking wrappers to the codebase. Especially since TSAN > > > already > > > > > > provides this deadlock detection for you. > > > > > > > > > > While the scheduler is trying hard to not hold two locks at once, it has > > to > > > > for > > > > > some use cases. There also isn't always a clear hierarchy as callbacks > > allow > > > > > leaf components to call into parent components. > > > > > > > > > > We are fairly convinced the current implementation is correct for now > > (uses > > > a > > > > > sane locking hierarchy) but we decided that introducing lock checking > > would > > > be > > > > > safer after we walked in a tricky corner case that could actually have > > > caused > > > > a > > > > > deadlock a few weeks ago (now fixed). > > > > > > > > > > We figured it would serve both to prove the current implementation is > > > correct > > > > as > > > > > well as prevent future regressions. And furthermore, it acts as > > > documentation > > > > of > > > > > the locking hierarchy to an eventual reader. > > > > > > > > > > Does TSAN cover deadlock possibilities? > > > > > > > > yep, see > > > > https://www.chromium.org/developers/testing/threadsanitizer-tsan-v2 > > > > https://github.com/google/sanitizers/wiki/ThreadSanitizerDeadlockDetector > > > > > > > > > I thought it only detected race > > > > > conditions. Where are the TSAN bots? I don't seem them on CQ nor on > > > waterfall. > > > > > All I can find are "TSAN v2 Linux" bots on the Memory FYI waterfall, > > > > > > > > correct that's where they are. there are also non-default CQ bots. > > > > > > > > > are we > > > > > happy to rely on those? > > > > > > > > The first question I have is do you think this code will change and you'll > > > need > > > > this? > > > > > > Yes it probably will as we expand to support more TaskTraits. One example is > > > timers which are #1 request on our list to eventually support the renderer > > > process' use case. This means delayed task on the multi-threaded > > non-background > > > side of things which implies more locks. > > > > > > > The second question is, given that this code in base/, it's easy for > anyone > > > else > > > > to start using this and then even if you remove your usage you won't be > able > > > to > > > > remove it. so introducing any new locking class in base/ seems like it > > should > > > > have a review from base/owners. > > > > > > We explicitly put it in base::internal:: and named it SchedulerLock to make > it > > > clear it was specific to this impl. I doubt anyone would think it's the > right > > > thing to use this in some random place outside of base/task_scheduler. > > > > > > In fact SchedulerLock isn't even a BASE_EXPORT so this is even less likely > > > (SchedulerLockImpl has to be for unit_tests sadly). > > > > Ping for thakis. Thanks! Cleaning up the reviewers list.
LG overall thanks for all the tests. Few comments. https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:82: if (predecessor == lock) nit: prefer DCHECK(predecessor != lock) rather than if () NOTREACHED(); Can you maybe add a string to it << "Cycle detected in lock predecessors" or so? https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:99: mutable Lock allowed_predecessor_map_lock_; Why mutable? https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:98: PlatformThread::Sleep(TimeDelta::FromMilliseconds(rand() % 20)); Why is it base::RandInt(0, 19) above but rand() % 20 here? https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:126: EXPECT_DEBUG_DEATH({ EXPECT_DEBUG_DEATH asserts that the given statements die in debug mode. But this will die on our bots in release builds too with DCHECKs on. I think you want to do #if DCHECK_IS_ON() ... EXPECT_DEATH(...) #endif https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:206: AcquireLocksWithPredecessorDifferentThreadsSafelyPredecessorLast) { One more test case worth mentioning maybe? This thread Other thread predecessor.Acquire() unrelated.Acquire() lock.Acquire() To be sure the other thread doesn't mess with the bookkeeping. This test here kinda covers that, cuz if it messed with it, lock would be held first which is out of order. But it's slightly different in that the held locks are empty when it has a chance to mess with it.
https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:19: Oh, can you wrap the whole test file in a nested anonymous namespace? Helps avoid name collisions between tests.
Thanks for the review! New patchset uploaded. https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:82: if (predecessor == lock) On 2016/03/08 21:22:56, danakj wrote: > nit: prefer DCHECK(predecessor != lock) rather than if () NOTREACHED(); > > Can you maybe add a string to it << "Cycle detected in lock predecessors" or so? Done with "Scheduler lock predecessor cycle detected." https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:99: mutable Lock allowed_predecessor_map_lock_; On 2016/03/08 21:22:56, danakj wrote: > Why mutable? Removed. In an earlier revision AssertSafeAcquire was const. That went out the window with GetAcquiredLocksOnCurrentThread, so the mutable is no longer needed. https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:19: On 2016/03/08 21:24:50, danakj wrote: > Oh, can you wrap the whole test file in a nested anonymous namespace? Helps > avoid name collisions between tests. Done. https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:98: PlatformThread::Sleep(TimeDelta::FromMilliseconds(rand() % 20)); On 2016/03/08 21:22:56, danakj wrote: > Why is it base::RandInt(0, 19) above but rand() % 20 here? Incomplete migration from the original lock unittests. Fixed. https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:126: EXPECT_DEBUG_DEATH({ On 2016/03/08 21:22:56, danakj wrote: > EXPECT_DEBUG_DEATH asserts that the given statements die in debug mode. > > But this will die on our bots in release builds too with DCHECKs on. I think you > want to do > > #if DCHECK_IS_ON() > ... > EXPECT_DEATH(...) > #endif Changed to #if DCHECK_IS_ON() #define EXPECT_DCHECK_DEATH(statement, regex) EXPECT_DEATH(statement, regex) #else #define EXPECT_DCHECK_DEATH(statement, regex) #endif and modified callers to EXPECT_DCHECK_DEATH https://codereview.chromium.org/1706123002/diff/320001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:206: AcquireLocksWithPredecessorDifferentThreadsSafelyPredecessorLast) { On 2016/03/08 21:22:56, danakj wrote: > One more test case worth mentioning maybe? > > This thread Other thread > > predecessor.Acquire() > unrelated.Acquire() > lock.Acquire() > > > To be sure the other thread doesn't mess with the bookkeeping. > > This test here kinda covers that, cuz if it messed with it, lock would be held > first which is out of order. But it's slightly different in that the held locks > are empty when it has a chance to mess with it. I'll buy that. Added a case.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706123002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706123002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706123002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706123002/360001
Patchset #10 (id:360001) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706123002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706123002/380001
Patchset #10 (id:380001) has been deleted
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706123002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706123002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706123002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706123002/400001
On 2016/03/09 17:12:52, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Ping for danakj@. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706123002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706123002/400001
LGTM https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:82: DCHECK(predecessor != lock) << On 2016/03/08 22:14:08, robliao wrote: > On 2016/03/08 21:22:56, danakj wrote: > > nit: prefer DCHECK(predecessor != lock) rather than if () NOTREACHED(); > > > > Can you maybe add a string to it << "Cycle detected in lock predecessors" or > so? > > Done with "Scheduler lock predecessor cycle detected." Now that I read this, it should use DCHECK_NE I guess.
Thanks Rob/Dana, a minor comment on the latest changes. https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:25: #define EXPECT_DCHECK_DEATH(statement, regex) Shouldn't this be #define EXPECT_DCHECK_DEATH(statement, regex) statement ? To at least run the statement in non-dcheck scenarios? Otherwise any statement that comes after the death test and depends on its state would fail? Also, if we do this then we can put only the specific statement that is expected to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and improve readability + making sure we test what we expect?
https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:25: #define EXPECT_DCHECK_DEATH(statement, regex) On 2016/03/14 18:26:04, gab wrote: > Shouldn't this be > #define EXPECT_DCHECK_DEATH(statement, regex) statement > ? > > To at least run the statement in non-dcheck scenarios? Otherwise any statement > that comes after the death test and depends on its state would fail? > > Also, if we do this then we can put only the specific statement that is expected > to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and improve > readability + making sure we test what we expect? Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH runs in a new process. That's why we sometimes have multiple statements inside the macro. Consider this: 1. lock.Acquire(); 2. EXPECT_DCHECK_DEATH({ 3. predecessor.Acquire(); 4. }, ""); On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs. On Posix, lines 1 and 3 run on different threads -> no DCHECK. We must write the test like this to ensure that |predecessor.Acquire()| DCHECKs on every platform: EXPECT_DCHECK_DEATH({ lock.Acquire(); predecessor.Acquire(); }, "");
On 2016/03/14 18:55:52, fdoray wrote: > https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... > File base/task_scheduler/scheduler_lock_unittest.cc (right): > > https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... > base/task_scheduler/scheduler_lock_unittest.cc:25: #define > EXPECT_DCHECK_DEATH(statement, regex) > On 2016/03/14 18:26:04, gab wrote: > > Shouldn't this be > > #define EXPECT_DCHECK_DEATH(statement, regex) statement > > ? > > > > To at least run the statement in non-dcheck scenarios? Otherwise any statement > > that comes after the death test and depends on its state would fail? > > > > Also, if we do this then we can put only the specific statement that is > expected > > to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and improve > > readability + making sure we test what we expect? > > Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH runs in a new > process. That's why we sometimes have multiple statements inside the macro. > > Consider this: > > 1. lock.Acquire(); > 2. EXPECT_DCHECK_DEATH({ > 3. predecessor.Acquire(); > 4. }, ""); > > On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs. > On Posix, lines 1 and 3 run on different threads -> no DCHECK. > > We must write the test like this to ensure that |predecessor.Acquire()| DCHECKs > on every platform: > > EXPECT_DCHECK_DEATH({ > lock.Acquire(); > predecessor.Acquire(); > }, ""); If things are feeling complicated here, you can just #if DCHECK_IS_ON() TEST(Foo, Bar) { ... } #endif
Updated most of the DCHECKs except one (requires operator<< for iterator) https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:25: #define EXPECT_DCHECK_DEATH(statement, regex) On 2016/03/14 18:55:52, fdoray wrote: > On 2016/03/14 18:26:04, gab wrote: > > Shouldn't this be > > #define EXPECT_DCHECK_DEATH(statement, regex) statement > > ? > > > > To at least run the statement in non-dcheck scenarios? Otherwise any statement > > that comes after the death test and depends on its state would fail? > > > > Also, if we do this then we can put only the specific statement that is > expected > > to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and improve > > readability + making sure we test what we expect? > > Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH runs in a new > process. That's why we sometimes have multiple statements inside the macro. > > Consider this: > > 1. lock.Acquire(); > 2. EXPECT_DCHECK_DEATH({ > 3. predecessor.Acquire(); > 4. }, ""); > > On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs. > On Posix, lines 1 and 3 run on different threads -> no DCHECK. > > We must write the test like this to ensure that |predecessor.Acquire()| DCHECKs > on every platform: > > EXPECT_DCHECK_DEATH({ > lock.Acquire(); > predecessor.Acquire(); > }, ""); The way we've structured the tests, we expect death as the last statement. Given that the last statement will fail, there shouldn't be statements after that point because now you're in a strange state. Other tests simply don't run these tests when DCHECK_IS_ON(), and we've followed the spirit of that with EXPECT_DCHECK_DEATH. +1 to fdoray's comment. The first CL's actually had the statement that caused problems alone in EXPECT_DEATH, but that caused problems with POSIX since POSIX runs a fork from that point. Windows re-executes the entire test. https://codereview.chromium.org/1706123002/diff/420001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/420001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:49: DCHECK(iter_at_lock != acquired_locks->end()); Note: This can't be converted as there's no operator<< for iterators.
https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_impl.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_impl.cc:82: DCHECK(predecessor != lock) << On 2016/03/14 18:00:36, danakj wrote: > On 2016/03/08 22:14:08, robliao wrote: > > On 2016/03/08 21:22:56, danakj wrote: > > > nit: prefer DCHECK(predecessor != lock) rather than if () NOTREACHED(); > > > > > > Can you maybe add a string to it << "Cycle detected in lock predecessors" or > > so? > > > > Done with "Scheduler lock predecessor cycle detected." > > Now that I read this, it should use DCHECK_NE I guess. Done.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706123002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706123002/420001
Description was changed from ========== TaskScheduler [2/8] Scheduler Lock The task scheduler lock helps ensure correctness of locks by verifying that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock is a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [2/9] Scheduler Lock The task scheduler lock helps ensure correctness of locks by verifying that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock is a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Thanks LGTM
https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:25: #define EXPECT_DCHECK_DEATH(statement, regex) On 2016/03/14 19:09:52, robliao wrote: > On 2016/03/14 18:55:52, fdoray wrote: > > On 2016/03/14 18:26:04, gab wrote: > > > Shouldn't this be > > > #define EXPECT_DCHECK_DEATH(statement, regex) statement > > > ? > > > > > > To at least run the statement in non-dcheck scenarios? Otherwise any > statement > > > that comes after the death test and depends on its state would fail? > > > > > > Also, if we do this then we can put only the specific statement that is > > expected > > > to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and improve > > > readability + making sure we test what we expect? > > > > Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH runs in a new > > process. That's why we sometimes have multiple statements inside the macro. > > > > Consider this: > > > > 1. lock.Acquire(); > > 2. EXPECT_DCHECK_DEATH({ > > 3. predecessor.Acquire(); > > 4. }, ""); > > > > On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs. > > On Posix, lines 1 and 3 run on different threads -> no DCHECK. > > > > We must write the test like this to ensure that |predecessor.Acquire()| > DCHECKs > > on every platform: > > > > EXPECT_DCHECK_DEATH({ > > lock.Acquire(); > > predecessor.Acquire(); > > }, ""); > > The way we've structured the tests, we expect death as the last statement. Given > that the last statement will fail, there shouldn't be statements after that > point because now you're in a strange state. > > Other tests simply don't run these tests when DCHECK_IS_ON(), and we've followed > the spirit of that with EXPECT_DCHECK_DEATH. > > +1 to fdoray's comment. The first CL's actually had the statement that caused > problems alone in EXPECT_DEATH, but that caused problems with POSIX since POSIX > runs a fork from that point. Windows re-executes the entire test. Ok, re-lgtm. PS: Would be nice to confirm that try bots indeed cover DCHECKs. Maybe intentionally break something in this CL in a patch set to verify the tests react as intended?
https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:25: #define EXPECT_DCHECK_DEATH(statement, regex) On 2016/03/14 19:51:13, gab wrote: > On 2016/03/14 19:09:52, robliao wrote: > > On 2016/03/14 18:55:52, fdoray wrote: > > > On 2016/03/14 18:26:04, gab wrote: > > > > Shouldn't this be > > > > #define EXPECT_DCHECK_DEATH(statement, regex) statement > > > > ? > > > > > > > > To at least run the statement in non-dcheck scenarios? Otherwise any > > statement > > > > that comes after the death test and depends on its state would fail? > > > > > > > > Also, if we do this then we can put only the specific statement that is > > > expected > > > > to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and improve > > > > readability + making sure we test what we expect? > > > > > > Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH runs in a new > > > process. That's why we sometimes have multiple statements inside the macro. > > > > > > Consider this: > > > > > > 1. lock.Acquire(); > > > 2. EXPECT_DCHECK_DEATH({ > > > 3. predecessor.Acquire(); > > > 4. }, ""); > > > > > > On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs. > > > On Posix, lines 1 and 3 run on different threads -> no DCHECK. > > > > > > We must write the test like this to ensure that |predecessor.Acquire()| > > DCHECKs > > > on every platform: > > > > > > EXPECT_DCHECK_DEATH({ > > > lock.Acquire(); > > > predecessor.Acquire(); > > > }, ""); > > > > The way we've structured the tests, we expect death as the last statement. > Given > > that the last statement will fail, there shouldn't be statements after that > > point because now you're in a strange state. > > > > Other tests simply don't run these tests when DCHECK_IS_ON(), and we've > followed > > the spirit of that with EXPECT_DCHECK_DEATH. > > > > +1 to fdoray's comment. The first CL's actually had the statement that caused > > problems alone in EXPECT_DEATH, but that caused problems with POSIX since > POSIX > > runs a fork from that point. Windows re-executes the entire test. > > Ok, re-lgtm. > > PS: Would be nice to confirm that try bots indeed cover DCHECKs. Maybe > intentionally break something in this CL in a patch set to verify the tests > react as intended? From the tests so far, the mobile ones at least do :-). The android ones were very flaky, so I had to disable them. For the other bots, we can see dcheck_always_on=1 (win_chromium_x64_rel_ng), so we should have good confidence that the DCHECK tests are being run. https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...
https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:25: #define EXPECT_DCHECK_DEATH(statement, regex) On 2016/03/14 19:55:41, robliao wrote: > On 2016/03/14 19:51:13, gab wrote: > > On 2016/03/14 19:09:52, robliao wrote: > > > On 2016/03/14 18:55:52, fdoray wrote: > > > > On 2016/03/14 18:26:04, gab wrote: > > > > > Shouldn't this be > > > > > #define EXPECT_DCHECK_DEATH(statement, regex) statement > > > > > ? > > > > > > > > > > To at least run the statement in non-dcheck scenarios? Otherwise any > > > statement > > > > > that comes after the death test and depends on its state would fail? > > > > > > > > > > Also, if we do this then we can put only the specific statement that is > > > > expected > > > > > to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and improve > > > > > readability + making sure we test what we expect? > > > > > > > > Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH runs in a new > > > > process. That's why we sometimes have multiple statements inside the > macro. > > > > > > > > Consider this: > > > > > > > > 1. lock.Acquire(); > > > > 2. EXPECT_DCHECK_DEATH({ > > > > 3. predecessor.Acquire(); > > > > 4. }, ""); > > > > > > > > On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs. > > > > On Posix, lines 1 and 3 run on different threads -> no DCHECK. > > > > > > > > We must write the test like this to ensure that |predecessor.Acquire()| > > > DCHECKs > > > > on every platform: > > > > > > > > EXPECT_DCHECK_DEATH({ > > > > lock.Acquire(); > > > > predecessor.Acquire(); > > > > }, ""); > > > > > > The way we've structured the tests, we expect death as the last statement. > > Given > > > that the last statement will fail, there shouldn't be statements after that > > > point because now you're in a strange state. > > > > > > Other tests simply don't run these tests when DCHECK_IS_ON(), and we've > > followed > > > the spirit of that with EXPECT_DCHECK_DEATH. > > > > > > +1 to fdoray's comment. The first CL's actually had the statement that > caused > > > problems alone in EXPECT_DEATH, but that caused problems with POSIX since > > POSIX > > > runs a fork from that point. Windows re-executes the entire test. > > > > Ok, re-lgtm. > > > > PS: Would be nice to confirm that try bots indeed cover DCHECKs. Maybe > > intentionally break something in this CL in a patch set to verify the tests > > react as intended? > > From the tests so far, the mobile ones at least do :-). The android ones were > very flaky, so I had to disable them. > > For the other bots, we can see dcheck_always_on=1 (win_chromium_x64_rel_ng), so > we should have good confidence that the DCHECK tests are being run. > > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... I seem to recall a surprising episode on chromium-dev where people thought DCHECKs were on but they weren't -- not sure if it was fixed. Feels to me it's worth an extra hours to do a try with an unintentional bug in this CL just to confirm for realz :-).
https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:25: #define EXPECT_DCHECK_DEATH(statement, regex) On 2016/03/14 20:02:52, gab wrote: > On 2016/03/14 19:55:41, robliao wrote: > > On 2016/03/14 19:51:13, gab wrote: > > > On 2016/03/14 19:09:52, robliao wrote: > > > > On 2016/03/14 18:55:52, fdoray wrote: > > > > > On 2016/03/14 18:26:04, gab wrote: > > > > > > Shouldn't this be > > > > > > #define EXPECT_DCHECK_DEATH(statement, regex) statement > > > > > > ? > > > > > > > > > > > > To at least run the statement in non-dcheck scenarios? Otherwise any > > > > statement > > > > > > that comes after the death test and depends on its state would fail? > > > > > > > > > > > > Also, if we do this then we can put only the specific statement that > is > > > > > expected > > > > > > to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and > improve > > > > > > readability + making sure we test what we expect? > > > > > > > > > > Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH runs in a > new > > > > > process. That's why we sometimes have multiple statements inside the > > macro. > > > > > > > > > > Consider this: > > > > > > > > > > 1. lock.Acquire(); > > > > > 2. EXPECT_DCHECK_DEATH({ > > > > > 3. predecessor.Acquire(); > > > > > 4. }, ""); > > > > > > > > > > On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs. > > > > > On Posix, lines 1 and 3 run on different threads -> no DCHECK. > > > > > > > > > > We must write the test like this to ensure that |predecessor.Acquire()| > > > > DCHECKs > > > > > on every platform: > > > > > > > > > > EXPECT_DCHECK_DEATH({ > > > > > lock.Acquire(); > > > > > predecessor.Acquire(); > > > > > }, ""); > > > > > > > > The way we've structured the tests, we expect death as the last statement. > > > Given > > > > that the last statement will fail, there shouldn't be statements after > that > > > > point because now you're in a strange state. > > > > > > > > Other tests simply don't run these tests when DCHECK_IS_ON(), and we've > > > followed > > > > the spirit of that with EXPECT_DCHECK_DEATH. > > > > > > > > +1 to fdoray's comment. The first CL's actually had the statement that > > caused > > > > problems alone in EXPECT_DEATH, but that caused problems with POSIX since > > > POSIX > > > > runs a fork from that point. Windows re-executes the entire test. > > > > > > Ok, re-lgtm. > > > > > > PS: Would be nice to confirm that try bots indeed cover DCHECKs. Maybe > > > intentionally break something in this CL in a patch set to verify the tests > > > react as intended? > > > > From the tests so far, the mobile ones at least do :-). The android ones were > > very flaky, so I had to disable them. > > > > For the other bots, we can see dcheck_always_on=1 (win_chromium_x64_rel_ng), > so > > we should have good confidence that the DCHECK tests are being run. > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... > > I seem to recall a surprising episode on chromium-dev where people thought > DCHECKs were on but they weren't -- not sure if it was fixed. Feels to me it's > worth an extra hours to do a try with an unintentional bug in this CL just to > confirm for realz :-). Sounds good. Feel free to monitor https://codereview.chromium.org/1802003002/ For the fireworks :-).
https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... File base/task_scheduler/scheduler_lock_unittest.cc (right): https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... base/task_scheduler/scheduler_lock_unittest.cc:25: #define EXPECT_DCHECK_DEATH(statement, regex) On 2016/03/14 20:25:07, robliao wrote: > On 2016/03/14 20:02:52, gab wrote: > > On 2016/03/14 19:55:41, robliao wrote: > > > On 2016/03/14 19:51:13, gab wrote: > > > > On 2016/03/14 19:09:52, robliao wrote: > > > > > On 2016/03/14 18:55:52, fdoray wrote: > > > > > > On 2016/03/14 18:26:04, gab wrote: > > > > > > > Shouldn't this be > > > > > > > #define EXPECT_DCHECK_DEATH(statement, regex) statement > > > > > > > ? > > > > > > > > > > > > > > To at least run the statement in non-dcheck scenarios? Otherwise any > > > > > statement > > > > > > > that comes after the death test and depends on its state would fail? > > > > > > > > > > > > > > Also, if we do this then we can put only the specific statement that > > is > > > > > > expected > > > > > > > to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and > > improve > > > > > > > readability + making sure we test what we expect? > > > > > > > > > > > > Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH runs in a > > new > > > > > > process. That's why we sometimes have multiple statements inside the > > > macro. > > > > > > > > > > > > Consider this: > > > > > > > > > > > > 1. lock.Acquire(); > > > > > > 2. EXPECT_DCHECK_DEATH({ > > > > > > 3. predecessor.Acquire(); > > > > > > 4. }, ""); > > > > > > > > > > > > On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs. > > > > > > On Posix, lines 1 and 3 run on different threads -> no DCHECK. > > > > > > > > > > > > We must write the test like this to ensure that > |predecessor.Acquire()| > > > > > DCHECKs > > > > > > on every platform: > > > > > > > > > > > > EXPECT_DCHECK_DEATH({ > > > > > > lock.Acquire(); > > > > > > predecessor.Acquire(); > > > > > > }, ""); > > > > > > > > > > The way we've structured the tests, we expect death as the last > statement. > > > > Given > > > > > that the last statement will fail, there shouldn't be statements after > > that > > > > > point because now you're in a strange state. > > > > > > > > > > Other tests simply don't run these tests when DCHECK_IS_ON(), and we've > > > > followed > > > > > the spirit of that with EXPECT_DCHECK_DEATH. > > > > > > > > > > +1 to fdoray's comment. The first CL's actually had the statement that > > > caused > > > > > problems alone in EXPECT_DEATH, but that caused problems with POSIX > since > > > > POSIX > > > > > runs a fork from that point. Windows re-executes the entire test. > > > > > > > > Ok, re-lgtm. > > > > > > > > PS: Would be nice to confirm that try bots indeed cover DCHECKs. Maybe > > > > intentionally break something in this CL in a patch set to verify the > tests > > > > react as intended? > > > > > > From the tests so far, the mobile ones at least do :-). The android ones > were > > > very flaky, so I had to disable them. > > > > > > For the other bots, we can see dcheck_always_on=1 (win_chromium_x64_rel_ng), > > so > > > we should have good confidence that the DCHECK tests are being run. > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... > > > > I seem to recall a surprising episode on chromium-dev where people thought > > DCHECKs were on but they weren't -- not sure if it was fixed. Feels to me it's > > worth an extra hours to do a try with an unintentional bug in this CL just to > > confirm for realz :-). > > Sounds good. Feel free to monitor > https://codereview.chromium.org/1802003002/ > > For the fireworks :-). And the fireworks have started (tests going red), so we should be good to go here.
On 2016/03/14 20:37:13, robliao wrote: > https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... > File base/task_scheduler/scheduler_lock_unittest.cc (right): > > https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... > base/task_scheduler/scheduler_lock_unittest.cc:25: #define > EXPECT_DCHECK_DEATH(statement, regex) > On 2016/03/14 20:25:07, robliao wrote: > > On 2016/03/14 20:02:52, gab wrote: > > > On 2016/03/14 19:55:41, robliao wrote: > > > > On 2016/03/14 19:51:13, gab wrote: > > > > > On 2016/03/14 19:09:52, robliao wrote: > > > > > > On 2016/03/14 18:55:52, fdoray wrote: > > > > > > > On 2016/03/14 18:26:04, gab wrote: > > > > > > > > Shouldn't this be > > > > > > > > #define EXPECT_DCHECK_DEATH(statement, regex) statement > > > > > > > > ? > > > > > > > > > > > > > > > > To at least run the statement in non-dcheck scenarios? Otherwise > any > > > > > > statement > > > > > > > > that comes after the death test and depends on its state would > fail? > > > > > > > > > > > > > > > > Also, if we do this then we can put only the specific statement > that > > > is > > > > > > > expected > > > > > > > > to generate the DCHECK in the EXPECT_DCHECK_DEATH statement and > > > improve > > > > > > > > readability + making sure we test what we expect? > > > > > > > > > > > > > > Note that on Posix, the |statement| of EXPECT_DCHECK_DEATH runs in > a > > > new > > > > > > > process. That's why we sometimes have multiple statements inside the > > > > macro. > > > > > > > > > > > > > > Consider this: > > > > > > > > > > > > > > 1. lock.Acquire(); > > > > > > > 2. EXPECT_DCHECK_DEATH({ > > > > > > > 3. predecessor.Acquire(); > > > > > > > 4. }, ""); > > > > > > > > > > > > > > On Windows, lines 1 and 3 run on the same thread -> line 3 DCHECKs. > > > > > > > On Posix, lines 1 and 3 run on different threads -> no DCHECK. > > > > > > > > > > > > > > We must write the test like this to ensure that > > |predecessor.Acquire()| > > > > > > DCHECKs > > > > > > > on every platform: > > > > > > > > > > > > > > EXPECT_DCHECK_DEATH({ > > > > > > > lock.Acquire(); > > > > > > > predecessor.Acquire(); > > > > > > > }, ""); > > > > > > > > > > > > The way we've structured the tests, we expect death as the last > > statement. > > > > > Given > > > > > > that the last statement will fail, there shouldn't be statements after > > > that > > > > > > point because now you're in a strange state. > > > > > > > > > > > > Other tests simply don't run these tests when DCHECK_IS_ON(), and > we've > > > > > followed > > > > > > the spirit of that with EXPECT_DCHECK_DEATH. > > > > > > > > > > > > +1 to fdoray's comment. The first CL's actually had the statement that > > > > caused > > > > > > problems alone in EXPECT_DEATH, but that caused problems with POSIX > > since > > > > > POSIX > > > > > > runs a fork from that point. Windows re-executes the entire test. > > > > > > > > > > Ok, re-lgtm. > > > > > > > > > > PS: Would be nice to confirm that try bots indeed cover DCHECKs. Maybe > > > > > intentionally break something in this CL in a patch set to verify the > > tests > > > > > react as intended? > > > > > > > > From the tests so far, the mobile ones at least do :-). The android ones > > were > > > > very flaky, so I had to disable them. > > > > > > > > For the other bots, we can see dcheck_always_on=1 > (win_chromium_x64_rel_ng), > > > so > > > > we should have good confidence that the DCHECK tests are being run. > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64... > > > > > > I seem to recall a surprising episode on chromium-dev where people thought > > > DCHECKs were on but they weren't -- not sure if it was fixed. Feels to me > it's > > > worth an extra hours to do a try with an unintentional bug in this CL just > to > > > confirm for realz :-). > > > > Sounds good. Feel free to monitor > > https://codereview.chromium.org/1802003002/ > > > > For the fireworks :-). > > And the fireworks have started (tests going red), so we should be good to go > here. Awesome, thanks! Eager to see whether Windows will fire, but at least Linux/Mac/ChromeOS/ChromeCast already did, so go ahead and CQ this one as you wish :-).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/1706123002/#ps420001 (title: "Move most DCHECKs to DCHECK_(EQ|NE)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706123002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706123002/420001
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [2/9] Scheduler Lock The task scheduler lock helps ensure correctness of locks by verifying that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock is a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [2/9] Scheduler Lock The task scheduler lock helps ensure correctness of locks by verifying that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock is a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [2/9] Scheduler Lock The task scheduler lock helps ensure correctness of locks by verifying that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock is a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [2/9] Scheduler Lock The task scheduler lock helps ensure correctness of locks by verifying that we cannot create lock cycles and checking to make sure that the locks acquired match the locks expected to be acquired in DCHECK_IS_ON() builds. The implementation will DCHECK if an unexpected lock is acquired. When DCHECK_IS_ON() is not set, SchedulerLock is a regular lock. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 Committed: https://crrev.com/b6f61ce5c35e985cf07279ea25fd846b0b9d6f9d Cr-Commit-Position: refs/heads/master@{#381075} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/b6f61ce5c35e985cf07279ea25fd846b0b9d6f9d Cr-Commit-Position: refs/heads/master@{#381075} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
