Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161213002:
TaskScheduler: Bump thread priority during shutdown. (Closed)
|
Created:
4 years, 5 months ago by fdoray Modified:
4 years, 5 months ago CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTaskScheduler: Bump thread priority during shutdown.
Bump the priority of BACKGROUND threads to NORMAL during shutdown.
This will prevent TaskTracker::Shutdown() from waiting for work to
be done by threads that never get a time slice.
BUG=553459
Committed: https://crrev.com/90324e233c21c76134756f5d9789235fb59b75aa
Cr-Commit-Position: refs/heads/master@{#407147}
Patch Set 1 #
Total comments: 16
Patch Set 2 : CR robliao #5 #
Total comments: 4
Patch Set 3 : CR gab #7 (improve unit tests) #Patch Set 4 : self-review #
Total comments: 6
Patch Set 5 : CR robliao #12 #Patch Set 6 : self-review #
Total comments: 4
Patch Set 7 : CR robliao #14 #Patch Set 8 : no priority bump on Linux #
Total comments: 3
Patch Set 9 : CR gab #26 (Set* -> Update* and disable on Linux) #
Messages
Total messages: 35 (11 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org
fdoray@chromium.org changed reviewers: + robliao@chromium.org
PTAL
Approach looks good. https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker.cc:32: bool thread_priority_bumped_for_shutdown = false; I wonder if it might be more extensible to keep track of the current priority, and that would change your if statement below to... if (outer_->task_tracker->IsShutdownInProgress() && thread_priority_ != ThreadPriority::NORMAL) { thread_priority_ = ThreadPriority::NORMAL; PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); } If GetCurrentPriority is fast, I'd go with that, but I speculate it comes with non-trivial cost. https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_unittest.cc:434: void WaitDone() { done_.Wait(); } Nit: Maybe WaitForGetWork https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_unittest.cc:445: TimeDelta GetSleepTimeout() override { return TimeDelta(); } Would be safer to return TimeDelta::Max() https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:231: new WaitableEvent(WaitableEvent::ResetPolicy::MANUAL, Unify this event creation with the one in Shutdown() https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.h:55: // after this. Comment why one would call this instead of Shutdown().
PTAnL https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker.cc:32: bool thread_priority_bumped_for_shutdown = false; On 2016/07/19 22:41:28, robliao wrote: > I wonder if it might be more extensible to keep track of the current priority, > and that would change your if statement below to... > > if (outer_->task_tracker->IsShutdownInProgress() && > thread_priority_ != ThreadPriority::NORMAL) { > thread_priority_ = ThreadPriority::NORMAL; > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > } > > If GetCurrentPriority is fast, I'd go with that, but I speculate it comes with > non-trivial cost. I like the idea of keeping track of |thread_priority_|. Not sure why the speed of GetCurrentPriority matters? FYI, some benchmarks: Windows - GetCurrentThreadPriority: 127ns Windows - SetCurrentThreadPriority: 124ns Mac - GetCurrentThreadPriority: 68ns Mac - SetCurrentThreadPriority: 936ns class Timer { public: ~Timer() { const TimeDelta elapsed_time = TimeTicks::Now() - start; LOG(ERROR) << "Elapsed time: " << elapsed_time; } private: const TimeTicks start = TimeTicks::Now(); }; constexpr size_t kNumIterations = 100000000; volatile int counter = 0; TEST(ThreadPriorityBenchmark, Counter) { Timer timer; for (size_t i = 0; i < kNumIterations; ++i) { ++counter; } } TEST(ThreadPriorityBenchmark, SetCurrentThreadPriority) { Timer timer; for (size_t i = 0; i < kNumIterations; ++i) { PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); ++counter; } } TEST(ThreadPriorityBenchmark, GetCurrentThreadPriority) { Timer timer; for (size_t i = 0; i < kNumIterations; ++i) { PlatformThread::GetCurrentThreadPriority(); ++counter; } } https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_unittest.cc:434: void WaitDone() { done_.Wait(); } On 2016/07/19 22:41:28, robliao wrote: > Nit: Maybe WaitForGetWork Done. https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_unittest.cc:445: TimeDelta GetSleepTimeout() override { return TimeDelta(); } On 2016/07/19 22:41:28, robliao wrote: > Would be safer to return TimeDelta::Max() Done. https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.cc:231: new WaitableEvent(WaitableEvent::ResetPolicy::MANUAL, On 2016/07/19 22:41:28, robliao wrote: > Unify this event creation with the one in Shutdown() n/a https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_tracker.h:55: // after this. On 2016/07/19 22:41:28, robliao wrote: > Comment why one would call this instead of Shutdown(). Done.
code lg, test comments though https://codereview.chromium.org/2161213002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2161213002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_unittest.cc:458: task_tracker.SetHasShutdownStartedForTesting(); Just to make sure, if you comment out this line, the test fails, right? I'd be more comfortable if the test was tested, currently it could pass in a number of ways with a bug elsewhere in the code it feels. How about have an ExpectPriorityDelegate on which the expectation can be updated. Then verify that a first task runs in BACKGROUND mode, then set shutdown, and verify priority of next task? Also, instead of doing the verifications in the Delegate I think we should post actual tasks and do the verification inside of them -- so maybe this test belongs more as an integration test at the SWPool level. https://codereview.chromium.org/2161213002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_unittest.cc:460: ExpectNormalPriorityDelegate* delegate = new ExpectNormalPriorityDelegate; Bind to unique_ptr here and move it below (or WrapUnique the inline new call -- that's what WrapUnique is meant for)
Quick comment. I still need to look through the updates. https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker.cc:32: bool thread_priority_bumped_for_shutdown = false; On 2016/07/20 18:22:21, fdoray wrote: > On 2016/07/19 22:41:28, robliao wrote: > > I wonder if it might be more extensible to keep track of the current priority, > > and that would change your if statement below to... > > > > if (outer_->task_tracker->IsShutdownInProgress() && > > thread_priority_ != ThreadPriority::NORMAL) { > > thread_priority_ = ThreadPriority::NORMAL; > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > } > > > > If GetCurrentPriority is fast, I'd go with that, but I speculate it comes with > > non-trivial cost. > > I like the idea of keeping track of |thread_priority_|. Not sure why the speed > of GetCurrentPriority matters? > > FYI, some benchmarks: > > Windows - GetCurrentThreadPriority: 127ns > Windows - SetCurrentThreadPriority: 124ns > Mac - GetCurrentThreadPriority: 68ns > Mac - SetCurrentThreadPriority: 936ns > > class Timer { > public: > ~Timer() { > const TimeDelta elapsed_time = TimeTicks::Now() - start; > LOG(ERROR) << "Elapsed time: " << elapsed_time; > } > > private: > const TimeTicks start = TimeTicks::Now(); > }; > > constexpr size_t kNumIterations = 100000000; > volatile int counter = 0; > > TEST(ThreadPriorityBenchmark, Counter) { > Timer timer; > for (size_t i = 0; i < kNumIterations; ++i) { > ++counter; > } > } > > TEST(ThreadPriorityBenchmark, SetCurrentThreadPriority) { > Timer timer; > for (size_t i = 0; i < kNumIterations; ++i) { > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > ++counter; > } > } > > TEST(ThreadPriorityBenchmark, GetCurrentThreadPriority) { > Timer timer; > for (size_t i = 0; i < kNumIterations; ++i) { > PlatformThread::GetCurrentThreadPriority(); > ++counter; > } > } If GetCurrentThreadPriority is nearly as fast as reading the variable, we can just do that instead of storing the state ourselves (and opening the door to potential synchronization bugs).
https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker.cc:32: bool thread_priority_bumped_for_shutdown = false; On 2016/07/20 19:14:57, robliao wrote: > On 2016/07/20 18:22:21, fdoray wrote: > > On 2016/07/19 22:41:28, robliao wrote: > > > I wonder if it might be more extensible to keep track of the current > priority, > > > and that would change your if statement below to... > > > > > > if (outer_->task_tracker->IsShutdownInProgress() && > > > thread_priority_ != ThreadPriority::NORMAL) { > > > thread_priority_ = ThreadPriority::NORMAL; > > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > > } > > > > > > If GetCurrentPriority is fast, I'd go with that, but I speculate it comes > with > > > non-trivial cost. > > > > I like the idea of keeping track of |thread_priority_|. Not sure why the speed > > of GetCurrentPriority matters? > > > > FYI, some benchmarks: > > > > Windows - GetCurrentThreadPriority: 127ns > > Windows - SetCurrentThreadPriority: 124ns > > Mac - GetCurrentThreadPriority: 68ns > > Mac - SetCurrentThreadPriority: 936ns > > > > class Timer { > > public: > > ~Timer() { > > const TimeDelta elapsed_time = TimeTicks::Now() - start; > > LOG(ERROR) << "Elapsed time: " << elapsed_time; > > } > > > > private: > > const TimeTicks start = TimeTicks::Now(); > > }; > > > > constexpr size_t kNumIterations = 100000000; > > volatile int counter = 0; > > > > TEST(ThreadPriorityBenchmark, Counter) { > > Timer timer; > > for (size_t i = 0; i < kNumIterations; ++i) { > > ++counter; > > } > > } > > > > TEST(ThreadPriorityBenchmark, SetCurrentThreadPriority) { > > Timer timer; > > for (size_t i = 0; i < kNumIterations; ++i) { > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > ++counter; > > } > > } > > > > TEST(ThreadPriorityBenchmark, GetCurrentThreadPriority) { > > Timer timer; > > for (size_t i = 0; i < kNumIterations; ++i) { > > PlatformThread::GetCurrentThreadPriority(); > > ++counter; > > } > > } > > If GetCurrentThreadPriority is nearly as fast as reading the variable, we can > just do that instead of storing the state ourselves (and opening the door to > potential synchronization bugs). I much prefer a local variable to a call to the OS here.
PTAnL https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker.cc:32: bool thread_priority_bumped_for_shutdown = false; On 2016/07/20 19:14:57, robliao wrote: > On 2016/07/20 18:22:21, fdoray wrote: > > On 2016/07/19 22:41:28, robliao wrote: > > > I wonder if it might be more extensible to keep track of the current > priority, > > > and that would change your if statement below to... > > > > > > if (outer_->task_tracker->IsShutdownInProgress() && > > > thread_priority_ != ThreadPriority::NORMAL) { > > > thread_priority_ = ThreadPriority::NORMAL; > > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > > } > > > > > > If GetCurrentPriority is fast, I'd go with that, but I speculate it comes > with > > > non-trivial cost. > > > > I like the idea of keeping track of |thread_priority_|. Not sure why the speed > > of GetCurrentPriority matters? > > > > FYI, some benchmarks: > > > > Windows - GetCurrentThreadPriority: 127ns > > Windows - SetCurrentThreadPriority: 124ns > > Mac - GetCurrentThreadPriority: 68ns > > Mac - SetCurrentThreadPriority: 936ns > > > > class Timer { > > public: > > ~Timer() { > > const TimeDelta elapsed_time = TimeTicks::Now() - start; > > LOG(ERROR) << "Elapsed time: " << elapsed_time; > > } > > > > private: > > const TimeTicks start = TimeTicks::Now(); > > }; > > > > constexpr size_t kNumIterations = 100000000; > > volatile int counter = 0; > > > > TEST(ThreadPriorityBenchmark, Counter) { > > Timer timer; > > for (size_t i = 0; i < kNumIterations; ++i) { > > ++counter; > > } > > } > > > > TEST(ThreadPriorityBenchmark, SetCurrentThreadPriority) { > > Timer timer; > > for (size_t i = 0; i < kNumIterations; ++i) { > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > ++counter; > > } > > } > > > > TEST(ThreadPriorityBenchmark, GetCurrentThreadPriority) { > > Timer timer; > > for (size_t i = 0; i < kNumIterations; ++i) { > > PlatformThread::GetCurrentThreadPriority(); > > ++counter; > > } > > } > > If GetCurrentThreadPriority is nearly as fast as reading the variable, we can > just do that instead of storing the state ourselves (and opening the door to > potential synchronization bugs). GetCurrentThreadPriority() is fast but not sure that 127ns is "nearly as fast" as reading a variable that can be in cache. Do you think we should still use it? https://codereview.chromium.org/2161213002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2161213002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_unittest.cc:458: task_tracker.SetHasShutdownStartedForTesting(); On 2016/07/20 18:35:36, gab wrote: > Just to make sure, if you comment out this line, the test fails, right? > > I'd be more comfortable if the test was tested, currently it could pass in a > number of ways with a bug elsewhere in the code it feels. > > How about have an ExpectPriorityDelegate on which the expectation can be > updated. Then verify that a first task runs in BACKGROUND mode, then set > shutdown, and verify priority of next task? > > Also, instead of doing the verifications in the Delegate I think we should post > actual tasks and do the verification inside of them -- so maybe this test > belongs more as an integration test at the SWPool level. As discussed offline, implemented ExpectPriorityDelegate. https://codereview.chromium.org/2161213002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_unittest.cc:460: ExpectNormalPriorityDelegate* delegate = new ExpectNormalPriorityDelegate; On 2016/07/20 18:35:36, gab wrote: > Bind to unique_ptr here and move it below (or WrapUnique the inline new call -- > that's what WrapUnique is meant for) Done.
lgtm
lgtm + comments. https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker.cc:32: bool thread_priority_bumped_for_shutdown = false; On 2016/07/20 20:30:37, fdoray wrote: > On 2016/07/20 19:14:57, robliao wrote: > > On 2016/07/20 18:22:21, fdoray wrote: > > > On 2016/07/19 22:41:28, robliao wrote: > > > > I wonder if it might be more extensible to keep track of the current > > priority, > > > > and that would change your if statement below to... > > > > > > > > if (outer_->task_tracker->IsShutdownInProgress() && > > > > thread_priority_ != ThreadPriority::NORMAL) { > > > > thread_priority_ = ThreadPriority::NORMAL; > > > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > > > } > > > > > > > > If GetCurrentPriority is fast, I'd go with that, but I speculate it comes > > with > > > > non-trivial cost. > > > > > > I like the idea of keeping track of |thread_priority_|. Not sure why the > speed > > > of GetCurrentPriority matters? > > > > > > FYI, some benchmarks: > > > > > > Windows - GetCurrentThreadPriority: 127ns > > > Windows - SetCurrentThreadPriority: 124ns > > > Mac - GetCurrentThreadPriority: 68ns > > > Mac - SetCurrentThreadPriority: 936ns > > > > > > class Timer { > > > public: > > > ~Timer() { > > > const TimeDelta elapsed_time = TimeTicks::Now() - start; > > > LOG(ERROR) << "Elapsed time: " << elapsed_time; > > > } > > > > > > private: > > > const TimeTicks start = TimeTicks::Now(); > > > }; > > > > > > constexpr size_t kNumIterations = 100000000; > > > volatile int counter = 0; > > > > > > TEST(ThreadPriorityBenchmark, Counter) { > > > Timer timer; > > > for (size_t i = 0; i < kNumIterations; ++i) { > > > ++counter; > > > } > > > } > > > > > > TEST(ThreadPriorityBenchmark, SetCurrentThreadPriority) { > > > Timer timer; > > > for (size_t i = 0; i < kNumIterations; ++i) { > > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > > ++counter; > > > } > > > } > > > > > > TEST(ThreadPriorityBenchmark, GetCurrentThreadPriority) { > > > Timer timer; > > > for (size_t i = 0; i < kNumIterations; ++i) { > > > PlatformThread::GetCurrentThreadPriority(); > > > ++counter; > > > } > > > } > > > > If GetCurrentThreadPriority is nearly as fast as reading the variable, we can > > just do that instead of storing the state ourselves (and opening the door to > > potential synchronization bugs). > > GetCurrentThreadPriority() is fast but not sure that 127ns is "nearly as fast" > as reading a variable that can be in cache. Do you think we should still use it? This state is already stored in the TEB, so I'd rather reference that than duplicate the state here. 127ns is a bit too long for my liking, so let's move the SetCurrentThreadPriority call and actual_thread_priority_ update into a separate method to avoid desynchronization. https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:149: ThreadPriority actual_thread_priority_; Use current_thread_priority since that's what it's tracking. https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_unittest.cc:484: // Create an ALIVE thread. Nit: Remove this comment. https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:54: // Causes HasShutdownStarted() to return true. Contrary to Shutdown(), Nit: s/Contrary to Shutdown()/Unlike Shutdown()/
https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker.cc:32: bool thread_priority_bumped_for_shutdown = false; On 2016/07/20 22:31:48, robliao wrote: > On 2016/07/20 20:30:37, fdoray wrote: > > On 2016/07/20 19:14:57, robliao wrote: > > > On 2016/07/20 18:22:21, fdoray wrote: > > > > On 2016/07/19 22:41:28, robliao wrote: > > > > > I wonder if it might be more extensible to keep track of the current > > > priority, > > > > > and that would change your if statement below to... > > > > > > > > > > if (outer_->task_tracker->IsShutdownInProgress() && > > > > > thread_priority_ != ThreadPriority::NORMAL) { > > > > > thread_priority_ = ThreadPriority::NORMAL; > > > > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > > > > } > > > > > > > > > > If GetCurrentPriority is fast, I'd go with that, but I speculate it > comes > > > with > > > > > non-trivial cost. > > > > > > > > I like the idea of keeping track of |thread_priority_|. Not sure why the > > speed > > > > of GetCurrentPriority matters? > > > > > > > > FYI, some benchmarks: > > > > > > > > Windows - GetCurrentThreadPriority: 127ns > > > > Windows - SetCurrentThreadPriority: 124ns > > > > Mac - GetCurrentThreadPriority: 68ns > > > > Mac - SetCurrentThreadPriority: 936ns > > > > > > > > class Timer { > > > > public: > > > > ~Timer() { > > > > const TimeDelta elapsed_time = TimeTicks::Now() - start; > > > > LOG(ERROR) << "Elapsed time: " << elapsed_time; > > > > } > > > > > > > > private: > > > > const TimeTicks start = TimeTicks::Now(); > > > > }; > > > > > > > > constexpr size_t kNumIterations = 100000000; > > > > volatile int counter = 0; > > > > > > > > TEST(ThreadPriorityBenchmark, Counter) { > > > > Timer timer; > > > > for (size_t i = 0; i < kNumIterations; ++i) { > > > > ++counter; > > > > } > > > > } > > > > > > > > TEST(ThreadPriorityBenchmark, SetCurrentThreadPriority) { > > > > Timer timer; > > > > for (size_t i = 0; i < kNumIterations; ++i) { > > > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > > > ++counter; > > > > } > > > > } > > > > > > > > TEST(ThreadPriorityBenchmark, GetCurrentThreadPriority) { > > > > Timer timer; > > > > for (size_t i = 0; i < kNumIterations; ++i) { > > > > PlatformThread::GetCurrentThreadPriority(); > > > > ++counter; > > > > } > > > > } > > > > > > If GetCurrentThreadPriority is nearly as fast as reading the variable, we > can > > > just do that instead of storing the state ourselves (and opening the door to > > > potential synchronization bugs). > > > > GetCurrentThreadPriority() is fast but not sure that 127ns is "nearly as fast" > > as reading a variable that can be in cache. Do you think we should still use > it? > > This state is already stored in the TEB, so I'd rather reference that than > duplicate the state here. 127ns is a bit too long for my liking, so let's move > the SetCurrentThreadPriority call and actual_thread_priority_ update into a > separate method to avoid desynchronization. Let me know if I understood your comment correctly. https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker.cc:149: ThreadPriority actual_thread_priority_; On 2016/07/20 22:31:48, robliao wrote: > Use current_thread_priority since that's what it's tracking. Done. https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_unittest.cc (right): https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_unittest.cc:484: // Create an ALIVE thread. On 2016/07/20 22:31:48, robliao wrote: > Nit: Remove this comment. Done. https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2161213002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:54: // Causes HasShutdownStarted() to return true. Contrary to Shutdown(), On 2016/07/20 22:31:48, robliao wrote: > Nit: s/Contrary to Shutdown()/Unlike Shutdown()/ Done.
Still lgtm. https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker.cc:32: bool thread_priority_bumped_for_shutdown = false; On 2016/07/21 13:21:56, fdoray wrote: > On 2016/07/20 22:31:48, robliao wrote: > > On 2016/07/20 20:30:37, fdoray wrote: > > > On 2016/07/20 19:14:57, robliao wrote: > > > > On 2016/07/20 18:22:21, fdoray wrote: > > > > > On 2016/07/19 22:41:28, robliao wrote: > > > > > > I wonder if it might be more extensible to keep track of the current > > > > priority, > > > > > > and that would change your if statement below to... > > > > > > > > > > > > if (outer_->task_tracker->IsShutdownInProgress() && > > > > > > thread_priority_ != ThreadPriority::NORMAL) { > > > > > > thread_priority_ = ThreadPriority::NORMAL; > > > > > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > > > > > } > > > > > > > > > > > > If GetCurrentPriority is fast, I'd go with that, but I speculate it > > comes > > > > with > > > > > > non-trivial cost. > > > > > > > > > > I like the idea of keeping track of |thread_priority_|. Not sure why the > > > speed > > > > > of GetCurrentPriority matters? > > > > > > > > > > FYI, some benchmarks: > > > > > > > > > > Windows - GetCurrentThreadPriority: 127ns > > > > > Windows - SetCurrentThreadPriority: 124ns > > > > > Mac - GetCurrentThreadPriority: 68ns > > > > > Mac - SetCurrentThreadPriority: 936ns > > > > > > > > > > class Timer { > > > > > public: > > > > > ~Timer() { > > > > > const TimeDelta elapsed_time = TimeTicks::Now() - start; > > > > > LOG(ERROR) << "Elapsed time: " << elapsed_time; > > > > > } > > > > > > > > > > private: > > > > > const TimeTicks start = TimeTicks::Now(); > > > > > }; > > > > > > > > > > constexpr size_t kNumIterations = 100000000; > > > > > volatile int counter = 0; > > > > > > > > > > TEST(ThreadPriorityBenchmark, Counter) { > > > > > Timer timer; > > > > > for (size_t i = 0; i < kNumIterations; ++i) { > > > > > ++counter; > > > > > } > > > > > } > > > > > > > > > > TEST(ThreadPriorityBenchmark, SetCurrentThreadPriority) { > > > > > Timer timer; > > > > > for (size_t i = 0; i < kNumIterations; ++i) { > > > > > PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL); > > > > > ++counter; > > > > > } > > > > > } > > > > > > > > > > TEST(ThreadPriorityBenchmark, GetCurrentThreadPriority) { > > > > > Timer timer; > > > > > for (size_t i = 0; i < kNumIterations; ++i) { > > > > > PlatformThread::GetCurrentThreadPriority(); > > > > > ++counter; > > > > > } > > > > > } > > > > > > > > If GetCurrentThreadPriority is nearly as fast as reading the variable, we > > can > > > > just do that instead of storing the state ourselves (and opening the door > to > > > > potential synchronization bugs). > > > > > > GetCurrentThreadPriority() is fast but not sure that 127ns is "nearly as > fast" > > > as reading a variable that can be in cache. Do you think we should still use > > it? > > > > This state is already stored in the TEB, so I'd rather reference that than > > duplicate the state here. 127ns is a bit too long for my liking, so let's move > > the SetCurrentThreadPriority call and actual_thread_priority_ update into a > > separate method to avoid desynchronization. > > Let me know if I understood your comment correctly. Yup! Thanks. https://codereview.chromium.org/2161213002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:136: void UpdateThreadPriority(ThreadPriority desired_thread_priority) { Nit: Use SetThreadPriority.
https://codereview.chromium.org/2161213002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:136: void UpdateThreadPriority(ThreadPriority desired_thread_priority) { On 2016/07/21 17:05:51, robliao wrote: > Nit: Use SetThreadPriority. Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2161213002/#ps120001 (title: "CR robliao #14")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2161213002/#ps140001 (title: "no priority bump on Linux")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Gotta run, but comments on latest update. https://codereview.chromium.org/2161213002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:136: void UpdateThreadPriority(ThreadPriority desired_thread_priority) { On 2016/07/21 17:05:51, robliao wrote: > Nit: Use SetThreadPriority. I prefered Update* as it didn't clash with underlying call as much and indicated there's logic behind it (it doesn't just set to |desired_thread_priority| blindly). https://codereview.chromium.org/2161213002/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:143: static_cast<int>(current_thread_priority_)) { If we can't bring it up, we shouldn't bring it down (i.e. don't do any of this on Linux -- and we probably also won't do priorities at all because of SCHED_PRIO_INHERIT IIUC).
https://codereview.chromium.org/2161213002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:136: void UpdateThreadPriority(ThreadPriority desired_thread_priority) { On 2016/07/21 21:29:51, gab wrote: > On 2016/07/21 17:05:51, robliao wrote: > > Nit: Use SetThreadPriority. > > I prefered Update* as it didn't clash with underlying call as much and indicated > there's logic behind it (it doesn't just set to |desired_thread_priority| > blindly). Done. I prefer Update* too. https://codereview.chromium.org/2161213002/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:143: static_cast<int>(current_thread_priority_)) { On 2016/07/21 21:29:51, gab wrote: > If we can't bring it up, we shouldn't bring it down (i.e. don't do any of this > on Linux -- and we probably also won't do priorities at all because of > SCHED_PRIO_INHERIT IIUC). Done. I disabled this method completely on Linux. Another reason not to do priorities at all on Linux (in addition to SCHED_PRIO_INHERIT) would be that we can't bump the priority of BACKGROUND threads during shutdown. In a separate CL, we should disable all code related to priorities on Linux (i.e. we shouldn't allow creating a BACKGROUND worker).
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, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2161213002/#ps160001 (title: "CR gab #26 (Set* -> Update* and disable on Linux)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2161213002/diff/140001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker.cc (right): https://codereview.chromium.org/2161213002/diff/140001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker.cc:143: static_cast<int>(current_thread_priority_)) { On 2016/07/22 12:56:32, fdoray wrote: > On 2016/07/21 21:29:51, gab wrote: > > If we can't bring it up, we shouldn't bring it down (i.e. don't do any of this > > on Linux -- and we probably also won't do priorities at all because of > > SCHED_PRIO_INHERIT IIUC). > > Done. I disabled this method completely on Linux. > > Another reason not to do priorities at all on Linux (in addition to > SCHED_PRIO_INHERIT) would be that we can't bump the priority of BACKGROUND > threads during shutdown. In a separate CL, we should disable all code related to > priorities on Linux (i.e. we shouldn't allow creating a BACKGROUND worker). An alternate option is to detach the BACKGROUND threads on shutdown (as long as we have logic for the foreground pool to pickup BACKGROUND work during shutdown).
On 2016/07/22 13:33:26, gab wrote: > https://codereview.chromium.org/2161213002/diff/140001/base/task_scheduler/sc... > File base/task_scheduler/scheduler_worker.cc (right): > > https://codereview.chromium.org/2161213002/diff/140001/base/task_scheduler/sc... > base/task_scheduler/scheduler_worker.cc:143: > static_cast<int>(current_thread_priority_)) { > On 2016/07/22 12:56:32, fdoray wrote: > > On 2016/07/21 21:29:51, gab wrote: > > > If we can't bring it up, we shouldn't bring it down (i.e. don't do any of > this > > > on Linux -- and we probably also won't do priorities at all because of > > > SCHED_PRIO_INHERIT IIUC). > > > > Done. I disabled this method completely on Linux. > > > > Another reason not to do priorities at all on Linux (in addition to > > SCHED_PRIO_INHERIT) would be that we can't bump the priority of BACKGROUND > > threads during shutdown. In a separate CL, we should disable all code related > to > > priorities on Linux (i.e. we shouldn't allow creating a BACKGROUND worker). > > An alternate option is to detach the BACKGROUND threads on shutdown (as long as > we have logic for the foreground pool to pickup BACKGROUND work during > shutdown). (still lgtm -- just thoughts for later, added to LL tasks sheet)
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Bump thread priority during shutdown. Bump the priority of BACKGROUND threads to NORMAL during shutdown. This will prevent TaskTracker::Shutdown() from waiting for work to be done by threads that never get a time slice. BUG=553459 ========== to ========== TaskScheduler: Bump thread priority during shutdown. Bump the priority of BACKGROUND threads to NORMAL during shutdown. This will prevent TaskTracker::Shutdown() from waiting for work to be done by threads that never get a time slice. BUG=553459 Committed: https://crrev.com/90324e233c21c76134756f5d9789235fb59b75aa Cr-Commit-Position: refs/heads/master@{#407147} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/90324e233c21c76134756f5d9789235fb59b75aa Cr-Commit-Position: refs/heads/master@{#407147} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
