Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341063002:
Make SequencedWorkerPool::IsRunningSequenceOnCurrentThread() private. (Closed)
DescriptionMake SequencedWorkerPool::IsRunningSequenceOnCurrentThread() private.
This method is not used outside of SequencedWorkerPool itself. Making it private
will prevent external users from using it directly, which will help the deprecation
of SequencedWorkerPool.
SequencedWorkerPool::IsRunningSequenceOnCurrentThread() is still used
by the RunsTasksOnCurrentThread() method of SequencedTaskRunners
returned by a SequencedWokrerPool.
BUG=622400
Committed: https://crrev.com/2c56f2de0b3d427b358872fd84535932cff39e86
Cr-Commit-Position: refs/heads/master@{#418984}
Patch Set 1 #
Total comments: 6
Patch Set 2 : CR gab/robliao #3-4 #
Total comments: 7
Patch Set 3 : CR danakj #10 (sort forward decls) #Patch Set 4 : CR danakj #12 (add comments in tests) #
Messages
Total messages: 21 (6 generated)
fdoray@chromium.org changed reviewers: + danakj@chromium.org, gab@chromium.org, robliao@chromium.org
PTAL
https://codereview.chromium.org/2341063002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2341063002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:844: DCHECK_NE(subtle::NoBarrier_Load(&g_all_pools_state), Actually, under redirection: SWP::GetSequencedTaskRunner() will still return a SWP TaskRunner, not one from the scheduler. This works because the hook is in PostTask() anyways, but this method can definitely be called in both modes. Note: maybe we should be returning TaskScheduler provided TaskRunners but then not all redirected tasks will go through SWP, haven't thought through whether that'd be okay. https://codereview.chromium.org/2341063002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:847: if (found == threads_.end()) Incorporate below, i.e. return found != threads_.end() && ...
https://codereview.chromium.org/2341063002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2341063002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:180: // SequencedWorkerPool::SequencedWorkerPoolSequencedTaskRunner --------------- Make SequencedWorkerPool::SequencedTaskRunner or SequencedWorkerPool::PoolTaskRunner to reduce duplication and avoid collisions with the existing SequencedTaskRunner.
Description was changed from ========== Make SequencedWorkerPool::IsRunningSequenceOnCurrentThread() private. This method is not used outside of base/. Make it private removes the need to implement it for the case where redirection to TaskScheduler is enabled. BUG=622400 ========== to ========== Make SequencedWorkerPool::IsRunningSequenceOnCurrentThread() private. Making this method private will help the migration of SequencedWorkerPool to base/task_scheduler. BUG=622400 ==========
PTAnL https://codereview.chromium.org/2341063002/diff/1/base/threading/sequenced_wo... File base/threading/sequenced_worker_pool.cc (right): https://codereview.chromium.org/2341063002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:180: // SequencedWorkerPool::SequencedWorkerPoolSequencedTaskRunner --------------- On 2016/09/15 14:14:16, robliao wrote: > Make SequencedWorkerPool::SequencedTaskRunner or > SequencedWorkerPool::PoolTaskRunner to reduce duplication and avoid collisions > with the existing SequencedTaskRunner. Done. (PoolSequencedTaskRunner instead of PoolTaskRunner since there is also a "SequencedWorkerPoolTaskRunner") https://codereview.chromium.org/2341063002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:844: DCHECK_NE(subtle::NoBarrier_Load(&g_all_pools_state), On 2016/09/15 13:50:47, gab wrote: > Actually, under redirection: SWP::GetSequencedTaskRunner() will still return a > SWP TaskRunner, not one from the scheduler. This works because the hook is in > PostTask() anyways, but this method can definitely be called in both modes. > > Note: maybe we should be returning TaskScheduler provided TaskRunners but then > not all redirected tasks will go through SWP, haven't thought through whether > that'd be okay. You're right. This CL no longer changes IsRunningSequenceOnCurrentThread(). I'll work on the TODO in a separate CL. https://codereview.chromium.org/2341063002/diff/1/base/threading/sequenced_wo... base/threading/sequenced_worker_pool.cc:847: if (found == threads_.end()) On 2016/09/15 13:50:47, gab wrote: > Incorporate below, i.e. > > return found != threads_.end() && ... I'll fix this method in a separate CL.
lgtm % comment https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:810: sequenced_task_runner_1, base::RetainedRef(pool()), This maps unsequenced_token -> sequenced_task_runner_1? Shouldn't that be unsequenced_task_runner?
danakj@: PTAL https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:810: sequenced_task_runner_1, base::RetainedRef(pool()), On 2016/09/15 16:34:39, gab wrote: > This maps unsequenced_token -> sequenced_task_runner_1? Shouldn't that be > unsequenced_task_runner? If we do that, we'll verify that unsequenced_task_runner->RunsTasksOnCurrentThread() returns false when called from a task posted to |sequenced_task_runner_2|. However, it returns true. unsequenced_task_runner->RunsTasksOnCurrentThread() is not equivalent to pool()->IsRunningSequenceOnCurrentThread(unsequenced_task_runner). The first call returns true if |unsequenced_task_runner| may run tasks on the current thread. The second call returns true if the current thread is running an unsequenced task.
> danakj@: PTAL https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:810: sequenced_task_runner_1, base::RetainedRef(pool()), On 2016/09/15 16:47:15, fdoray wrote: > On 2016/09/15 16:34:39, gab wrote: > > This maps unsequenced_token -> sequenced_task_runner_1? Shouldn't that be > > unsequenced_task_runner? > > If we do that, we'll verify that > unsequenced_task_runner->RunsTasksOnCurrentThread() returns false when called > from a task posted to |sequenced_task_runner_2|. However, it returns true. > > unsequenced_task_runner->RunsTasksOnCurrentThread() is not equivalent to > pool()->IsRunningSequenceOnCurrentThread(unsequenced_task_runner). > > The first call returns true if |unsequenced_task_runner| may run tasks on the > current thread. The second call returns true if the current thread is running an > unsequenced task. Ah, got it, thanks!
> Making this method private will help the migration of > SequencedWorkerPool to base/task_scheduler. I saw more explanation in comments, can you add it here? https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.h:369: class PoolSequencedTaskRunner; sort please
On 2016/09/15 18:18:24, danakj wrote: > > Making this method private will help the migration of > > SequencedWorkerPool to base/task_scheduler. Oh it was in the original description, hm..
LGTM https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:802: sequenced_task_runner_1->PostTask( Can you add a comment on each of these 3 PostTasks just explaining what condition it is or isn't checking? It's rather indirect, esp getting subtle wrt unsequenced_task_runner.
Description was changed from ========== Make SequencedWorkerPool::IsRunningSequenceOnCurrentThread() private. Making this method private will help the migration of SequencedWorkerPool to base/task_scheduler. BUG=622400 ========== to ========== Make SequencedWorkerPool::IsRunningSequenceOnCurrentThread() private. This method is not used outside of SequencedWorkerPool itself. Making it private will prevent external users from using it directly, which will help the deprecation of SequencedWorkerPool. SequencedWorkerPool::IsRunningSequenceOnCurrentThread() is still used by the RunsTasksOnCurrentThread() method of SequencedTaskRunners returned by a SequencedWokrerPool. BUG=622400 ==========
PTAnL I updated the CL's description. The only goal of this CL is prevent developers from adding new calls to this method. https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool.h (right): https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool.h:369: class PoolSequencedTaskRunner; On 2016/09/15 18:18:24, danakj wrote: > sort please Done.
On 2016/09/15 18:29:06, fdoray wrote: > PTAnL > > I updated the CL's description. The only goal of this CL is prevent developers > from adding new calls to this method. Thanks, looks better!
https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... File base/threading/sequenced_worker_pool_unittest.cc (right): https://codereview.chromium.org/2341063002/diff/20001/base/threading/sequence... base/threading/sequenced_worker_pool_unittest.cc:802: sequenced_task_runner_1->PostTask( On 2016/09/15 18:23:42, danakj wrote: > Can you add a comment on each of these 3 PostTasks just explaining what > condition it is or isn't checking? It's rather indirect, esp getting subtle wrt > unsequenced_task_runner. Done. I removed the second PostTask() since it doesn't test anything different than the first one (just replace 1 <--> 2).
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2341063002/#ps60001 (title: "CR danakj #12 (add comments in tests)")
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 ========== Make SequencedWorkerPool::IsRunningSequenceOnCurrentThread() private. This method is not used outside of SequencedWorkerPool itself. Making it private will prevent external users from using it directly, which will help the deprecation of SequencedWorkerPool. SequencedWorkerPool::IsRunningSequenceOnCurrentThread() is still used by the RunsTasksOnCurrentThread() method of SequencedTaskRunners returned by a SequencedWokrerPool. BUG=622400 ========== to ========== Make SequencedWorkerPool::IsRunningSequenceOnCurrentThread() private. This method is not used outside of SequencedWorkerPool itself. Making it private will prevent external users from using it directly, which will help the deprecation of SequencedWorkerPool. SequencedWorkerPool::IsRunningSequenceOnCurrentThread() is still used by the RunsTasksOnCurrentThread() method of SequencedTaskRunners returned by a SequencedWokrerPool. BUG=622400 Committed: https://crrev.com/2c56f2de0b3d427b358872fd84535932cff39e86 Cr-Commit-Position: refs/heads/master@{#418984} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2c56f2de0b3d427b358872fd84535932cff39e86 Cr-Commit-Position: refs/heads/master@{#418984} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
