Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831883003:
Do not inherit TaskPriority in TaskTraits. (Closed)
DescriptionDo not inherit TaskPriority in TaskTraits.
In order to make the TaskTraits constructor constexpr, do not
call GetTaskPriorityForCurrentThread() from it. Instead, keep
track of whether the TaskPriority was set explicitly. When the
TaskTraits are actually used, substitute the TaskPriority with
the return value of GetTaskPriorityForCurrentThread() if it
wasn't set explicitly.
BUG=713683
Review-Url: https://codereview.chromium.org/2831883003
Cr-Commit-Position: refs/heads/master@{#467152}
Committed: https://chromium.googlesource.com/chromium/src/+/02cf16644800b5c644a6067e7077882fefcaf7f1
Patch Set 1 #Patch Set 2 : self-review #
Total comments: 1
Patch Set 3 : self-review #
Total comments: 6
Patch Set 4 : CR-gab-9 #
Total comments: 14
Patch Set 5 : CR-gab-13 #Patch Set 6 : CR-gab-21 #Patch Set 7 : self-review #Patch Set 8 : self-review #
Total comments: 3
Patch Set 9 : CR-gab-34 #
Messages
Total messages: 44 (29 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 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: + gab@chromium.org, robliao@chromium.org
PTAL https://codereview.chromium.org/2831883003/diff/20001/base/task_scheduler/pos... File base/task_scheduler/post_task.cc (right): https://codereview.chromium.org/2831883003/diff/20001/base/task_scheduler/pos... base/task_scheduler/post_task.cc:19: TaskTraits traits_with_explicit_priority(traits); I'm aware that this is not tested. Instead of writing new tests, I'm thinking of changing task_scheduler_impl_unittest.cc to post_task_unittest.cc (i.e. same tests but going through the post_task.h API) and adding TaskPriority::INHERITED to the set of tested TraitsExecutionModePair. WDYT?
Meta-comment below, will stop review at that for now per it affecting the whole CL https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/pos... File base/task_scheduler/post_task.cc (right): https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/pos... base/task_scheduler/post_task.cc:18: TaskTraits SetExplicitPriorityInTaskTraits(const TaskTraits& traits) { This doesn't "set" a priority, it returns a new one based on |traits|. https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/pos... base/task_scheduler/post_task.cc:24: return traits_with_explicit_priority; How about this whole function as: return traits.priority() == TaskPriority::INHERITED ? traits.WithPriority(internal::GetTaskPriorityForCurrentThread()) : traits; this avoids one copy in both cases I think. https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/tes... File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/tes... base/task_scheduler/test_utils.h:20: TaskTraits CreateTaskTraits(); Can we instead hold a const bitfield in TaskTraits of which traits were specified explicitly? post_task.cc can then decide to replace those that weren't with inherited properties where that makes sense. I think that (1) scales better (2) is less intrusive and (3) avoids forcing an undesired "beyond max" value to be handled by switches/etc.
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...
PTAnL Only kept "set explicitly" state for the TaskPriority trait since we wouldn't use it for other traits. https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/pos... File base/task_scheduler/post_task.cc (right): https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/pos... base/task_scheduler/post_task.cc:18: TaskTraits SetExplicitPriorityInTaskTraits(const TaskTraits& traits) { On 2017/04/25 14:19:16, gab wrote: > This doesn't "set" a priority, it returns a new one based on |traits|. Done. https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/pos... base/task_scheduler/post_task.cc:24: return traits_with_explicit_priority; On 2017/04/25 14:19:16, gab wrote: > How about this whole function as: > > return traits.priority() == TaskPriority::INHERITED > ? traits.WithPriority(internal::GetTaskPriorityForCurrentThread()) > : traits; > > this avoids one copy in both cases I think. Done. A copy is required to call WithPriority() (cannot be called on const TaskTraits). https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/tes... File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/2831883003/diff/40001/base/task_scheduler/tes... base/task_scheduler/test_utils.h:20: TaskTraits CreateTaskTraits(); On 2017/04/25 14:19:16, gab wrote: > Can we instead hold a const bitfield in TaskTraits of which traits were > specified explicitly? post_task.cc can then decide to replace those that weren't > with inherited properties where that makes sense. > > I think that (1) scales better (2) is less intrusive and (3) avoids forcing an > undesired "beyond max" value to be handled by switches/etc. Done.
https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/pos... File base/task_scheduler/post_task.cc (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/pos... base/task_scheduler/post_task.cc:36: TaskTraits CreateTaskTraitsWithExplicitPriority(const TaskTraits& traits) { I prefer GetTaskTraitsWithExplicitPriority() since TaskTraits is effectively a POD return, it's not "creating" much IMO. https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:22: priority_set_explicitly_(false), Should be set to true in .WithPriority()? https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:23: priority_(internal::GetTaskPriorityForCurrentThread()), This doesn't match CL desc. https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:24: shutdown_behavior_(TaskShutdownBehavior::SKIP_ON_SHUTDOWN) {} Use inline member initialization in header for all traits https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:157: bool priority_set_explicitly_ : 1; Let's ask Etienne to make sure these bit optimizations are worth it (and not causing undesired churn).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
etienneb@chromium.org changed reviewers: + etienneb@chromium.org
https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:157: bool priority_set_explicitly_ : 1; On 2017/04/25 17:16:41, gab wrote: > Let's ask Etienne to make sure these bit optimizations are worth it (and not > causing undesired churn). Using bool x : 1; bool y : 1; This will use less memory space for every allocated objects. But every access will use more code as the compiler need to emit code to extract the bit. So, this is a tradeoff between code size and memory size.
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...
PTAnL https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/pos... File base/task_scheduler/post_task.cc (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/pos... base/task_scheduler/post_task.cc:36: TaskTraits CreateTaskTraitsWithExplicitPriority(const TaskTraits& traits) { On 2017/04/25 17:16:41, gab wrote: > I prefer GetTaskTraitsWithExplicitPriority() since TaskTraits is effectively a > POD return, it's not "creating" much IMO. Done. https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:22: priority_set_explicitly_(false), On 2017/04/25 17:16:41, gab wrote: > Should be set to true in .WithPriority()? Sorry bad merge. https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:23: priority_(internal::GetTaskPriorityForCurrentThread()), On 2017/04/25 17:16:41, gab wrote: > This doesn't match CL desc. Sorry bad merge. https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:24: shutdown_behavior_(TaskShutdownBehavior::SKIP_ON_SHUTDOWN) {} On 2017/04/25 17:16:41, gab wrote: > Use inline member initialization in header for all traits We have to choose between initialization in header of bitfield. http://stackoverflow.com/questions/29286203/default-values-for-bit-fields-in-c11 I'm leaning towards bitfield since member initialization won't be needed with constexpr constructor (all members will be initialized with a call to a template function in the constructor https://codereview.chromium.org/2829083002/) https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:157: bool priority_set_explicitly_ : 1; On 2017/04/25 17:45:59, etienneb wrote: > On 2017/04/25 17:16:41, gab wrote: > > Let's ask Etienne to make sure these bit optimizations are worth it (and not > > causing undesired churn). > > Using > bool x : 1; > bool y : 1; > > This will use less memory space for every allocated objects. > But every access will use more code as the compiler need to emit code to extract > the bit. > > So, this is a tradeoff between code size and memory size. Since there will be very few accesses (ignoring constexpr constructors) and a few instances in memory at all times, I'd say it's worth it.
https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.cc (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.cc:24: shutdown_behavior_(TaskShutdownBehavior::SKIP_ON_SHUTDOWN) {} On 2017/04/25 17:57:48, fdoray wrote: > On 2017/04/25 17:16:41, gab wrote: > > Use inline member initialization in header for all traits > > We have to choose between initialization in header of bitfield. > http://stackoverflow.com/questions/29286203/default-values-for-bit-fields-in-c11 > > I'm leaning towards bitfield since member initialization won't be needed with > constexpr constructor (all members will be initialized with a call to a template > function in the constructor https://codereview.chromium.org/2829083002/) Fast-access matters more than bit savings IMO (i.e. extra codegen to extract bit is undesired). Member initialization OTOH is about readability IMO and I prefer that.
https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:157: bool priority_set_explicitly_ : 1; On 2017/04/25 17:57:48, fdoray wrote: > On 2017/04/25 17:45:59, etienneb wrote: > > On 2017/04/25 17:16:41, gab wrote: > > > Let's ask Etienne to make sure these bit optimizations are worth it (and not > > > causing undesired churn). > > > > Using > > bool x : 1; > > bool y : 1; > > > > This will use less memory space for every allocated objects. > > But every access will use more code as the compiler need to emit code to > extract > > the bit. > > > > So, this is a tradeoff between code size and memory size. > > Since there will be very few accesses (ignoring constexpr constructors) and a > few instances in memory at all times, I'd say it's worth it. If this is not "largely" used, I'll stay with the simple version. "bool" and not "bool : 1"
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 fdoray@chromium.org
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...
Description was changed from ========== Add TaskPriority::INHERITED. Initialize the |priority_| member of TaskTraits with TaskPriority::INHERITED instead of with the return value of GetTaskPriorityForCurrentThread(). Substitue TaskPriority::INHERITED with the return value of GetTaskPriorityForCurrentThread() when the TaskTraits are sent to a base/task_scheduler/post_task.h function. This is a prerequisite to make the constructor of TaskTraits constexpr. BUG=713683 ========== to ========== Do not inherit TaskPriority in TaskTraits. In order to make the TaskTraits constructor constexpr, do not call GetTaskPriorityForCurrentThread() from it. Instead, keep track of whether the TaskPriority was set explicitly. When the TaskTraits are actually used, substitute the TaskPriority with the return value of GetTaskPriorityForCurrentThread() if it wasn't set explicitly. BUG=713683 ==========
https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2831883003/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_traits.h:157: bool priority_set_explicitly_ : 1; On 2017/04/25 18:05:47, etienneb wrote: > On 2017/04/25 17:57:48, fdoray wrote: > > On 2017/04/25 17:45:59, etienneb wrote: > > > On 2017/04/25 17:16:41, gab wrote: > > > > Let's ask Etienne to make sure these bit optimizations are worth it (and > not > > > > causing undesired churn). > > > > > > Using > > > bool x : 1; > > > bool y : 1; > > > > > > This will use less memory space for every allocated objects. > > > But every access will use more code as the compiler need to emit code to > > extract > > > the bit. > > > > > > So, this is a tradeoff between code size and memory size. > > > > Since there will be very few accesses (ignoring constexpr constructors) and a > > few instances in memory at all times, I'd say it's worth it. > > If this is not "largely" used, I'll stay with the simple version. > "bool" and not "bool : 1" Done.
ptanl
The CQ bit was unchecked by fdoray@chromium.org
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...
lgtm w/ comment https://codereview.chromium.org/2831883003/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2831883003/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:161: TaskPriority priority_ = TaskPriority::BACKGROUND; GetTaskPriorityForCurrentThread() defaults to USER_VISIBLE so this should too.
lgtm https://codereview.chromium.org/2831883003/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2831883003/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:161: TaskPriority priority_ = TaskPriority::BACKGROUND; On 2017/04/25 18:50:42, gab wrote: > GetTaskPriorityForCurrentThread() defaults to USER_VISIBLE so this should too. +1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2831883003/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/2831883003/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_traits.h:161: TaskPriority priority_ = TaskPriority::BACKGROUND; On 2017/04/25 20:06:47, robliao wrote: > On 2017/04/25 18:50:42, gab wrote: > > GetTaskPriorityForCurrentThread() defaults to USER_VISIBLE so this should too. > > +1 Done.
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/2831883003/#ps160001 (title: "CR-gab-34")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493155241816060,
"parent_rev": "7dacd22215a5c3a7670fde93d2b99188131809a4", "commit_rev":
"02cf16644800b5c644a6067e7077882fefcaf7f1"}
Message was sent while issue was closed.
Description was changed from ========== Do not inherit TaskPriority in TaskTraits. In order to make the TaskTraits constructor constexpr, do not call GetTaskPriorityForCurrentThread() from it. Instead, keep track of whether the TaskPriority was set explicitly. When the TaskTraits are actually used, substitute the TaskPriority with the return value of GetTaskPriorityForCurrentThread() if it wasn't set explicitly. BUG=713683 ========== to ========== Do not inherit TaskPriority in TaskTraits. In order to make the TaskTraits constructor constexpr, do not call GetTaskPriorityForCurrentThread() from it. Instead, keep track of whether the TaskPriority was set explicitly. When the TaskTraits are actually used, substitute the TaskPriority with the return value of GetTaskPriorityForCurrentThread() if it wasn't set explicitly. BUG=713683 Review-Url: https://codereview.chromium.org/2831883003 Cr-Commit-Position: refs/heads/master@{#467152} Committed: https://chromium.googlesource.com/chromium/src/+/02cf16644800b5c644a6067e7077... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/02cf16644800b5c644a6067e7077... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
