TaskScheduler: Set the IO allowed bit in TaskTracker::RunTask().

base/task_scheduler/task_tracker.cc

Index: base/task_scheduler/task_tracker.cc
diff --git a/base/task_scheduler/task_tracker.cc b/base/task_scheduler/task_tracker.cc
index d06a84dc1f47dd7a4cb16f4463a4fa0c486a6dca..dc78409894c745ad3ba92c7d15074cefcc74b379 100644
--- a/base/task_scheduler/task_tracker.cc
+++ b/base/task_scheduler/task_tracker.cc
@@ -219,12 +219,12 @@ bool TaskTracker::RunTask(std::unique_ptr<Task> task,
   const bool is_delayed = !task->delayed_run_time.is_null();
 
   if (can_run_task) {
-    // All tasks run through here and the scheduler itself doesn't use

[danakj, 2016/11/28 22:55:56]

This comment is no less true now is it? So IOAllowed should be able to follow
the same pattern and just leave the thread in whatever state since the next task
on the thread will come through here? Just making sure I understand, and then
wondering why the change?

[fdoray, 2016/11/29 15:41:20]

I decided to reset ThreadRestrictions in RunTask() to avoid having singletons
and/or IO disallowed on the main thread after running TaskTracker tests.

We could also reset ThreadRestrictions from
TaskSchedulerTaskTrackerTest::TearDown(). I don't like this solution because
once we add wait restrictions:
- We would need a ScopedWaitAllowed in SchedulerWorker::WaitForWork(). Might as
well reset the state from RunTask().
- - TaskSchedulerTaskTrackerTest would need to be a friend of ThreadRestrictions
to reset the wait allowed bit.

-    // singletons. Therefore, it isn't necessary to reset the singleton allowed
-    // bit after running the task.
-    ThreadRestrictions::SetSingletonAllowed(
-        task->traits.shutdown_behavior() !=
-        TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN);
+    const bool previous_singleton_allowed =
+        ThreadRestrictions::SetSingletonAllowed(
+            task->traits.shutdown_behavior() !=
+            TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN);
+    const bool previous_io_allowed =
+        ThreadRestrictions::SetIOAllowed(task->traits.with_file_io());

[danakj, 2016/11/28 22:55:56]

Is reading and writing thread-local storage before every task going to show up
in profiling? Seems like since threads are owned by task scheduler it could do
something cheaper.

[fdoray, 2016/11/29 15:41:20]

I don't think we need to optimize this since ThreadRestrictions methods are
no-ops in non-DCHECK builds. In release with DCHECK builds,
SetSingletonAllowed() takes 10 ns on my Windows Z840.

  TimeTicks start = TimeTicks::Now();
  constexpr size_t kIterations = 1000000000;
  for (size_t i = 0; i < kIterations; ++i)
    ThreadRestrictions::SetSingletonAllowed(i % 2 == 0);
  TimeTicks end = TimeTicks::Now();
  LOG(ERROR) << "Elapsed: " << (end - start);

[danakj, 2016/12/01 21:59:04]

The assert methods are no-ops. The setters are not.
https://cs.chromium.org/chromium/src/base/threading/thread_restrictions.cc?rc...
While that's quick per run, hundreds of tasks run per frame in chrome, perhaps?
So thats getting into microseconds out of a 6ms frame budget with 144hz
monitors. Is this wise?
 

[fdoray, 2016/12/01 22:30:43]

Yes they are. The whole implementation file is guarded by a DCHECK_IS_ON()
https://cs.chromium.org/chromium/src/base/threading/thread_restrictions.cc?rc....
There is a no-op implementation when !DCHECK_IS_ON()
https://cs.chromium.org/chromium/src/base/threading/thread_restrictions.h?sq=...

 
     {
       // Set up SequenceToken as expected for the scope of the task.
@@ -261,6 +261,9 @@ bool TaskTracker::RunTask(std::unique_ptr<Task> task,
       PerformRunTask(std::move(task));
     }
 
+    ThreadRestrictions::SetIOAllowed(previous_io_allowed);
+    ThreadRestrictions::SetSingletonAllowed(previous_singleton_allowed);
+
     AfterRunTask(shutdown_behavior);
   }