Task Scheduler [1/8] Task Traits

base/task_scheduler/task_traits.h

Index: base/task_scheduler/task_traits.h
diff --git a/base/task_scheduler/task_traits.h b/base/task_scheduler/task_traits.h
new file mode 100644
index 0000000000000000000000000000000000000000..8b564f44f5a5cdb8477ac9e2f4966d807e1e2204
--- /dev/null
+++ b/base/task_scheduler/task_traits.h
@@ -0,0 +1,122 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_TASK_SCHEDULER_TASK_TRAITS_H_
+#define BASE_TASK_SCHEDULER_TASK_TRAITS_H_
+
+#include "base/base_export.h"
+#include "build/build_config.h"
+
+namespace base {
+
+using TaskPriorityUnderlyingType = char;
+
+// Valid priorities supported by the task scheduler.

[gab, 2016/02/17 19:27:55]

Please add to this comment that values for each entry must match priority
(higher value == higher priority). This property is used in later parts of the
impl.

[robliao, 2016/02/17 20:41:09]

Done.

+enum class TaskPriority : TaskPriorityUnderlyingType {
+  // This task affects UI immediately after a user interaction.
+  // Example: Generating data shown in the UI immediately after a click.
+  USER_BLOCKING = 2,

[jam, 2016/02/17 16:50:08]

nit: just wondering why you're starting this with 2? i.e. seems that nothing
will ever be more important than USER_BLOCKING, but maybe later we will expand
the background case into several different priorities

[robliao, 2016/02/17 19:06:00]

It's currently an implementation detail that higher values mean higher priority
in our scheduler. We could space this out a bit more to allow for potential
future expansion. 

What do you folks think about
USER_BLOCKING = 100
USER_VISIBLE = 75
BACKGROUND = 50
?

[gab, 2016/02/17 19:27:55]

I think keeping 0,1,2 for now is fine. We don't persist these anywhere so we
could easily bump them later if we introduce intermediate ones.

I'm also fine making USER_BLOCKING a max value to express to a reader that it
will always be highest.

[robliao, 2016/02/17 20:41:09]

After given it some more thought, 0, 1, and 2 and changing values as needed
should be fine. Consumers should not depend on the actual values of these enums.
They should instead be relying on the established ordering, which we can make
sure to preserve should we change the values.

[jam, 2016/02/17 21:48:26]

sgtm to change later as needed. i didn't know the values are implementation
dependent, so enforcing that through compile checks would be good to document it

[robliao, 2016/02/17 23:12:56]

Done. Added a static_assert.

+  // This task affects UI or responsiveness of future user interactions. It is
+  // not an immediate response to a user interaction.
+  // Examples:
+  // - Updating the UI to reflect progress on a long task.
+  // - Loading data that might be shown in the UI after a future user
+  //   interaction.
+  USER_VISIBLE = 1,
+  // Everything else (user won't notice if this takes an arbitrarily long time
+  // to complete).
+  BACKGROUND = 0,
+};
+
+const TaskPriorityUnderlyingType kNumTaskPriorities = 3;
+
+// Valid shutdown behaviors supported by the task scheduler.
+enum class TaskShutdownBehavior {
+  // Tasks posted with this mode which have not started executing before
+  // shutdown is initiated will never run. Tasks with this mode running at
+  // shutdown will be ignored (the worker thread will not be joined).
+  //
+  // This option provides a nice way to post stuff you don't want blocking
+  // shutdown. For example, you might be doing a slow DNS lookup and if it's
+  // blocked on the OS, you may not want to stop shutdown, since the result
+  // doesn't really matter at that point.
+  //
+  // However, you need to be very careful what you do in your callback when you
+  // use this option. Since the thread will continue to run until the OS
+  // terminates the process, the app can be in the process of tearing down when
+  // you're running. This means any singletons or global objects you use may
+  // suddenly become invalid out from under you. For this reason, it's best to
+  // use this only for slow but simple operations like the DNS example.
+  CONTINUE_ON_SHUTDOWN,
+
+  // Tasks posted with this mode that have not started executing at
+  // shutdown will never run. However, any task that has already begun
+  // executing when shutdown is invoked will be allowed to continue and
+  // will block shutdown until completion.
+  //
+  // Note: Because TaskScheduler::Shutdown() may block while these tasks are
+  // executing, care must be taken to ensure that they do not block on the
+  // thread that called TaskScheduler::Shutdown(), as this may lead to deadlock.
+  SKIP_ON_SHUTDOWN,
+
+  // Tasks posted with this mode before shutdown is complete will block shutdown
+  // until they're executed. Generally, this should be used only to save
+  // critical user data.
+  //
+  // Note: Tasks with BACKGROUND priority that block shutdown will be promoted
+  // to USER_VISIBLE priority during shutdown.
+  BLOCK_SHUTDOWN,
+};
+
+// Describes metadata for a single task or a group of tasks.
+struct BASE_EXPORT TaskTraits {
+  // Constructs a default TaskTraits for tasks with
+  //     (1) no I/O,
+  //     (2) low priority, and
+  //     (3) may block shutdown or be skipped on shutdown.
+  // Tasks that require stricter guarantees should highlight those by requesting
+  // explicit traits below.
+  TaskTraits();
+  ~TaskTraits();
+
+  // Allows tasks with these traits to do file I/O.
+  TaskTraits& WithFileIO();

[jam, 2016/02/17 16:50:08]

i'm curious, why do these WithFoo methods modify the struct instead of returning
a new one?

[robliao, 2016/02/17 19:06:00]

There were a few goals behind this setup:
1) We wanted to it to be easy to specify task traits inline a method call.
2) Adding a new trait should not require a global refactoring of all uses of
TaskTraits.
3) The method of setting a value should avoid unnecessary copying of the
structure.

With the above in mind, we landed on the named parameter idiom of C++:
https://isocpp.org/wiki/faq/ctors#named-parameter-idiom

And it allows us to do this:
   PostTaskWithTraits(FROM_HERE,
TaskTraits().WithPriority(USER_BLOCKING).WithFileIO(), ...)

[jam, 2016/02/17 21:48:26]

sure, I was just wondering about returning another copy. i see you're trying to
reduce copies, so that sounds fine.

+
+  // Applies |priority| to tasks with these traits.
+  TaskTraits& WithPriority(TaskPriority priority);
+
+  // Applies |shutdown_behavior| to tasks with these traits.
+  TaskTraits& WithShutdownBehavior(TaskShutdownBehavior shutdown_behavior);
+
+  // Returns true if file I/O is allowed by these traits.
+  bool with_file_io() const { return with_file_io_; }
+
+  // Returns the priority of tasks with these traits.
+  TaskPriority priority() const { return priority_; }
+
+  // Returns the shutdown behavior of tasks with these traits.
+  TaskShutdownBehavior shutdown_behavior() const { return shutdown_behavior_; }
+
+ private:

[jam, 2016/02/17 16:50:08]

per style guide, if this has private section shouldn't this be a class?

i think it would be simpler if you removed the private and getters.

[robliao, 2016/02/17 19:06:00]

This can likely be a class. Changed.

[gab, 2016/02/17 19:27:55]

I think what jam meant is that per style it should be a class if it has private
members but that he preferred a struct with public members + no getters (I agree
with that -- keeping the WithFoo() methods for chaining practicality but
allowing public member access seems fine).

[robliao, 2016/02/17 20:41:09]

There are two reasons why we think these should remain private:

1) There would be two ways to set values. Consumers would have to choose on
using the named parameter idiom or direct struct setting, introducing
inconsistency into the codebase.
2) This inconsistency would also make it difficult to perform codesearches. For
example, we would have to perform two searches to determine if someone had a
trait with file IO, one for the named parameter idiom and another for the direct
struct reference.

[jam, 2016/02/17 21:48:26]

ok, up to you. in that case this should be a class per style guide

[robliao, 2016/02/17 23:12:56]

Done.

+  bool with_file_io_;
+  TaskPriority priority_;
+  TaskShutdownBehavior shutdown_behavior_;
+};
+
+// Describes how tasks are executed by a task runner.
+enum class ExecutionMode {
+  // Can execute multiple tasks at a time in any order.
+  PARALLEL,
+
+  // Executes one task at a time in posting order. The sequence’s priority is
+  // equivalent to the highest priority pending task in the sequence.
+  SEQUENCED,
+
+  // Executes one task at a time on a single thread in posting order.
+  SINGLE_THREADED,
+};
+
+}  // namespace base
+
+#endif  // BASE_TASK_SCHEDULER_TASK_TRAITS_H_