PlatformThread: implement a lease system for thread types [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 3 comments![]( "Open in Gerrit")
std::ostream& operator<<(std::ostream& os, ThreadType type) {operator<< is only for tests? If so let's move this function to platform_thread_unittest.cc.
I think it will be useful to have ThreadTypeToString() here though (that operator<< can reuse), because I will need this for thread pool changes.
bool may_change_affinity = inhibit_affinity_count_ == 0;If find this quite confusing: what if there's
- kPresentation with may_change_affinity = false
- kAudioProcessing with may_change_affinity = true
Then we wouldn't change affinity but the intention was probably to change it.
Since may_change_affinity is only used for ScopedBoostPriority, I'm trying something here: https://chromium-review.googlesource.com/c/chromium/src/+/7539489
That will make ScopedBoostPriority/ScopedBoostablePriority quite similar, and we should just forbid creating a lease under a ScopedBoostPriority.
virtual void DonateThreadTypeLease(Discussed in chat, it might be possible to just have MTSI create its own lease.
Related details
- Markus Handell
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. |
Markus Handell (Gerrit)
Markus Handell added 2 comments![]( "Open in Gerrit")
std::ostream& operator<<(std::ostream& os, ThreadType type) {operator<< is only for tests? If so let's move this function to platform_thread_unittest.cc.
I think it will be useful to have ThreadTypeToString() here though (that operator<< can reuse), because I will need this for thread pool changes.
Done
bool may_change_affinity = inhibit_affinity_count_ == 0;If find this quite confusing: what if there's
- kPresentation with may_change_affinity = false
- kAudioProcessing with may_change_affinity = true
Then we wouldn't change affinity but the intention was probably to change it.
Since may_change_affinity is only used for ScopedBoostPriority, I'm trying something here: https://chromium-review.googlesource.com/c/chromium/src/+/7539489
That will make ScopedBoostPriority/ScopedBoostablePriority quite similar, and we should just forbid creating a lease under a ScopedBoostPriority.
Yeah I agree affinity is a strange beast to keep with thread type. What's implemented here is just that if any lease requires non-affinity-changing, then affinity will be inhibited.
So LMK when you've landed the other CL then?
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. |
Markus Handell (Gerrit)
Markus Handell added 1 comment![]( "Open in Gerrit")
Discussed in chat, it might be possible to just have MTSI create its own lease.
Done
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 3 comments![]( "Open in Gerrit")
void (*set_current_thread_type_impl_)(ThreadType, MessagePumpType, bool);Nit: replacing this by a virtual SetCurrentThreadType() method seems like intrusive as a way to have test mocks.
std::optional<ThreadType> thread_type() const {It's not immediately clear why this is an optional. Looking at the impl it looks like the becomes nullopt if the object is std::move().
I couldn't find any usage though, so maybe we just remove it?
Or we could also disallow move constructor.
bool may_change_affinity = inhibit_affinity_count_ == 0;Markus HandellIf find this quite confusing: what if there's
- kPresentation with may_change_affinity = false
- kAudioProcessing with may_change_affinity = true
Then we wouldn't change affinity but the intention was probably to change it.
Since may_change_affinity is only used for ScopedBoostPriority, I'm trying something here: https://chromium-review.googlesource.com/c/chromium/src/+/7539489
That will make ScopedBoostPriority/ScopedBoostablePriority quite similar, and we should just forbid creating a lease under a ScopedBoostPriority.
Yeah I agree affinity is a strange beast to keep with thread type. What's implemented here is just that if any lease requires non-affinity-changing, then affinity will be inhibited.
So LMK when you've landed the other CL then?
https://chromium-review.googlesource.com/c/chromium/src/+/7539489 landed
So we should be able to remove may_change_affinity
Related details
- Markus Handell
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 1 comment![]( "Open in Gerrit")
return num_scoped_boostable_priority_instances > 0;I landed a CL that keeps track of this as a linked list
https://chromium-review.googlesource.com/c/chromium/src/+/7545022
So this can be replaced by `current_boost_scope`
Nit: This could become `ScopedBoostPriorityBase::CurrentThreadHasScope()`
Markus Handell (Gerrit)
Markus Handell added 4 comments![]( "Open in Gerrit")
void (*set_current_thread_type_impl_)(ThreadType, MessagePumpType, bool);Nit: replacing this by a virtual SetCurrentThreadType() method seems like intrusive as a way to have test mocks.
Replaced with virtual method at the expense of slightly more complex usage due to not trivially destructible anymore.
std::optional<ThreadType> thread_type() const {It's not immediately clear why this is an optional. Looking at the impl it looks like the becomes nullopt if the object is std::move().
I couldn't find any usage though, so maybe we just remove it?
Or we could also disallow move constructor.
It's used in the follow-up in the chain. Yes correct, used to disable moved objects. The follow-up CL doesn't make use of move leases anymore so I removed movability and am relaxing tests a bit.
bool may_change_affinity = inhibit_affinity_count_ == 0;Markus HandellIf find this quite confusing: what if there's
- kPresentation with may_change_affinity = false
- kAudioProcessing with may_change_affinity = true
Then we wouldn't change affinity but the intention was probably to change it.
Since may_change_affinity is only used for ScopedBoostPriority, I'm trying something here: https://chromium-review.googlesource.com/c/chromium/src/+/7539489
That will make ScopedBoostPriority/ScopedBoostablePriority quite similar, and we should just forbid creating a lease under a ScopedBoostPriority.
Etienne Pierre-DorayYeah I agree affinity is a strange beast to keep with thread type. What's implemented here is just that if any lease requires non-affinity-changing, then affinity will be inhibited.
So LMK when you've landed the other CL then?
https://chromium-review.googlesource.com/c/chromium/src/+/7539489 landed
So we should be able to remove may_change_affinity
Done
return num_scoped_boostable_priority_instances > 0;I landed a CL that keeps track of this as a linked list
https://chromium-review.googlesource.com/c/chromium/src/+/7545022
So this can be replaced by `current_boost_scope`
Nit: This could become `ScopedBoostPriorityBase::CurrentThreadHasScope()`
- 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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 7 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
7 comments
LGTM % comment
I'm wondering about this call: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.cc;l=807;drc=e63596721df61bbc199c38c4a102597ad81ad154;bpv=1;bpt=1
Landing this will effectively cancel the effect of SchedulingPolicy::Feature::kWebRTC AFAICT. This is desirable long term but I don't know if you intend to roll out this way.
std::optional<ThreadType> effective_;Nit: effective_thread_type_
std::optional<ThreadType> base_;Nit: default_thread_type_
void SetCurrent(ThreadType type);Nit: SetDefault()
// The `may_change_affinity` parameter determines whether this call can
// change the thread CPU affinity on platforms where it is available. It
// should only be used in cases where e.g. a temporary thread boost should
// not change placement.Nit: Remove stale comment.
void PlatformThreadBase::SetCurrentThreadType(ThreadType thread_type) {Since GetCurrentThreadType() returns something different than this, I would rename SetDefaultThreadType() for less confusion. But this can be a follow-up that I do; maybe simply add a TODO at the declaration.
DCHECK(!(ScopedBoostablePriority::CurrentThreadHasScope() &&
highest_lease.has_value()));Can we do `DCHECK(!ScopedBoostablePriority::CurrentThreadHasScope())` in RaiseThreadTypeLease's constructor?
That seems simpler than MaybeUpdate() that's called in other places.
Related details
- Markus Handell
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. |
Markus Handell (Gerrit)
Markus Handell added 8 comments![]( "Open in Gerrit")
LGTM % comment
I'm wondering about this call: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.cc;l=807;drc=e63596721df61bbc199c38c4a102597ad81ad154;bpv=1;bpt=1Landing this will effectively cancel the effect of SchedulingPolicy::Feature::kWebRTC AFAICT. This is desirable long term but I don't know if you intend to roll out this way.
Yep that was unintended, thanks for noticing. This should be fixed in the new PS.
std::optional<ThreadType> effective_;Markus HandellNit: effective_thread_type_
Done
std::optional<ThreadType> base_;Markus HandellNit: default_thread_type_
Done
void SetCurrent(ThreadType type);Markus HandellNit: SetDefault()
Done
// The `may_change_affinity` parameter determines whether this call can
// change the thread CPU affinity on platforms where it is available. It
// should only be used in cases where e.g. a temporary thread boost should
// not change placement.Markus HandellNit: Remove stale comment.
Done
void PlatformThreadBase::SetCurrentThreadType(ThreadType thread_type) {Since GetCurrentThreadType() returns something different than this, I would rename SetDefaultThreadType() for less confusion. But this can be a follow-up that I do; maybe simply add a TODO at the declaration.
Done
void PlatformThreadBase::SetCurrentThreadType(ThreadType thread_type) {Since GetCurrentThreadType() returns something different than this, I would rename SetDefaultThreadType() for less confusion. But this can be a follow-up that I do; maybe simply add a TODO at the declaration.
Added a TODO; maybe we should consider holding off on updating every caller after some grace time in prod in the interest of any rollbacks?
DCHECK(!(ScopedBoostablePriority::CurrentThreadHasScope() &&
highest_lease.has_value()));Can we do `DCHECK(!ScopedBoostablePriority::CurrentThreadHasScope())` in RaiseThreadTypeLease's constructor?
That seems simpler than MaybeUpdate() that's called in other places.
Done, and updated ScopedBoostPriorityBase to check for leases as well.
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 2 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
LGTM
DCHECK(!PlatformThread::CurrentThreadHasLeases());It's fine to create a ScopedBoostPriority within a lease (and I think we sometimes will), so I don't think we want this check.
But they must strictly nest, so I think we want to do `DCHECK(!ScopedBoostablePriority::CurrentThreadHasScope())` inside of ~RaiseThreadTypeLease
Related details
- Markus Handell
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. |
Markus Handell (Gerrit)
Markus Handell added 1 comment![]( "Open in Gerrit")
DCHECK(!PlatformThread::CurrentThreadHasLeases());It's fine to create a ScopedBoostPriority within a lease (and I think we sometimes will), so I don't think we want this check.
But they must strictly nest, so I think we want to do `DCHECK(!ScopedBoostablePriority::CurrentThreadHasScope())` inside of ~RaiseThreadTypeLease
- Daniel Cheng
- 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. |
Markus Handell (Gerrit)
Markus Handell added 1 comment![]( "Open in Gerrit")
dcheng for renderer_main.cc, fdoray for the rest
Related details
- Daniel Cheng
- 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 Code-Review+1![]( "Open in Gerrit")
- Daniel Cheng
- Markus Handell
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 11 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
11 comments
LGTM w/nits
Overall, I do think the interaction between `ScopedPriorityBoost` and `RaiseThreadTypeLease`; I get the impression that they don't serve exactly the same purpose, but it's not also not 100% clear to me:
- when one is appropriate vs the other
- how they interact
- So improving the documentation here would also be good for a followup
// they're the first type set on the thread.At least to me, this comment doesn't really help me understand why this is OK only if they're the first type set on the thread. I feel like there are some other assumptions about the system that should probably be mentioned here.
(Maybe it'll be less confusing once the current renames above are done?)
uint32_t bitmask = 0;These two fields need some comments.
0};I would omit this, because what this actually means is "explicitly initialize the first element to zero and then initialize the remaining elements from an empty initializer list".
See https://en.cppreference.com/w/cpp/language/list_initialization.html and https://en.cppreference.com/w/cpp/language/aggregate_initialization.html. In the latter, for "Implicitly initialized elements":
Otherwise, if the element is not a reference, the element is copy-initialized from an empty initializer list.
```suggestion
};
```
class BASE_EXPORT ThreadTypeManager {This class should have a comment of some sort that describes it's API, even if it's in `internal`.
if (CurrentIOThread::IsSet()) {I think this is pre-existing, but perhaps it'd be nice to come back here and clarify in a comment if both these bits can be set, and if so, why IO thread takes precedence.
// The lease system is currently not compatible with ScopedBoostablePriority.Is there a crbug to fix this? Or are they fundamentally incompatible? It's unclear if this should be a TODO with a crbug or not.
bitmask |= (1u << type);Maybe a TODO to use `base::EnumSet` here would be appropriate.
std::ostream& operator<<(std::ostream& os, ThreadType type) {If you feel we need these (I thought googletest has some default printing for enums, even if it's not as helpful as possible), they need to be defined alongside where the type is defined to avoid ODR violations.
// used at the same time.Same question about whether or not a TODO is appropriate here.
Related details
- Markus Handell
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. |
Markus Handell (Gerrit)
Markus Handell added 11 comments![]( "Open in Gerrit")
LGTM w/nits
Overall, I do think the interaction between `ScopedPriorityBoost` and `RaiseThreadTypeLease`; I get the impression that they don't serve exactly the same purpose, but it's not also not 100% clear to me:
- when one is appropriate vs the other
- how they interact
- So improving the documentation here would also be good for a followup
Ack, @etie...@chromium.org perhaps let's discuss this on newly filed https://g-issues.chromium.org/issues/483622914?
}Markus HandellNit:
```suggestion
}
NOTREACHED();
```
Done
// they're the first type set on the thread.At least to me, this comment doesn't really help me understand why this is OK only if they're the first type set on the thread. I feel like there are some other assumptions about the system that should probably be mentioned here.
(Maybe it'll be less confusing once the current renames above are done?)
Agree the comment isn't good enough. It's ok to create leases when it's non-nullopt too, it's just that the lease may not be effective (consider a kDefault base priority, and kBackground lease - the lease will not change priority). We need the optional to discriminate between the "thread type currently unmanaged" and the "thread type managed by lease or default" states. I added doc comments that hopefully should help clarify.
uint32_t bitmask = 0;These two fields need some comments.
Done
I would omit this, because what this actually means is "explicitly initialize the first element to zero and then initialize the remaining elements from an empty initializer list".
See https://en.cppreference.com/w/cpp/language/list_initialization.html and https://en.cppreference.com/w/cpp/language/aggregate_initialization.html. In the latter, for "Implicitly initialized elements":
Otherwise, if the element is not a reference, the element is copy-initialized from an empty initializer list.
```suggestion
};
```
Done
class BASE_EXPORT ThreadTypeManager {This class should have a comment of some sort that describes it's API, even if it's in `internal`.
Done
if (CurrentIOThread::IsSet()) {I think this is pre-existing, but perhaps it'd be nice to come back here and clarify in a comment if both these bits can be set, and if so, why IO thread takes precedence.
Looked into the code and clarified that they can not be set at the same time.
// The lease system is currently not compatible with ScopedBoostablePriority.Is there a crbug to fix this? Or are they fundamentally incompatible? It's unclear if this should be a TODO with a crbug or not.
I've filed crbug.com/483622914 on this topic. Unlike with leases, you can control thread type in ScopedBoostableThread from another thread, and affinity is managed. ScopedBoostableThread and leases aren't fully incompatible, as using the former while the latter exists works (but not the other way around). Rephrased the doc comment.
bitmask |= (1u << type);Maybe a TODO to use `base::EnumSet` here would be appropriate.
Done
std::ostream& operator<<(std::ostream& os, ThreadType type) {If you feel we need these (I thought googletest has some default printing for enums, even if it's not as helpful as possible), they need to be defined alongside where the type is defined to avoid ODR violations.
Yes, and it was defined in platform_thread.cc, but I placed it here on request by etiennep@ who will migrate for some later ThreadPool business.
// used at the same time.Same question about whether or not a TODO is appropriate here.
The code you commented on was removed in the later patch set. I've equipped the platform thread code with documentary & todos. I equipped the header with TODOs on the relevant classes.
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
17 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_type.cc
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/threading/scoped_thread_priority.h
Insertions: 4, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/threading/platform_thread.cc
Insertions: 21, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: base/threading/platform_thread.h
Insertions: 24, Deletions: 4.
The diff is too large to show. Please review the diff.
```
Change information
PlatformThread: implement a lease system for thread types
Currently, managing thread priority involves manual calls to
SetCurrentThreadType, which can lead to conflicts or incorrect
states when multiple components attempt to boost or restore a
thread's priority.
There's also future need to manage leases of higher priority
from objects, existing on potentially the same threads, and that
do not have a relation to each other.
This patch introduces a lease-based management system:
- Introduces base::internal::ThreadTypeManager to track a thread's
base type and active leases.
- Added PlatformThread::RaiseThreadTypeLease, a RAII object that
ensures a thread's effective type is the maximum of all active
leases and its base type.
- Updated Blink's MainThreadSchedulerImpl and BeginFrameProvider
to use leases, simplifying the logic for boosting renderer threads
to kPresentation and dropping them back during compositor gestures.
- M base/BUILD.gn
- A base/task/thread_type.cc
- M base/task/thread_type.h
- M base/threading/platform_thread.cc
- M base/threading/platform_thread.h
- M base/threading/platform_thread_unittest.cc
- M base/threading/scoped_thread_priority.cc
- M base/threading/scoped_thread_priority.h
- M content/renderer/renderer_main.cc
- M third_party/blink/common/features.cc
- M third_party/blink/public/common/features.h
- M third_party/blink/renderer/platform/graphics/begin_frame_provider.cc
- M third_party/blink/renderer/platform/graphics/begin_frame_provider.h
- M third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.cc
- M third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.h
- M third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl_unittest.cc
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.h
- M third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl_unittest.cc
Code-Review: +1 by Francois Pierre Doray, +1 by Etienne Pierre-Doray, +1 by Daniel Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 4 comments![]( "Open in Gerrit")
Markus HandellLGTM w/nits
Overall, I do think the interaction between `ScopedPriorityBoost` and `RaiseThreadTypeLease`; I get the impression that they don't serve exactly the same purpose, but it's not also not 100% clear to me:
- when one is appropriate vs the other
- how they interact
- So improving the documentation here would also be good for a followup
Ack, @etie...@chromium.org perhaps let's discuss this on newly filed https://g-issues.chromium.org/issues/483622914?
SG, we should clarify the interaction, but I don't think there'll be a plan to improve interaction because there isn't any use case for it, so maybe we should adjust the wording in consequence.
// they're the first type set on the thread.Markus HandellAt least to me, this comment doesn't really help me understand why this is OK only if they're the first type set on the thread. I feel like there are some other assumptions about the system that should probably be mentioned here.
(Maybe it'll be less confusing once the current renames above are done?)
Agree the comment isn't good enough. It's ok to create leases when it's non-nullopt too, it's just that the lease may not be effective (consider a kDefault base priority, and kBackground lease - the lease will not change priority). We need the optional to discriminate between the "thread type currently unmanaged" and the "thread type managed by lease or default" states. I added doc comments that hopefully should help clarify.
FWIW, I'd expect a default thread type to always be set by the time a lease is created. (if that's the case we could simply DCHECK).
See my other comment for renderer main though.
// The lease system is currently not compatible with ScopedBoostablePriority.Markus HandellIs there a crbug to fix this? Or are they fundamentally incompatible? It's unclear if this should be a TODO with a crbug or not.
I've filed crbug.com/483622914 on this topic. Unlike with leases, you can control thread type in ScopedBoostableThread from another thread, and affinity is managed. ScopedBoostableThread and leases aren't fully incompatible, as using the former while the latter exists works (but not the other way around). Rephrased the doc comment.
SG, we should clarify the interaction, but I don't think there'll be a plan to improve interaction because there isn't any use case for it, so maybe we should adjust the wording in consequence.
base::PlatformThread::SetCurrentThreadType(base::ThreadType::kPresentation);We should still do SetCurrentThreadType(ThreadType::kDefault) before CreateMainThreadScheduler above no?
See my other comment about always having a default threadtype
Related details
- Markus Handell
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Markus Handell (Gerrit)
Markus Handell added 1 comment![]( "Open in Gerrit")
// they're the first type set on the thread.Markus HandellAt least to me, this comment doesn't really help me understand why this is OK only if they're the first type set on the thread. I feel like there are some other assumptions about the system that should probably be mentioned here.
(Maybe it'll be less confusing once the current renames above are done?)
Etienne Pierre-DorayAgree the comment isn't good enough. It's ok to create leases when it's non-nullopt too, it's just that the lease may not be effective (consider a kDefault base priority, and kBackground lease - the lease will not change priority). We need the optional to discriminate between the "thread type currently unmanaged" and the "thread type managed by lease or default" states. I added doc comments that hopefully should help clarify.
FWIW, I'd expect a default thread type to always be set by the time a lease is created. (if that's the case we could simply DCHECK).
See my other comment for renderer main though.
What's the scenario you're worried about?
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
// they're the first type set on the thread.Markus HandellAt least to me, this comment doesn't really help me understand why this is OK only if they're the first type set on the thread. I feel like there are some other assumptions about the system that should probably be mentioned here.
(Maybe it'll be less confusing once the current renames above are done?)
Etienne Pierre-DorayAgree the comment isn't good enough. It's ok to create leases when it's non-nullopt too, it's just that the lease may not be effective (consider a kDefault base priority, and kBackground lease - the lease will not change priority). We need the optional to discriminate between the "thread type currently unmanaged" and the "thread type managed by lease or default" states. I added doc comments that hopefully should help clarify.
Markus HandellFWIW, I'd expect a default thread type to always be set by the time a lease is created. (if that's the case we could simply DCHECK).
See my other comment for renderer main though.
What's the scenario you're worried about?
I had in mind we don't set ThreadType if the default isn't set, but that's not the case, so I think it's fine.
Markus Handell (Gerrit)
Markus Handell added 1 comment![]( "Open in Gerrit")
base::PlatformThread::SetCurrentThreadType(base::ThreadType::kPresentation);We should still do SetCurrentThreadType(ThreadType::kDefault) before CreateMainThreadScheduler above no?
See my other comment about always having a default threadtype
Acknowledged, no technical need at this point.