[task] Expose internal::GetCurrentTaskImportance() [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 3 comments![]( "Open in Gerrit")
public:Etienne Pierre-DorayThe comment refers to "this TaskRunner", but this is a static method, so it's not so clear what "this TaskRunner" means. It's definitely not the TaskRunner on which this is called in:
`task_runner->GetCurrentThreadType()`
To reduce confusion, I suggest making this a standalone function. Depending on where you plan to use this, it could be in the anonymous namespace?
Done, moved to internal:: in base/task/thread_type.h
base::ThreadType ToThreadType(TaskPriority priority) {Etienne Pierre-DorayHi, sorry for the drive-by, but I have a question about this: how will this be used in the renderer, and is this the best fit for thread type (vs. UseCase)?
We use priority in blink a lot for relative ordering of various things, and I don't know how well that really matches thread priority. For example, during loading we:
- Run image loading tasks at extremely high priority so it happens before the next paint (i.e. this is coordinated with rendering priority), which helps prevent layout shift (by a good amount IIRC) and has a small LCP benefit (~1 frame max improvement).
- We periodically bump up rendering priority (to very high) to balance loading tasks and rendering.
- We also use very high priority internally for script continuation, e.g. to break up executing module scripts during loading, where we want input and maybe rendering to run, but not other things.In all cases I don't know that the elevated priority implies a difference in thread priority compared to the other loading tasks (normal priority)? Some other recent features, e.g. [1], are using UseCase to determine drive performance settings, so I'm wondering if we should align this (assuming this will drive thread priority).
Hi, sorry for the drive-by, but I have a question about this: how will this be used in the renderer
To be clear, this won't be used to drive thread's OS priority. What this will be used for:
- (short term) The immediate use case is to allow media tasks to post to ThreadPool from task queues > ThreadType::kDefault.
- (short term) better identify "background" task queue (for disabling), the current impl simply pick the first one with the lowest priority
- (short term) replace ScopedSetTaskPriorityForCurrentThread that works awkwardly for non-threadpool context.
- (long term) prevent tasks from implicitly escalating priority when posted to the ThreadType, and inherit some kind of originating work importance when requested.
is this the best fit for thread type (vs. UseCase)
In this context, ThreadType is use to describe "work importance" in a scheduler agnostic (and rather coarse) way, but I didn't want to depend on renaming ThreadType, nor introducing yet another concept of priority in base.
I want this to work outside of renderers or any particular scheduler though.
I guess another question is: is it a better fit to derive ThreadType from UseCase rather than derive from TaskPriority. (I think TaskPriority is more appropriate)
Related details
- Francois Pierre Doray
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
base::ThreadType ToThreadType(TaskPriority priority) {Etienne Pierre-DorayHi, sorry for the drive-by, but I have a question about this: how will this be used in the renderer, and is this the best fit for thread type (vs. UseCase)?
We use priority in blink a lot for relative ordering of various things, and I don't know how well that really matches thread priority. For example, during loading we:
- Run image loading tasks at extremely high priority so it happens before the next paint (i.e. this is coordinated with rendering priority), which helps prevent layout shift (by a good amount IIRC) and has a small LCP benefit (~1 frame max improvement).
- We periodically bump up rendering priority (to very high) to balance loading tasks and rendering.
- We also use very high priority internally for script continuation, e.g. to break up executing module scripts during loading, where we want input and maybe rendering to run, but not other things.In all cases I don't know that the elevated priority implies a difference in thread priority compared to the other loading tasks (normal priority)? Some other recent features, e.g. [1], are using UseCase to determine drive performance settings, so I'm wondering if we should align this (assuming this will drive thread priority).
Hi, sorry for the drive-by, but I have a question about this: how will this be used in the renderer
To be clear, this won't be used to drive thread's OS priority. What this will be used for:
- (short term) The immediate use case is to allow media tasks to post to ThreadPool from task queues > ThreadType::kDefault.
- (short term) better identify "background" task queue (for disabling), the current impl simply pick the first one with the lowest priority
- (short term) replace ScopedSetTaskPriorityForCurrentThread that works awkwardly for non-threadpool context.
- (long term) prevent tasks from implicitly escalating priority when posted to the ThreadType, and inherit some kind of originating work importance when requested.
is this the best fit for thread type (vs. UseCase)In this context, ThreadType is use to describe "work importance" in a scheduler agnostic (and rather coarse) way, but I didn't want to depend on renaming ThreadType, nor introducing yet another concept of priority in base.
I want this to work outside of renderers or any particular scheduler though.
I guess another question is: is it a better fit to derive ThreadType from UseCase rather than derive from TaskPriority. (I think TaskPriority is more appropriate)
Got it, thanks for the context!
In this context, ThreadType is use to describe "work importance" in a scheduler agnostic (and rather coarse) way, but I didn't want to depend on renaming ThreadType, nor introducing yet another concept of priority in base.
Ah, I see. I understand not wanting to introduce another priority concept -- there are already a bunch of overloaded things. Case in point: blink::ThreadType vs base::ThreadType! I think that's partially where my confusion is coming from, as I think of thread type more as an (usually) immutable property of the thread, which has little if any correlation to how work on that thread is arranged. I guess something like (Task)PriorityGroup also matches the intent here?
I want this to work outside of renderers or any particular scheduler though.
I guess another question is: is it a better fit to derive ThreadType from UseCase rather than derive from TaskPriority. (I think TaskPriority is more appropriate)
From the use cases you're describing, yes, TaskPriority is more appropriate -- although it's hard to tell if this is the correct mapping without seeing if and how it will affect the main/worker threads. For example, requestIdleCallback() can escalate priority by calling setTimeout() or a number of other APIs, so I don't think this works in the 4th case (if applied globally on the main thread). You could also consider not creating a mapping here if it isn't needed (yet?) for the main/web-worker threads?
Related details
- Etienne Pierre-Doray
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray voted and added 11 comments![]( "Open in Gerrit")
Votes added by Francois Pierre Doray
| Code-Review | +1 |
LGTM with comments
ThreadTypeMapping thread_type_mapping_ = &DefaultTaskPriorityToThreadType;This is always overridden I believe, see https://chromium-review.googlesource.com/c/chromium/src/+/7521684/23/base/task/sequence_manager/sequence_manager.cc#39?
Set the initial value to `nullptr`, so it's more straightforward what code path sets this, and avoid having different code paths that evolve differently in the future.
Add a comment explaining that this returns the ThreadType corresponding to `priority`, using the mapping provided via SetThreadTypeMapping if applicable.
Reason: Improve readability. The reader can understand the method without reading the implementation.
// will be returned by SingleThreadTaskRunner::GetCurrentThreadType().Update this comment.
Move this to line 470.
Reason: There is code that posts tasks in unexpected places (for example on ChromeOS there is code that posts a task the first time the state of a feature is accessed). We don't want to modify the task importance for tasks posted outside of the callback (e.g. in SequenceManager code), so we should set the CurrentTaskImportance for the narrowest scope possible.
// after doing so.Document the `thread_type` argument.
// task. This will be equal or lower than the current thread's ThreadType```suggestion
// task. This will be equal to or lower than the current thread's ThreadType
```
switch (priority) {Same comment as https://chromium-review.googlesource.com/c/chromium/src/+/7521684/23/services/network/public/cpp/network_service_task_priority.cc: it's possible to group `case` statements to make it easier to identify TaskPriorities that map to the same ThreadType.
case NetworkServiceTaskPriority::kLowestPriority:
return base::ThreadType::kBackground;
case NetworkServiceTaskPriority::kIdlePriority:
return base::ThreadType::kBackground;
case NetworkServiceTaskPriority::kThrottledPriority:
return base::ThreadType::kBackground;Optional nit:
Can be rewritten like this for clarity:
```suggestion
case NetworkServiceTaskPriority::kLowestPriority:
case NetworkServiceTaskPriority::kIdlePriority:
case NetworkServiceTaskPriority::kThrottledPriority:
return base::ThreadType::kBackground;
```
I find that it makes it easier to realize that multiple NetworkServiceTaskPriority map to the same ThreadType.
base::ThreadType ToThreadType(TaskPriority priority) {Etienne Pierre-DorayHi, sorry for the drive-by, but I have a question about this: how will this be used in the renderer, and is this the best fit for thread type (vs. UseCase)?
We use priority in blink a lot for relative ordering of various things, and I don't know how well that really matches thread priority. For example, during loading we:
- Run image loading tasks at extremely high priority so it happens before the next paint (i.e. this is coordinated with rendering priority), which helps prevent layout shift (by a good amount IIRC) and has a small LCP benefit (~1 frame max improvement).
- We periodically bump up rendering priority (to very high) to balance loading tasks and rendering.
- We also use very high priority internally for script continuation, e.g. to break up executing module scripts during loading, where we want input and maybe rendering to run, but not other things.In all cases I don't know that the elevated priority implies a difference in thread priority compared to the other loading tasks (normal priority)? Some other recent features, e.g. [1], are using UseCase to determine drive performance settings, so I'm wondering if we should align this (assuming this will drive thread priority).
Hi, sorry for the drive-by, but I have a question about this: how will this be used in the renderer
To be clear, this won't be used to drive thread's OS priority. What this will be used for:
- (short term) The immediate use case is to allow media tasks to post to ThreadPool from task queues > ThreadType::kDefault.
- (short term) better identify "background" task queue (for disabling), the current impl simply pick the first one with the lowest priority
- (short term) replace ScopedSetTaskPriorityForCurrentThread that works awkwardly for non-threadpool context.
- (long term) prevent tasks from implicitly escalating priority when posted to the ThreadType, and inherit some kind of originating work importance when requested.
is this the best fit for thread type (vs. UseCase)In this context, ThreadType is use to describe "work importance" in a scheduler agnostic (and rather coarse) way, but I didn't want to depend on renaming ThreadType, nor introducing yet another concept of priority in base.
I want this to work outside of renderers or any particular scheduler though.
I guess another question is: is it a better fit to derive ThreadType from UseCase rather than derive from TaskPriority. (I think TaskPriority is more appropriate)
+1 to EtienneP's response. I think we could clarify on the comment for GetCurrentTaskImportance() that this is used to influence the priority of tasks posted from the current task, but not to influence Thread Priority of the current task.
switch (priority) {Same comment as https://chromium-review.googlesource.com/c/chromium/src/+/7521684/23/services/network/public/cpp/network_service_task_priority.cc: it's possible to group `case` statements to make it easier to identify TaskPriorities that map to the same ThreadType.
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 13 comments![]( "Open in Gerrit")
+bashi@ for services/network
ThreadTypeMapping thread_type_mapping_ = &DefaultTaskPriorityToThreadType;This is always overridden I believe, see https://chromium-review.googlesource.com/c/chromium/src/+/7521684/23/base/task/sequence_manager/sequence_manager.cc#39?
Set the initial value to `nullptr`, so it's more straightforward what code path sets this, and avoid having different code paths that evolve differently in the future.
There are many PrioritySettings in tests that don't go through PrioritySettings::CreateDefault(), so this turned out to be easier.
I removed the redundant SetThreadTypeMapping() in CreateDefault() though.
Add a comment explaining that this returns the ThreadType corresponding to `priority`, using the mapping provided via SetThreadTypeMapping if applicable.
Reason: Improve readability. The reader can understand the method without reading the implementation.
Done
// will be returned by SingleThreadTaskRunner::GetCurrentThreadType().Etienne Pierre-DorayUpdate this comment.
Done
// will be returned by SingleThreadTaskRunner::GetCurrentThreadType().Etienne Pierre-DorayUpdate this comment.
Done
Move this to line 470.
Reason: There is code that posts tasks in unexpected places (for example on ChromeOS there is code that posts a task the first time the state of a feature is accessed). We don't want to modify the task importance for tasks posted outside of the callback (e.g. in SequenceManager code), so we should set the CurrentTaskImportance for the narrowest scope possible.
Done
// after doing so.Etienne Pierre-DorayDocument the `thread_type` argument.
Done
// after doing so.Etienne Pierre-DorayDocument the `thread_type` argument.
Done
// task. This will be equal or lower than the current thread's ThreadType```suggestion
// task. This will be equal to or lower than the current thread's ThreadType
```
Done
switch (priority) {Same comment as https://chromium-review.googlesource.com/c/chromium/src/+/7521684/23/services/network/public/cpp/network_service_task_priority.cc: it's possible to group `case` statements to make it easier to identify TaskPriorities that map to the same ThreadType.
Done
case NetworkServiceTaskPriority::kLowestPriority:
return base::ThreadType::kBackground;
case NetworkServiceTaskPriority::kIdlePriority:
return base::ThreadType::kBackground;
case NetworkServiceTaskPriority::kThrottledPriority:
return base::ThreadType::kBackground;Optional nit:
Can be rewritten like this for clarity:
```suggestion
case NetworkServiceTaskPriority::kLowestPriority:
case NetworkServiceTaskPriority::kIdlePriority:
case NetworkServiceTaskPriority::kThrottledPriority:
return base::ThreadType::kBackground;
```I find that it makes it easier to realize that multiple NetworkServiceTaskPriority map to the same ThreadType.
Done
base::ThreadType ToThreadType(TaskPriority priority) {Etienne Pierre-DorayHi, sorry for the drive-by, but I have a question about this: how will this be used in the renderer, and is this the best fit for thread type (vs. UseCase)?
We use priority in blink a lot for relative ordering of various things, and I don't know how well that really matches thread priority. For example, during loading we:
- Run image loading tasks at extremely high priority so it happens before the next paint (i.e. this is coordinated with rendering priority), which helps prevent layout shift (by a good amount IIRC) and has a small LCP benefit (~1 frame max improvement).
- We periodically bump up rendering priority (to very high) to balance loading tasks and rendering.
- We also use very high priority internally for script continuation, e.g. to break up executing module scripts during loading, where we want input and maybe rendering to run, but not other things.In all cases I don't know that the elevated priority implies a difference in thread priority compared to the other loading tasks (normal priority)? Some other recent features, e.g. [1], are using UseCase to determine drive performance settings, so I'm wondering if we should align this (assuming this will drive thread priority).
Francois Pierre DorayHi, sorry for the drive-by, but I have a question about this: how will this be used in the renderer
To be clear, this won't be used to drive thread's OS priority. What this will be used for:
- (short term) The immediate use case is to allow media tasks to post to ThreadPool from task queues > ThreadType::kDefault.
- (short term) better identify "background" task queue (for disabling), the current impl simply pick the first one with the lowest priority
- (short term) replace ScopedSetTaskPriorityForCurrentThread that works awkwardly for non-threadpool context.
- (long term) prevent tasks from implicitly escalating priority when posted to the ThreadType, and inherit some kind of originating work importance when requested.
is this the best fit for thread type (vs. UseCase)In this context, ThreadType is use to describe "work importance" in a scheduler agnostic (and rather coarse) way, but I didn't want to depend on renaming ThreadType, nor introducing yet another concept of priority in base.
I want this to work outside of renderers or any particular scheduler though.
I guess another question is: is it a better fit to derive ThreadType from UseCase rather than derive from TaskPriority. (I think TaskPriority is more appropriate)
+1 to EtienneP's response. I think we could clarify on the comment for GetCurrentTaskImportance() that this is used to influence the priority of tasks posted from the current task, but not to influence Thread Priority of the current task.
Done
switch (priority) {Same comment as https://chromium-review.googlesource.com/c/chromium/src/+/7521684/23/services/network/public/cpp/network_service_task_priority.cc: it's possible to group `case` statements to make it easier to identify TaskPriorities that map to the same ThreadType.
- Kenichi Ishibashi
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
1 comment
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hayato Ito (Gerrit)
Hayato Ito voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |