Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206263002:
Add Lock::HandlesMultipleThreadPriorities(). (Closed)
DescriptionAdd Lock::HandlesMultipleThreadPriorities().
This static method is available on all platform. It replaces
Lock::PriorityInheritanceAvailable(). It will simplify code
that checks whether priority inversion will be mitigated when
Lock is used from different thread priorities.
E.g.:
if (Lock::Lock::PriorityInheritanceAvailable())
PlatformThread::SetCurrentThreadPriority(...);
Becomes:
if (Lock::HandlesMultipleThreadPriorities())
PlatformThread::SetCurrentThreadPriority(...);
BUG=611856
Committed: https://crrev.com/d84dc069a147eb0209b9f7713beafb3de0dec26b
Cr-Commit-Position: refs/heads/master@{#409890}
Patch Set 1 #
Total comments: 4
Patch Set 2 : rename to HandlesMultipleThreadPriorities, #elif defined(OS_WIN) #Patch Set 3 : self-review #
Total comments: 7
Patch Set 4 : CR gab/robliao #10-11 #
Total comments: 6
Patch Set 5 : CR robliao #14 (improve comment) #Patch Set 6 : CR gab #18 #Messages
Total messages: 28 (9 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org, thestig@chromium.org
PTAL
https://codereview.chromium.org/2206263002/diff/1/base/synchronization/lock.h File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/1/base/synchronization/lock.h... base/synchronization/lock.h:66: static bool IsSafeWithMultipleThreadPriorities() { I would go with HandlesMultipleThreadPriorities. Safe is too strong of a guarantee.
https://codereview.chromium.org/2206263002/diff/1/base/synchronization/lock.h File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/1/base/synchronization/lock.h... base/synchronization/lock.h:70: return true; Add a comment about why that's true on Windows (summarize and refer to https://msdn.microsoft.com/library/windows/desktop/ms684831.aspx) and also #elif (OS_WIN) above and #else #error "Unsupported platform" #endif
Description was changed from
==========
Add Lock::IsSafeWithMultipleThreadPriorities().
This static method is available on all platform. It replaces
Lock::PriorityInheritanceAvailable(). It will make code that checks
if it is safe to use a lock with different thread priorities simpler
and clearer.
E.g.:
if (Lock::Lock::PriorityInheritanceAvailable())
PlatformThread::SetCurrentThreadPriority(...);
Becomes:
if (Lock::IsSafeWithMultipleThreadPriorities())
PlatformThread::SetCurrentThreadPriority(...);
BUG=611856
==========
to
==========
Add Lock::IsSafeWithMultipleThreadPriorities().
This static method is available on all platform. It replaces
Lock::PriorityInheritanceAvailable(). It will simplify code
that checks whether priority inversion will be mitigated when
Lock is used from different thread priorities.
E.g.:
if (Lock::Lock::PriorityInheritanceAvailable())
PlatformThread::SetCurrentThreadPriority(...);
Becomes:
if (Lock::HandlesMultipleThreadPriorities())
PlatformThread::SetCurrentThreadPriority(...);
BUG=611856
==========
Description was changed from
==========
Add Lock::IsSafeWithMultipleThreadPriorities().
This static method is available on all platform. It replaces
Lock::PriorityInheritanceAvailable(). It will simplify code
that checks whether priority inversion will be mitigated when
Lock is used from different thread priorities.
E.g.:
if (Lock::Lock::PriorityInheritanceAvailable())
PlatformThread::SetCurrentThreadPriority(...);
Becomes:
if (Lock::HandlesMultipleThreadPriorities())
PlatformThread::SetCurrentThreadPriority(...);
BUG=611856
==========
to
==========
Add Lock::HandlesMultipleThreadPriorities().
This static method is available on all platform. It replaces
Lock::PriorityInheritanceAvailable(). It will simplify code
that checks whether priority inversion will be mitigated when
Lock is used from different thread priorities.
E.g.:
if (Lock::Lock::PriorityInheritanceAvailable())
PlatformThread::SetCurrentThreadPriority(...);
Becomes:
if (Lock::HandlesMultipleThreadPriorities())
PlatformThread::SetCurrentThreadPriority(...);
BUG=611856
==========
PTAnL https://codereview.chromium.org/2206263002/diff/1/base/synchronization/lock.h File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/1/base/synchronization/lock.h... base/synchronization/lock.h:66: static bool IsSafeWithMultipleThreadPriorities() { On 2016/08/03 18:02:10, robliao wrote: > I would go with HandlesMultipleThreadPriorities. Safe is too strong of a > guarantee. Done. https://codereview.chromium.org/2206263002/diff/1/base/synchronization/lock.h... base/synchronization/lock.h:70: return true; On 2016/08/03 18:04:57, gab wrote: > Add a comment about why that's true on Windows (summarize and refer to > https://msdn.microsoft.com/library/windows/desktop/ms684831.aspx) > > and also > #elif (OS_WIN) above and > #else > #error "Unsupported platform" > #endif Done.
https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... base/synchronization/lock.h:68: // unbounded amount of time by holding a Lock. Hmmm? Unbounded? The whole point here I thought was that when we return true, low priority threads *will* be readied in a reasonable amount of time? If you meant a low priority thread holding the lock while it's active, then I don't think this comment is necessary as that's true for threads of any priority -- the property at stake here handles readying threads holding the blocking resource, everything else is business as usual with Locks. https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... base/synchronization/lock.h:78: #error "Unsupported platform" nit: no indent for preprocessor commands
https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... base/synchronization/lock.h:68: // unbounded amount of time by holding a Lock. On 2016/08/03 18:47:25, gab wrote: > Hmmm? Unbounded? The whole point here I thought was that when we return true, > low priority threads *will* be readied in a reasonable amount of time? > > If you meant a low priority thread holding the lock while it's active, then I > don't think this comment is necessary as that's true for threads of any priority > -- the property at stake here handles readying threads holding the blocking > resource, everything else is business as usual with Locks. It will be readied, but if it's doing a really long I/O op, the higher priority thread is still blocked. Granted this is true regardless of lock thread priority support. https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... base/synchronization/lock.h:78: #error "Unsupported platform" Maybe "Unsupported platform. Must specify if the lock handles multiple thread priorities."
PTAnL https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... base/synchronization/lock.h:68: // unbounded amount of time by holding a Lock. On 2016/08/03 19:17:15, robliao wrote: > On 2016/08/03 18:47:25, gab wrote: > > Hmmm? Unbounded? The whole point here I thought was that when we return true, > > low priority threads *will* be readied in a reasonable amount of time? > > > > If you meant a low priority thread holding the lock while it's active, then I > > don't think this comment is necessary as that's true for threads of any > priority > > -- the property at stake here handles readying threads holding the blocking > > resource, everything else is business as usual with Locks. > > It will be readied, but if it's doing a really long I/O op, the higher priority > thread is still blocked. > > Granted this is true regardless of lock thread priority support. Done. Removed last sentence because it is true regardless of whether they are used from multiple priorities. https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... base/synchronization/lock.h:78: #error "Unsupported platform" On 2016/08/03 19:17:15, robliao wrote: > Maybe "Unsupported platform. Must specify if the lock handles multiple thread > priorities." Done. https://codereview.chromium.org/2206263002/diff/40001/base/synchronization/lo... base/synchronization/lock.h:78: #error "Unsupported platform" On 2016/08/03 18:47:25, gab wrote: > nit: no indent for preprocessor commands Done.
lgtm + comment. Thanks for the improvement! https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... base/synchronization/lock.h:64: // Whether Lock mitigates priority inversion when used from different thread One more nit: Since we're generalizing this, I would rephrase the comment as follows. Whether Lock makes special accommodation under contention when used with threads of different priorities, including but not limited to priority inheritance of the lock holder from the lock waiter of a higher priority.
thestig@: PTAL https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... base/synchronization/lock.h:64: // Whether Lock mitigates priority inversion when used from different thread On 2016/08/03 19:30:07, robliao wrote: > One more nit: Since we're generalizing this, I would rephrase the comment as > follows. > > Whether Lock makes special accommodation under contention when used with threads > of different priorities, including but not limited to priority inheritance of > the lock holder from the lock waiter of a higher priority. Done.
lgtm if everyone else is happy
thestig@: Thanks. gab@: PTAL
https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... base/synchronization/lock.h:66: // a Lock to run when higher priority threads try to acquire it. I think the second sentence is specific to the implementation (i.e. this sounds like something that should go in the OS_POSIX block and the OS_WIN block already has its comment) https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... base/synchronization/lock.h:76: #error Must specify if the lock handles multiple thread priorities. #error Unsupported platform is sufficient IMO
gab@: PTAL https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... File base/synchronization/lock.h (right): https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... base/synchronization/lock.h:66: // a Lock to run when higher priority threads try to acquire it. On 2016/08/03 19:36:25, gab wrote: > I think the second sentence is specific to the implementation (i.e. this sounds > like something that should go in the OS_POSIX block and the OS_WIN block already > has its comment) Done. https://codereview.chromium.org/2206263002/diff/60001/base/synchronization/lo... base/synchronization/lock.h:76: #error Must specify if the lock handles multiple thread priorities. On 2016/08/03 19:36:25, gab wrote: > #error Unsupported platform > > is sufficient IMO Done.
ping gab
lgtm
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2206263002/#ps100001 (title: "CR gab #18")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Add Lock::HandlesMultipleThreadPriorities().
This static method is available on all platform. It replaces
Lock::PriorityInheritanceAvailable(). It will simplify code
that checks whether priority inversion will be mitigated when
Lock is used from different thread priorities.
E.g.:
if (Lock::Lock::PriorityInheritanceAvailable())
PlatformThread::SetCurrentThreadPriority(...);
Becomes:
if (Lock::HandlesMultipleThreadPriorities())
PlatformThread::SetCurrentThreadPriority(...);
BUG=611856
==========
to
==========
Add Lock::HandlesMultipleThreadPriorities().
This static method is available on all platform. It replaces
Lock::PriorityInheritanceAvailable(). It will simplify code
that checks whether priority inversion will be mitigated when
Lock is used from different thread priorities.
E.g.:
if (Lock::Lock::PriorityInheritanceAvailable())
PlatformThread::SetCurrentThreadPriority(...);
Becomes:
if (Lock::HandlesMultipleThreadPriorities())
PlatformThread::SetCurrentThreadPriority(...);
BUG=611856
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
Add Lock::HandlesMultipleThreadPriorities().
This static method is available on all platform. It replaces
Lock::PriorityInheritanceAvailable(). It will simplify code
that checks whether priority inversion will be mitigated when
Lock is used from different thread priorities.
E.g.:
if (Lock::Lock::PriorityInheritanceAvailable())
PlatformThread::SetCurrentThreadPriority(...);
Becomes:
if (Lock::HandlesMultipleThreadPriorities())
PlatformThread::SetCurrentThreadPriority(...);
BUG=611856
==========
to
==========
Add Lock::HandlesMultipleThreadPriorities().
This static method is available on all platform. It replaces
Lock::PriorityInheritanceAvailable(). It will simplify code
that checks whether priority inversion will be mitigated when
Lock is used from different thread priorities.
E.g.:
if (Lock::Lock::PriorityInheritanceAvailable())
PlatformThread::SetCurrentThreadPriority(...);
Becomes:
if (Lock::HandlesMultipleThreadPriorities())
PlatformThread::SetCurrentThreadPriority(...);
BUG=611856
Committed: https://crrev.com/d84dc069a147eb0209b9f7713beafb3de0dec26b
Cr-Commit-Position: refs/heads/master@{#409890}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d84dc069a147eb0209b9f7713beafb3de0dec26b Cr-Commit-Position: refs/heads/master@{#409890} |
