[task] Introduce inheriting ThreadType TaskTraits [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +1 |
1 comment
Related details
- Francois Pierre Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray added 11 comments![]( "Open in Gerrit")
: thread_type_(This contains the MaxThreadType, and not the actually inherited thread type? Add documentation.
constexpr explicit MaxThreadType(ThreadType max_thread_Type)```suggestion
constexpr explicit MaxThreadType(ThreadType max_thread_type)
```
constexpr TaskTraits traits6 = {TaskShutdownBehavior::BLOCK_SHUTDOWN, true}; // expected-error@*:* {{no matching constructor for initialization}}Add no compile tests which specify the new traits (e.g. cannot use InheritThreadType + explicit TaskPriority).
FROM_HERE, TaskTraits(), ThreadType::kDefault,Add an EXPECT statement to verify that this is plumbed correctly.
GetCurrentTaskImportance());We should have unit tests that exercise this code, i.e. post to a PooledParallelTaskRunner with ThreadType inheritance and verify that this results in running the correct ThreadGroup.
GetCurrentTaskImportance())) {}This is tricky... If the SequencedTaskRunner is created on a high priority thread and them plumbed to a low priority thread, the PostTasks will still be high priority. I suspect that this isn't the desired behavior.
Should we have a CHECK that the CurrentTaskImportance at PostTask time == the creation CurrentTaskImportance, until we figure out something better for this use case? i.e. for now don't allow using ThreadType inheritance and then passing the SequencedTaskRunner to a different ThreadType?
GetCurrentTaskImportance()));We should have unit tests that exercise this code, i.e. post to ThreadPool with ThreadType inheritance and verify that this results in running the correct ThreadGroup.
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 12 comments![]( "Open in Gerrit")
This contains the MaxThreadType, and not the actually inherited thread type? Add documentation.
Done
constexpr explicit MaxThreadType(ThreadType max_thread_Type)```suggestion
constexpr explicit MaxThreadType(ThreadType max_thread_type)
```
Done
constexpr TaskTraits traits6 = {TaskShutdownBehavior::BLOCK_SHUTDOWN, true}; // expected-error@*:* {{no matching constructor for initialization}}Add no compile tests which specify the new traits (e.g. cannot use InheritThreadType + explicit TaskPriority).
Done
FROM_HERE, TaskTraits(), ThreadType::kDefault,Add an EXPECT statement to verify that this is plumbed correctly.
I added a new test with InheritThreadType for this.
We should have unit tests that exercise this code, i.e. post to a PooledParallelTaskRunner with ThreadType inheritance and verify that this results in running the correct ThreadGroup.
Added test through GetTraitsExecutionModePairsToCoverAllSchedulingOptions()
GetCurrentTaskImportance())) {}This is tricky... If the SequencedTaskRunner is created on a high priority thread and them plumbed to a low priority thread, the PostTasks will still be high priority. I suspect that this isn't the desired behavior.
Should we have a CHECK that the CurrentTaskImportance at PostTask time == the creation CurrentTaskImportance, until we figure out something better for this use case? i.e. for now don't allow using ThreadType inheritance and then passing the SequencedTaskRunner to a different ThreadType?
From use cases, I don't think we can expect the task runner to never be used from a lower priority context; but this requires the higher priority task to willingly "share" its task runner.
But I think we can expect it will never be used in a context that would give it higher priority (taking into account the MaxThreadType)
So I added a DCHECK for that.
We should have unit tests that exercise this code, i.e. post to ThreadPool with ThreadType inheritance and verify that this results in running the correct ThreadGroup.
Added tests through GetTraitsExecutionModePairsToCoverAllSchedulingOptions()
Related details
- 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 2 comments![]( "Open in Gerrit")
Votes added by Francois Pierre Doray
| Code-Review | +1 |
2 comments
LGTM, but tests are failing
GetCurrentTaskImportance())) {}Etienne Pierre-DorayThis is tricky... If the SequencedTaskRunner is created on a high priority thread and them plumbed to a low priority thread, the PostTasks will still be high priority. I suspect that this isn't the desired behavior.
Should we have a CHECK that the CurrentTaskImportance at PostTask time == the creation CurrentTaskImportance, until we figure out something better for this use case? i.e. for now don't allow using ThreadType inheritance and then passing the SequencedTaskRunner to a different ThreadType?
From use cases, I don't think we can expect the task runner to never be used from a lower priority context; but this requires the higher priority task to willingly "share" its task runner.
But I think we can expect it will never be used in a context that would give it higher priority (taking into account the MaxThreadType)
So I added a DCHECK for that.
- 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. |
Francois Pierre Doray (Gerrit)
Francois Pierre Doray 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. |