Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1702813002:
Task Scheduler [1/8] Task Traits (Closed)
DescriptionTask Scheduler Task Traits
This change is a subset of https://codereview.chromium.org/1698183005/
BUG=553459
Committed: https://crrev.com/df7c0802a46e34a48c5497a9133dddd25b94a14f
Cr-Commit-Position: refs/heads/master@{#376063}
Patch Set 1 #
Total comments: 17
Patch Set 2 : CR Feedback #Patch Set 3 : Update #Patch Set 4 : Update #
Dependent Patchsets: Messages
Total messages: 24 (11 generated)
Description was changed from ========== Chrome Task Scheduler Task Traits Adds Task Traits documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== Chrome Task Scheduler Task Traits Adds Task Traits documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
Original Code Review: https://codereview.chromium.org/1706533002/ This Code Review updates the Base URL.
On 2016/02/16 23:38:08, robliao wrote: > Original Code Review: https://codereview.chromium.org/1706533002/ > > This Code Review updates the Base URL. lgtm rubberstamp based on other review, thanks
robliao@chromium.org changed reviewers: + jam@chromium.org
jam: Here's the first in our sequence of Task Scheduler CLs. Please take a look. Thanks! Special note on the OWNERs file: We intend for the entire implementation to go through you for review. The OWNERs file will serve as an immediate reference for anyone looking at the code. If you would like to pursue a different solution, feel free to let us know.
https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:19: USER_BLOCKING = 2, 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 https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:84: TaskTraits& WithFileIO(); i'm curious, why do these WithFoo methods modify the struct instead of returning a new one? https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:101: private: 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.
https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:19: USER_BLOCKING = 2, On 2016/02/17 16:50:08, jam wrote: > 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 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 ? https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:84: TaskTraits& WithFileIO(); On 2016/02/17 16:50:08, jam wrote: > i'm curious, why do these WithFoo methods modify the struct instead of returning > a new one? 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(), ...) https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:101: private: On 2016/02/17 16:50:08, jam wrote: > 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. This can likely be a class. Changed.
https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:15: // Valid priorities supported by the task scheduler. 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. https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:19: USER_BLOCKING = 2, On 2016/02/17 19:06:00, robliao wrote: > On 2016/02/17 16:50:08, jam wrote: > > 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 > > 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 > ? 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. https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:101: private: On 2016/02/17 19:06:00, robliao wrote: > On 2016/02/17 16:50:08, jam wrote: > > 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. > > This can likely be a class. Changed. 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).
Description was changed from ========== Chrome Task Scheduler Task Traits Adds Task Traits documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== Task Scheduler Task Traits This change is a subset of https://codereview.chromium.org/1698183005/ As documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... BUG=553459 ==========
Description was changed from ========== Task Scheduler Task Traits This change is a subset of https://codereview.chromium.org/1698183005/ As documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... BUG=553459 ========== to ========== Task Scheduler Task Traits This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:15: // Valid priorities supported by the task scheduler. On 2016/02/17 19:27:55, gab wrote: > 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. Done. https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:19: USER_BLOCKING = 2, On 2016/02/17 19:27:55, gab wrote: > On 2016/02/17 19:06:00, robliao wrote: > > On 2016/02/17 16:50:08, jam wrote: > > > 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 > > > > 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 > > ? > > 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. 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. https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:101: private: On 2016/02/17 19:27:55, gab wrote: > On 2016/02/17 19:06:00, robliao wrote: > > On 2016/02/17 16:50:08, jam wrote: > > > 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. > > > > This can likely be a class. Changed. > > 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). 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.
https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:19: USER_BLOCKING = 2, On 2016/02/17 20:41:09, robliao wrote: > On 2016/02/17 19:27:55, gab wrote: > > On 2016/02/17 19:06:00, robliao wrote: > > > On 2016/02/17 16:50:08, jam wrote: > > > > 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 > > > > > > 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 > > > ? > > > > 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. > > 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. 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 https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:84: TaskTraits& WithFileIO(); On 2016/02/17 19:06:00, robliao wrote: > On 2016/02/17 16:50:08, jam wrote: > > i'm curious, why do these WithFoo methods modify the struct instead of > returning > > a new one? > > 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(), ...) sure, I was just wondering about returning another copy. i see you're trying to reduce copies, so that sounds fine. https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:101: private: On 2016/02/17 20:41:09, robliao wrote: > On 2016/02/17 19:27:55, gab wrote: > > On 2016/02/17 19:06:00, robliao wrote: > > > On 2016/02/17 16:50:08, jam wrote: > > > > 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. > > > > > > This can likely be a class. Changed. > > > > 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). > > 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. ok, up to you. in that case this should be a class per style guide
btw in case it's not clear, I'm wondering if we should add static_assert to ensure that the priority is in the right order that the implementation depends on? i defer to you though, so lgtm in the meantime
Patchset #4 (id:30006) has been deleted
Yup, I was in the middle of adding the static_assert, which is now available. https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:19: USER_BLOCKING = 2, On 2016/02/17 21:48:26, jam wrote: > On 2016/02/17 20:41:09, robliao wrote: > > On 2016/02/17 19:27:55, gab wrote: > > > On 2016/02/17 19:06:00, robliao wrote: > > > > On 2016/02/17 16:50:08, jam wrote: > > > > > 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 > > > > > > > > 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 > > > > ? > > > > > > 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. > > > > 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. > > 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 Done. Added a static_assert. https://codereview.chromium.org/1702813002/diff/1/base/task_scheduler/task_tr... base/task_scheduler/task_traits.h:101: private: On 2016/02/17 21:48:26, jam wrote: > On 2016/02/17 20:41:09, robliao wrote: > > On 2016/02/17 19:27:55, gab wrote: > > > On 2016/02/17 19:06:00, robliao wrote: > > > > On 2016/02/17 16:50:08, jam wrote: > > > > > 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. > > > > > > > > This can likely be a class. Changed. > > > > > > 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). > > > > 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. > > ok, up to you. in that case this should be a class per style guide Done.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1702813002/#ps50006 (title: "Update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1702813002/50006 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1702813002/50006
Message was sent while issue was closed.
Description was changed from ========== Task Scheduler Task Traits This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== Task Scheduler Task Traits This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50006)
Message was sent while issue was closed.
Description was changed from ========== Task Scheduler Task Traits This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== Task Scheduler Task Traits This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 Committed: https://crrev.com/df7c0802a46e34a48c5497a9133dddd25b94a14f Cr-Commit-Position: refs/heads/master@{#376063} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/df7c0802a46e34a48c5497a9133dddd25b94a14f Cr-Commit-Position: refs/heads/master@{#376063} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
