Add an API to allow WeakPtr validity (and Callback::IsCancelled()) to be tested off its "bound" sequence |
||||||||
Issue descriptionBeing able to do an optimistic check for the WeakPtr's validity off-thread (without using it) is required to be able to prune the TaskScheduler's queues from cancelled delayed tasks. Discussion @ https://groups.google.com/a/chromium.org/d/topic/scheduler-dev/JBKYWCaODbI/discussion WIP @ https://chromium-review.googlesource.com/527413 Doc : https://docs.google.com/document/d/1IGzq9Nx69GS_2iynGmPWo5sFAD2DcCyBY1zIvZwF7k8/edit#
,
Apr 23 2018
Olivier will take a stab at this this week :)
,
Apr 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c3c5c50d6167ec45c36bc01f2e49275c94e99d88 commit c3c5c50d6167ec45c36bc01f2e49275c94e99d88 Author: Olivier Li <olivierli@chromium.org> Date: Fri Apr 27 20:17:47 2018 Add sequence verification to WeakPtrBase::reset() Bug: 730693 Change-Id: Ia754b382d9c4626b24ecab368d25b3c6331230be Reviewed-on: https://chromium-review.googlesource.com/1024434 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#554479} [modify] https://crrev.com/c3c5c50d6167ec45c36bc01f2e49275c94e99d88/base/memory/weak_ptr.cc [modify] https://crrev.com/c3c5c50d6167ec45c36bc01f2e49275c94e99d88/base/memory/weak_ptr.h [modify] https://crrev.com/c3c5c50d6167ec45c36bc01f2e49275c94e99d88/base/memory/weak_ptr_unittest.cc
,
Apr 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/35d520e5adbfbbb4a1a657a97b03a53e4e8f75b3 commit 35d520e5adbfbbb4a1a657a97b03a53e4e8f75b3 Author: Olivier Li <olivierli@chromium.org> Date: Mon Apr 30 23:15:27 2018 Thread-safe validity check for WeakPtr Change-Id: Idb7667c42bbe16ab9eb7bb4421980401641b8c94 Bug: 730693 Change-Id: Idb7667c42bbe16ab9eb7bb4421980401641b8c94 Reviewed-on: https://chromium-review.googlesource.com/1032635 Commit-Queue: Oliver Li <olivierli@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#554912} [modify] https://crrev.com/35d520e5adbfbbb4a1a657a97b03a53e4e8f75b3/base/memory/weak_ptr.cc [modify] https://crrev.com/35d520e5adbfbbb4a1a657a97b03a53e4e8f75b3/base/memory/weak_ptr.h [modify] https://crrev.com/35d520e5adbfbbb4a1a657a97b03a53e4e8f75b3/base/memory/weak_ptr_unittest.cc [modify] https://crrev.com/35d520e5adbfbbb4a1a657a97b03a53e4e8f75b3/base/synchronization/atomic_flag.cc [modify] https://crrev.com/35d520e5adbfbbb4a1a657a97b03a53e4e8f75b3/base/synchronization/atomic_flag.h
,
May 1 2018
There are a couple of problems with these changes, so I've left notes on the WIP and landed CLs, and will proceed with reverts. Two issues are: 1. WeakPtrs are like any other pointer type with regard to being written-to and read-from concurrently. There is no need for reset() to be called from the thread on which the WeakPtr's Flag is bound, nor to protect against concurrent reset() vs check/dereference of an individual WeakPtr. 2. Since we only have a single use-case for this "optimistic" WeakPtr validity check, we should not reduce the checks in place for all other call-sites - rather than changing the semantics of operator bool() we should add an explicit UnsafeIsValid() API, or similar, if that is what is required. gab: Can you provide a link somewhere to the TaskScheduler code that requires this in the first place?
,
May 1 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fb4a528bfd1bec78646f16b5be33a4bc89a16ef9 commit fb4a528bfd1bec78646f16b5be33a4bc89a16ef9 Author: Wez <wez@chromium.org> Date: Tue May 01 09:02:11 2018 Revert "Thread-safe validity check for WeakPtr" This reverts commit 35d520e5adbfbbb4a1a657a97b03a53e4e8f75b3. Reason for revert: As noted on the bug & in CL comments, this loosens the constraints on operator bool() for all callers, where we should instead add an explicit unsafe-I-know-what-I'm-doing API. Original change's description: > Thread-safe validity check for WeakPtr > > Change-Id: Idb7667c42bbe16ab9eb7bb4421980401641b8c94 > > Bug: 730693 > Change-Id: Idb7667c42bbe16ab9eb7bb4421980401641b8c94 > Reviewed-on: https://chromium-review.googlesource.com/1032635 > Commit-Queue: Oliver Li <olivierli@chromium.org> > Reviewed-by: Gabriel Charette <gab@chromium.org> > Cr-Commit-Position: refs/heads/master@{#554912} TBR=gab@chromium.org,olivierli@chromium.org Change-Id: I57c9ac10a23938bff7a84ef53539a7deb740650b No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 730693 Reviewed-on: https://chromium-review.googlesource.com/1037004 Reviewed-by: Wez <wez@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#555019} [modify] https://crrev.com/fb4a528bfd1bec78646f16b5be33a4bc89a16ef9/base/memory/weak_ptr.cc [modify] https://crrev.com/fb4a528bfd1bec78646f16b5be33a4bc89a16ef9/base/memory/weak_ptr.h [modify] https://crrev.com/fb4a528bfd1bec78646f16b5be33a4bc89a16ef9/base/memory/weak_ptr_unittest.cc [modify] https://crrev.com/fb4a528bfd1bec78646f16b5be33a4bc89a16ef9/base/synchronization/atomic_flag.cc [modify] https://crrev.com/fb4a528bfd1bec78646f16b5be33a4bc89a16ef9/base/synchronization/atomic_flag.h
,
May 1 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/caf863b9947d54ea503247c308c95b8e0794cff3 commit caf863b9947d54ea503247c308c95b8e0794cff3 Author: Wez <wez@chromium.org> Date: Tue May 01 11:03:29 2018 Revert "Add sequence verification to WeakPtrBase::reset()" This reverts commit c3c5c50d6167ec45c36bc01f2e49275c94e99d88. Reason for revert: CL adds a constraint to WeakPtrBase::reset() that it be called on the same thread to which the Flag is bound, which is not an actual requirement for correct use of WeakPtrs. Original change's description: > Add sequence verification to WeakPtrBase::reset() > > Bug: 730693 > Change-Id: Ia754b382d9c4626b24ecab368d25b3c6331230be > Reviewed-on: https://chromium-review.googlesource.com/1024434 > Commit-Queue: Gabriel Charette <gab@chromium.org> > Reviewed-by: Gabriel Charette <gab@chromium.org> > Cr-Commit-Position: refs/heads/master@{#554479} TBR=gab@chromium.org,olivierli@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 730693 Change-Id: I833711a36feeca567113330985619930f313967f Reviewed-on: https://chromium-review.googlesource.com/1037083 Reviewed-by: Wez <wez@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#555024} [modify] https://crrev.com/caf863b9947d54ea503247c308c95b8e0794cff3/base/memory/weak_ptr.cc [modify] https://crrev.com/caf863b9947d54ea503247c308c95b8e0794cff3/base/memory/weak_ptr.h [modify] https://crrev.com/caf863b9947d54ea503247c308c95b8e0794cff3/base/memory/weak_ptr_unittest.cc
,
May 1 2018
,
May 1 2018
Olivier's rotation with us is done and the scope of this has grown out-of-scope for a one week rotation. Doc summarizing latest updates : https://docs.google.com/document/d/1IGzq9Nx69GS_2iynGmPWo5sFAD2DcCyBY1zIvZwF7k8/edit#
,
May 1 2018
,
Jul 17
Nicolas will take a stab this week on a rotation through our team :)
,
Jul 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ff1eab6792bc012492f1ebcb1f042c520fe2d838 commit ff1eab6792bc012492f1ebcb1f042c520fe2d838 Author: Nicolas Ouellet-payeur <nicolaso@chromium.org> Date: Thu Jul 19 18:41:50 2018 Add MaybeValid() to WeakPtr This re-lands commit 35d520e5adbfbbb4a1a657a97b03a53e4e8f75b3 Instead of changing the behavior of operator bool(), we instead define a new method, WeakPtr::MaybeValid(). Outlined in the design doc: https://docs.google.com/document/d/1IGzq9Nx69GS_2iynGmPWo5sFAD2DcCyBY1zIvZwF7k8/edit Bug: 730693 Change-Id: I9fcc8804e11a8adb82111e21814b375b59231687 Reviewed-on: https://chromium-review.googlesource.com/1141085 Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Wez <wez@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#576572} [modify] https://crrev.com/ff1eab6792bc012492f1ebcb1f042c520fe2d838/base/memory/weak_ptr.cc [modify] https://crrev.com/ff1eab6792bc012492f1ebcb1f042c520fe2d838/base/memory/weak_ptr.h [modify] https://crrev.com/ff1eab6792bc012492f1ebcb1f042c520fe2d838/base/memory/weak_ptr_unittest.cc [modify] https://crrev.com/ff1eab6792bc012492f1ebcb1f042c520fe2d838/base/synchronization/atomic_flag.cc [modify] https://crrev.com/ff1eab6792bc012492f1ebcb1f042c520fe2d838/base/synchronization/atomic_flag.h
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40f8e9a571636737228190af5a062869dbf028d4 commit 40f8e9a571636737228190af5a062869dbf028d4 Author: Nicolas Ouellet-payeur <nicolaso@chromium.org> Date: Mon Jul 30 16:26:48 2018 Add MaybeValid() to base::Callback This is a thread-safe validity check for WeakPtr-based Callbacks using the recently added WeakPtr::MaybeValid(). Bug: 730693 Change-Id: I174efb30bb16d2776e33ec64d48a913943e770a0 Reviewed-on: https://chromium-review.googlesource.com/1144208 Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#579061} [modify] https://crrev.com/40f8e9a571636737228190af5a062869dbf028d4/base/bind_internal.h [modify] https://crrev.com/40f8e9a571636737228190af5a062869dbf028d4/base/callback_internal.cc [modify] https://crrev.com/40f8e9a571636737228190af5a062869dbf028d4/base/callback_internal.h [modify] https://crrev.com/40f8e9a571636737228190af5a062869dbf028d4/base/callback_unittest.cc [modify] https://crrev.com/40f8e9a571636737228190af5a062869dbf028d4/third_party/blink/renderer/platform/heap/persistent.h [modify] https://crrev.com/40f8e9a571636737228190af5a062869dbf028d4/third_party/blink/renderer/platform/web_task_runner.cc [modify] https://crrev.com/40f8e9a571636737228190af5a062869dbf028d4/third_party/blink/renderer/platform/wtf/functional.h
,
Jul 30
Sweet :), using this in TaskScheduler is tracked in issue 653394 so we're done here! Thanks Nicolas |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by gab@chromium.org
, Apr 12 2018