[task] Use ThreadType in thread pool sort key [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 voted and added 8 comments![]( "Open in Gerrit")
Votes added by Francois Pierre Doray
| Code-Review | +1 |
8 comments
LGTM with comments
if (!task_tracker_->WillPostTaskNow(task, sequence->thread_type_racy())) {This uses a method with the name "racy" in it, but it's not actually racy because the lock is held (via `transaction`). This is misleading. It would be preferable to use `transaction.thread_type()` here (which is implemented by calling `thread_type_racy()`, but see my other comment there).
Document what each index represents in a comment.
Or, actually, I think the code would be more self-explanatory if you simply changes this to 2 distinct variables (num_foreground_task_sources_, num_background_task_sources_), and a simple if/else in DecrementNumTaskSourcesForThreadType/IncrementNumTaskSourcesForThreadType.
ThreadType thread_type() const { return task_source_->thread_type_racy(); }This method is not racy, but it calls a method with "racy" in the name. This is confusing - I have to ask myself why it's valid. I would recommend changing this to:
`return TaskPriorityToThreadType(task_source_->traits_.priority())`
Which requires exposing `TaskPriorityToThreadType`, but I think that's fine. It can be a private static method of TaskSource (I think inner classes have access to private members of outer classes?).
auto priority = priority_queue_.PeekSortKey().thread_type();nit: thread_type
task_source->thread_type_racy(), task_source->thread_policy());Get non-racy thread type from the transaction (see my comment in base/task/thread_pool/task_source.h).
void ThreadGroup::HandoffNonUserBlockingTaskSourcesToOtherThreadGroup(Update method name.
// This works because all USER_BLOCKING tasks are at the front of the queue.Update comment.
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 voted and added 7 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +2 |
7 comments
if (!task_tracker_->WillPostTaskNow(task, sequence->thread_type_racy())) {This uses a method with the name "racy" in it, but it's not actually racy because the lock is held (via `transaction`). This is misleading. It would be preferable to use `transaction.thread_type()` here (which is implemented by calling `thread_type_racy()`, but see my other comment there).
Document what each index represents in a comment.
Or, actually, I think the code would be more self-explanatory if you simply changes this to 2 distinct variables (num_foreground_task_sources_, num_background_task_sources_), and a simple if/else in DecrementNumTaskSourcesForThreadType/IncrementNumTaskSourcesForThreadType.
Done
ThreadType thread_type() const { return task_source_->thread_type_racy(); }This method is not racy, but it calls a method with "racy" in the name. This is confusing - I have to ask myself why it's valid. I would recommend changing this to:
`return TaskPriorityToThreadType(task_source_->traits_.priority())`
Which requires exposing `TaskPriorityToThreadType`, but I think that's fine. It can be a private static method of TaskSource (I think inner classes have access to private members of outer classes?).
Done here, but in follow-up this will become a bit more complicated:
thread_type becomes a function of multiple traits and originating thread_type.
auto priority = priority_queue_.PeekSortKey().thread_type();Etienne Pierre-Doraynit: thread_type
Done
task_source->thread_type_racy(), task_source->thread_policy());Get non-racy thread type from the transaction (see my comment in base/task/thread_pool/task_source.h).
Done
void ThreadGroup::HandoffNonUserBlockingTaskSourcesToOtherThreadGroup(Etienne Pierre-DorayUpdate method name.
Done
// This works because all USER_BLOCKING tasks are at the front of the queue.Etienne Pierre-DorayUpdate comment.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: base/task/thread_pool/priority_queue.h
Insertions: 6, Deletions: 8.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/task/thread_pool/task_source.h
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/task/thread_pool/priority_queue.cc
Insertions: 14, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/task/thread_pool/thread_group.h
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/task/thread_pool/thread_pool_impl.cc
Insertions: 2, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/task/thread_pool/task_source.cc
Insertions: 4, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/task/thread_pool/pooled_single_thread_task_runner_manager.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/task/thread_pool/thread_group.cc
Insertions: 12, Deletions: 16.
The diff is too large to show. Please review the diff.
```
Change information
[task] Use ThreadType in thread pool sort key
This CL uses ThreadType to represent task source importance instead of
task priority.
In follow-up, this is necessary to represent importance of sequence
that are posted with ThreadType inheritance, and to create ThreadGroup
whose importance can't be described by the current TaskPriority.
Luckily, all existing TaskPriority can be mapped to ThreadType
so the conversion is not lossy.
- M base/task/post_job.cc
- M base/task/thread_pool/job_task_source.cc
- M base/task/thread_pool/pooled_single_thread_task_runner_manager.cc
- M base/task/thread_pool/priority_queue.cc
- M base/task/thread_pool/priority_queue.h
- M base/task/thread_pool/priority_queue_unittest.cc
- M base/task/thread_pool/sequence.cc
- M base/task/thread_pool/sequence_unittest.cc
- M base/task/thread_pool/task_source.cc
- M base/task/thread_pool/task_source.h
- M base/task/thread_pool/task_source_sort_key.cc
- M base/task/thread_pool/task_source_sort_key.h
- M base/task/thread_pool/task_source_sort_key_unittest.cc
- M base/task/thread_pool/task_tracker.cc
- M base/task/thread_pool/task_tracker.h
- M base/task/thread_pool/thread_group.cc
- M base/task/thread_pool/thread_group.h
- M base/task/thread_pool/thread_group_impl.cc
- M base/task/thread_pool/thread_group_impl_unittest.cc
- M base/task/thread_pool/thread_group_unittest.cc
- M base/task/thread_pool/thread_pool_impl.cc
- M base/task/thread_pool/thread_pool_impl.h
Code-Review: +1 by Francois Pierre Doray
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |