Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2317253007:
Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc (Closed)
DescriptionRemove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc
SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.
Developers should use SequenceChecker to verify that tasks run
sequentially. Note: SequenceChecker::CalledOnValidSequence
is a no-op in non-DCHECK builds.
BUG=622400
Committed: https://crrev.com/7cffceeef702204d07f906a905019f650c12e0ba
Cr-Commit-Position: refs/heads/master@{#418554}
Patch Set 1 #
Total comments: 8
Patch Set 2 : CR michaeln #9 #
Messages
Total messages: 21 (11 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + michaeln@chromium.org
PTAL
https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_task_runner.cc (left): https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_task_runner.cc:67: return sequenced_worker_pool_->IsRunningSequenceOnCurrentThread( wdyt about changing the impl w/o changing the public facing api of this class? seems like you could accomplish the switchover to sequence checker entirely in this method body? if (id == PRI) return primary_checker.CalledOnValidThread(); return commit_checker.CalledOnValidThread(); https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_task_runner.cc (right): https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_task_runner.cc:34: return sequenced_worker_pool_->RunsTasksOnCurrentThread(); i think you've changed the semantics of this method? it's supposed to be equivalent to IsRunningOnPrimarySequence()
PTAnL https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_task_runner.cc (left): https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_task_runner.cc:67: return sequenced_worker_pool_->IsRunningSequenceOnCurrentThread( On 2016/09/12 22:49:17, michaeln wrote: > wdyt about changing the impl w/o changing the public facing api of this class? > seems like you could accomplish the switchover to sequence checker entirely in > this method body? > > if (id == PRI) > return primary_checker.CalledOnValidThread(); > return commit_checker.CalledOnValidThread(); SequenceChecker::CalledOnValidThread() always returns true in non-DCHECK builds. I prefer having AssertIsRunningOn(Primary|Commit)Sequence and no IsRunningOn(Primary|Commit)Sequence to prevent such use cases: if (dom_storage_task_runner->IsRunningOnPrimarySequence()) { // This will always run in non-DCHECK builds !!! } https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_task_runner.cc (right): https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_task_runner.cc:34: return sequenced_worker_pool_->RunsTasksOnCurrentThread(); On 2016/09/12 22:49:17, michaeln wrote: > i think you've changed the semantics of this method? it's supposed to be > equivalent to IsRunningOnPrimarySequence() This method is part of the TaskRunner interface https://cs.chromium.org/chromium/src/base/task_runner.h?l=82 "Returns true if the current thread is a thread on which a task may be run, and false if no task will be run on the current thread." Derived classes shouldn't have a different description for this method. Can you point me to a call site where having an implementation that complies with the description in task_runner.h doesn't work?
lgtm, but please see comments about being surprised at the loss of ability to rely on 'sequence checking' for more than just dchecking https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_task_runner.cc (left): https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_task_runner.cc:67: return sequenced_worker_pool_->IsRunningSequenceOnCurrentThread( On 2016/09/13 14:24:13, fdoray wrote: > On 2016/09/12 22:49:17, michaeln wrote: > > wdyt about changing the impl w/o changing the public facing api of this class? > > seems like you could accomplish the switchover to sequence checker entirely in > > this method body? > > > > if (id == PRI) > > return primary_checker.CalledOnValidThread(); > > return commit_checker.CalledOnValidThread(); > > SequenceChecker::CalledOnValidThread() always returns true in non-DCHECK builds. > I prefer having AssertIsRunningOn(Primary|Commit)Sequence and no > IsRunningOn(Primary|Commit)Sequence to prevent such use cases: > > if (dom_storage_task_runner->IsRunningOnPrimarySequence()) { > // This will always run in non-DCHECK builds !!! > } Surprising? Ok, I agree given the lack of the actual functionality, we need to take away the methods that promise to provide that functionality. I see the Checker class is a debug build only kind of utility. > SequenceChecker works everywhere in Chrome Please remove this comment from the CL description, maybe mention that SequenceChecker only works properly in builds with DCHECKs enabled. https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_task_runner.cc (right): https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_task_runner.cc:34: return sequenced_worker_pool_->RunsTasksOnCurrentThread(); On 2016/09/13 14:24:13, fdoray wrote: > On 2016/09/12 22:49:17, michaeln wrote: > > i think you've changed the semantics of this method? it's supposed to be > > equivalent to IsRunningOnPrimarySequence() > > This method is part of the TaskRunner interface > https://cs.chromium.org/chromium/src/base/task_runner.h?l=82 "Returns true if > the current thread is a thread on which a task may be run, and false if no task > will be run on the current thread." > > Derived classes shouldn't have a different description for this method. Can you > point me to a call site where having an implementation that complies with the > description in task_runner.h doesn't work? The base class is too abstract to provide a full description of its various concrete impls. hmmm... if we can't provide a useful impl for this concrete class, maybe just return true; Given that infrastructure no longer provides the ability to check if we're actually running on the desired sequence in a release build, i suppose we have no choice but to alter the semantics of this method of this class. Since the comment in the base class says its valid to always return true, i'd vote to do that so its more clear its not actually a useful method of this implementation of taskrunner. Fortunately, I don't see any dom_storage related callsites where it matters. You asked for a ptr to a call site where a more discerning impl would matter, here's one outside of the dom_storage use case... base_session_service.cc line 37 If the method always returns true (a fully description compliant impl), that callsite will never branch. If the runner is backed by a logical sequence and the method returns true when on a different sequence (which i think is the best we can do know based on what you said), this code would do the wrong thing.
Description was changed from ========== Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc SequencedWorkerPool is being migrated to base/task_scheduler and there is no equivalent to IsRunningSequenceOnCurrentThread() in base/task_scheduler. Developers should use SequenceChecker to verify that tasks run sequentially. Unlike SequencedWorkerPool::IsRunningSequenceOnCurrentThread(), SequenceChecker works everywhere in Chrome (MessageLoop, SequencedWorkerPool, base/task_scheduler...). BUG=622400 ========== to ========== Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc SequencedWorkerPool is being migrated to base/task_scheduler and there is no equivalent to IsRunningSequenceOnCurrentThread() in base/task_scheduler. Developers should use SequenceChecker to verify that tasks run sequentially. SequenceChecker is not specific to SequencedWorkerPool; it works in MessageLoops and TaskScheduler in addition to SequencedWorkerPool. Note: SequenceChecker::CalledOnValidSequence is a no-op in non-DCHECK builds. BUG=622400 ==========
Description was changed from ========== Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc SequencedWorkerPool is being migrated to base/task_scheduler and there is no equivalent to IsRunningSequenceOnCurrentThread() in base/task_scheduler. Developers should use SequenceChecker to verify that tasks run sequentially. SequenceChecker is not specific to SequencedWorkerPool; it works in MessageLoops and TaskScheduler in addition to SequencedWorkerPool. Note: SequenceChecker::CalledOnValidSequence is a no-op in non-DCHECK builds. BUG=622400 ========== to ========== Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc SequencedWorkerPool is being migrated to base/task_scheduler and there is no equivalent to IsRunningSequenceOnCurrentThread() in base/task_scheduler. Developers should use SequenceChecker to verify that tasks run sequentially. Note: SequenceChecker::CalledOnValidSequence is a no-op in non-DCHECK builds. BUG=622400 ==========
https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_task_runner.cc (left): https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_task_runner.cc:67: return sequenced_worker_pool_->IsRunningSequenceOnCurrentThread( On 2016/09/14 00:35:12, michaeln wrote: > On 2016/09/13 14:24:13, fdoray wrote: > > On 2016/09/12 22:49:17, michaeln wrote: > > > wdyt about changing the impl w/o changing the public facing api of this > class? > > > seems like you could accomplish the switchover to sequence checker entirely > in > > > this method body? > > > > > > if (id == PRI) > > > return primary_checker.CalledOnValidThread(); > > > return commit_checker.CalledOnValidThread(); > > > > SequenceChecker::CalledOnValidThread() always returns true in non-DCHECK > builds. > > I prefer having AssertIsRunningOn(Primary|Commit)Sequence and no > > IsRunningOn(Primary|Commit)Sequence to prevent such use cases: > > > > if (dom_storage_task_runner->IsRunningOnPrimarySequence()) { > > // This will always run in non-DCHECK builds !!! > > } > > Surprising? Ok, I agree given the lack of the actual functionality, we need to > take away the methods that promise to provide that functionality. I see the > Checker class is a debug build only kind of utility. > > > SequenceChecker works everywhere in Chrome > > Please remove this comment from the CL description, maybe mention that > SequenceChecker only works properly in builds with DCHECKs enabled. Done. I rewrote the last paragraph of the CL description. https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... File content/browser/dom_storage/dom_storage_task_runner.cc (right): https://codereview.chromium.org/2317253007/diff/1/content/browser/dom_storage... content/browser/dom_storage/dom_storage_task_runner.cc:34: return sequenced_worker_pool_->RunsTasksOnCurrentThread(); On 2016/09/14 00:35:12, michaeln wrote: > On 2016/09/13 14:24:13, fdoray wrote: > > On 2016/09/12 22:49:17, michaeln wrote: > > > i think you've changed the semantics of this method? it's supposed to be > > > equivalent to IsRunningOnPrimarySequence() > > > > This method is part of the TaskRunner interface > > https://cs.chromium.org/chromium/src/base/task_runner.h?l=82 "Returns true if > > the current thread is a thread on which a task may be run, and false if no > task > > will be run on the current thread." > > > > Derived classes shouldn't have a different description for this method. Can > you > > point me to a call site where having an implementation that complies with the > > description in task_runner.h doesn't work? > > The base class is too abstract to provide a full description of its various > concrete impls. > > hmmm... if we can't provide a useful impl for this concrete class, maybe just > return true; > > Given that infrastructure no longer provides the ability to check if we're > actually running on the desired sequence in a release build, i suppose we have > no choice but to alter the semantics of this method of this class. Since the > comment in the base class says its valid to always return true, i'd vote to do > that so its more clear its not actually a useful method of this implementation > of taskrunner. Fortunately, I don't see any dom_storage related callsites where > it matters. Done > > You asked for a ptr to a call site where a more discerning impl would matter, > here's one outside of the dom_storage use case... > base_session_service.cc line 37 > > If the method always returns true (a fully description compliant impl), that > callsite will never branch. > > If the runner is backed by a logical sequence and the method returns true when > on a different sequence (which i think is the best we can do know based on what > you said), this code would do the wrong thing. As code is migrated to base/task_scheduler, more tasks will run on sequences rather than specific thread. In that world, I believe that TaskRunner::RunsTasksOnCurrentThread (with its current description) isn't helpful and should be removed. It could be replaced with: // Returns true iff the current task can't run at the same // time as another task posted to this TaskRunner. In // particular, will return true if the current task was posted // to this TaskRunner. bool IsSequencedWithCurrentTask() const = 0; But all of this won't be done immediately.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/2317253007/#ps20001 (title: "CR michaeln #9")
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 ========== Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc SequencedWorkerPool is being migrated to base/task_scheduler and there is no equivalent to IsRunningSequenceOnCurrentThread() in base/task_scheduler. Developers should use SequenceChecker to verify that tasks run sequentially. Note: SequenceChecker::CalledOnValidSequence is a no-op in non-DCHECK builds. BUG=622400 ========== to ========== Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc SequencedWorkerPool is being migrated to base/task_scheduler and there is no equivalent to IsRunningSequenceOnCurrentThread() in base/task_scheduler. Developers should use SequenceChecker to verify that tasks run sequentially. Note: SequenceChecker::CalledOnValidSequence is a no-op in non-DCHECK builds. BUG=622400 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc SequencedWorkerPool is being migrated to base/task_scheduler and there is no equivalent to IsRunningSequenceOnCurrentThread() in base/task_scheduler. Developers should use SequenceChecker to verify that tasks run sequentially. Note: SequenceChecker::CalledOnValidSequence is a no-op in non-DCHECK builds. BUG=622400 ========== to ========== Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc SequencedWorkerPool is being migrated to base/task_scheduler and there is no equivalent to IsRunningSequenceOnCurrentThread() in base/task_scheduler. Developers should use SequenceChecker to verify that tasks run sequentially. Note: SequenceChecker::CalledOnValidSequence is a no-op in non-DCHECK builds. BUG=622400 Committed: https://crrev.com/7cffceeef702204d07f906a905019f650c12e0ba Cr-Commit-Position: refs/heads/master@{#418554} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7cffceeef702204d07f906a905019f650c12e0ba Cr-Commit-Position: refs/heads/master@{#418554}
Message was sent while issue was closed.
> As code is migrated to base/task_scheduler, more tasks will run on sequences > rather than specific thread. In that world, I believe that > TaskRunner::RunsTasksOnCurrentThread (with its current description) isn't > helpful and should be removed. > > It could be replaced with: > > // Returns true iff the current task can't run at the same > // time as another task posted to this TaskRunner. In > // particular, will return true if the current task was posted > // to this TaskRunner. > bool IsSequencedWithCurrentTask() const = 0; > > But all of this won't be done immediately. It might make sense to DEPRECATE the existing RunsTasksOnCurrentThread() method and puts some words of warning in the .h file about how it's not trustworthy due to the migration to task_scheduler.
Message was sent while issue was closed.
On 2016/09/14 19:13:50, michaeln wrote: > > As code is migrated to base/task_scheduler, more tasks will run on sequences > > rather than specific thread. In that world, I believe that > > TaskRunner::RunsTasksOnCurrentThread (with its current description) isn't > > helpful and should be removed. > > > > It could be replaced with: > > > > // Returns true iff the current task can't run at the same > > // time as another task posted to this TaskRunner. In > > // particular, will return true if the current task was posted > > // to this TaskRunner. > > bool IsSequencedWithCurrentTask() const = 0; > > > > But all of this won't be done immediately. > > It might make sense to DEPRECATE the existing RunsTasksOnCurrentThread() method > and puts some words of warning in the .h file about how it's not trustworthy due > to the migration to task_scheduler. We opened a bug this morning crbug.com/646905 . We plan to find a new name and description for RunsTasksOnCurrentThread() that matches developer's needs / expectations. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
