[blink] Add unit tests for BusyLoopOnRendererMain [chromium/src : main]
Anand Ravi (Gerrit)
Anand Ravi added 1 comment![]( "Open in Gerrit")
As promised in [7226094](https://chromium-review.googlesource.com/c/chromium/src/+/7226094), unit tests for the feature.
Please review, thanks!
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 5 comments![]( "Open in Gerrit")
As promised in [7226094](https://chromium-review.googlesource.com/c/chromium/src/+/7226094), unit tests for the feature.
Please review, thanks!
Sweet, thanks for following up! Tests look great, a few comments on other parts.
BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kBusyLoopOnRendererMain);If these are only needed in the blink scheduler, they can go in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/common/features.h and .cc.
MaybeSetBusyLoop();I know this is existing, but does the `UpdatePolicy()` below not also call `MaybeSetBusyLoop()`? Or does it early-out before that?
if (!::features::IsEligibleForThrottleMainFrameTo60Hz() ||Optional since this matches existing code, but can you early exit this if the features aren't enabled, i.e. can we just do nothing in that case? That's a bit more idiomatic for feature code unless something below still needs to run.
void SetCurrentUseCase(UseCase use_case) {There's a `SetUseCaseAndUpdatePolicy()` that was added to main_thread_scheduler_impl_unittest.cc that allows overriding the `UseCase` that gets used within `UpdatePolicy()`. Could that be used (probably needs to be moved up to the main test class) instead of adding these two methods? I think that would allow MaybeSetBusyLoop() to be called via UpdatePolicy, which tests a bit more.
Related details
- Anand Ravi
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. |
Anand Ravi (Gerrit)
Anand Ravi added 4 comments![]( "Open in Gerrit")
BLINK_COMMON_EXPORT BASE_DECLARE_FEATURE(kBusyLoopOnRendererMain);If these are only needed in the blink scheduler, they can go in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/common/features.h and .cc.
Done
I know this is existing, but does the `UpdatePolicy()` below not also call `MaybeSetBusyLoop()`? Or does it early-out before that?
You are correct. Removed the line and verified that busy loop scale factor is indeed updated.
if (!::features::IsEligibleForThrottleMainFrameTo60Hz() ||Optional since this matches existing code, but can you early exit this if the features aren't enabled, i.e. can we just do nothing in that case? That's a bit more idiomatic for feature code unless something below still needs to run.
Done. I had to make the tests parametrized to achieve this but I agree that its better now that the production code is idiomatic, even if the tests are a bit more cumbersome to understand.
void SetCurrentUseCase(UseCase use_case) {There's a `SetUseCaseAndUpdatePolicy()` that was added to main_thread_scheduler_impl_unittest.cc that allows overriding the `UseCase` that gets used within `UpdatePolicy()`. Could that be used (probably needs to be moved up to the main test class) instead of adding these two methods? I think that would allow MaybeSetBusyLoop() to be called via UpdatePolicy, which tests a bit more.
The use case value is updated in UpdatePolicy() [(cs)](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc;l=1657;drc=e63596721df61bbc199c38c4a102597ad81ad154) which makes it difficult to test how busy-looping interacts with the use-case for all use-cases.
It also trips a few carefully placed asserts in UpdatePolicy() which expect use-cases to change in a specific manner or require other values to be set.
Related details
- 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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
3 comments
float busy_loop_scale_factor;Initialize to 0?
void SetCurrentUseCase(UseCase use_case) {Anand RaviThere's a `SetUseCaseAndUpdatePolicy()` that was added to main_thread_scheduler_impl_unittest.cc that allows overriding the `UseCase` that gets used within `UpdatePolicy()`. Could that be used (probably needs to be moved up to the main test class) instead of adding these two methods? I think that would allow MaybeSetBusyLoop() to be called via UpdatePolicy, which tests a bit more.
The use case value is updated in UpdatePolicy() [(cs)](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc;l=1657;drc=e63596721df61bbc199c38c4a102597ad81ad154) which makes it difficult to test how busy-looping interacts with the use-case for all use-cases.
It also trips a few carefully placed asserts in UpdatePolicy() which expect use-cases to change in a specific manner or require other values to be set.
The use case value is updated in UpdatePolicy() [(cs)](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc;l=1657;drc=e63596721df61bbc199c38c4a102597ad81ad154) which makes it difficult to test how busy-looping interacts with the use-case for all use-cases.
`SetUseCaseAndUpdatePolicy()` sets a special `use_case_override_`, and `ComputeCurrentUseCase()` is overridden to return that [1], so that should just work when iterating over all use cases?
It also trips a few carefully placed asserts in UpdatePolicy() which expect use-cases to change in a specific manner or require other values to be set.
Hmm I tried this locally with your patch and didn't hit any checks (and the tests all pass). Which checks did you hit?
---
If that doesn't work, maybe consider moving `ForceSetBusyLoop()` into `MainThreadSchedulerImplForTest` since it's a friend class, to cut down on the number of test-only methods. You could also pass the UseCase to `MaybeSetBusyLoop()` to avoid having to set the use case. But it would be nice to go through `UpdatePolicy()` if possible just to ensure your method and side effects are being called, which can also helps prevent future regressions.
Related details
- Anand Ravi
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. |
Anand Ravi (Gerrit)
Anand Ravi added 2 comments![]( "Open in Gerrit")
float busy_loop_scale_factor;Anand RaviInitialize to 0?
Fixed.
void SetCurrentUseCase(UseCase use_case) {Anand RaviThere's a `SetUseCaseAndUpdatePolicy()` that was added to main_thread_scheduler_impl_unittest.cc that allows overriding the `UseCase` that gets used within `UpdatePolicy()`. Could that be used (probably needs to be moved up to the main test class) instead of adding these two methods? I think that would allow MaybeSetBusyLoop() to be called via UpdatePolicy, which tests a bit more.
Scott HaseleyThe use case value is updated in UpdatePolicy() [(cs)](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc;l=1657;drc=e63596721df61bbc199c38c4a102597ad81ad154) which makes it difficult to test how busy-looping interacts with the use-case for all use-cases.
It also trips a few carefully placed asserts in UpdatePolicy() which expect use-cases to change in a specific manner or require other values to be set.
The use case value is updated in UpdatePolicy() [(cs)](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/scheduler/main_thread/main_thread_scheduler_impl.cc;l=1657;drc=e63596721df61bbc199c38c4a102597ad81ad154) which makes it difficult to test how busy-looping interacts with the use-case for all use-cases.
`SetUseCaseAndUpdatePolicy()` sets a special `use_case_override_`, and `ComputeCurrentUseCase()` is overridden to return that [1], so that should just work when iterating over all use cases?
It also trips a few carefully placed asserts in UpdatePolicy() which expect use-cases to change in a specific manner or require other values to be set.
Hmm I tried this locally with your patch and didn't hit any checks (and the tests all pass). Which checks did you hit?
---
If that doesn't work, maybe consider moving `ForceSetBusyLoop()` into `MainThreadSchedulerImplForTest` since it's a friend class, to cut down on the number of test-only methods. You could also pass the UseCase to `MaybeSetBusyLoop()` to avoid having to set the use case. But it would be nice to go through `UpdatePolicy()` if possible just to ensure your method and side effects are being called, which can also helps prevent future regressions.
I remember this check being tripped in the initial versions if I iterated on the use cases more than once. I modified the tests to use SetUseCaseAndUpdatePolicy() and iterate more than once on the use cases .. and the tests pass locally.
So I have switched it to use SetUseCaseAndUpdatePolicy().
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anand Ravi (Gerrit)
Anand Ravi added 1 comment![]( "Open in Gerrit")
Victor, can you review the one line change in cc? Thanks!
Related details
- Victor Miura
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Miura (Gerrit)
Victor Miura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Victor Miura
| Code-Review | +1 |
1 comment
- Anand Ravi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anand Ravi (Gerrit)
Anand Ravi voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anand Ravi (Gerrit)
Anand Ravi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anand Ravi
| Commit-Queue | +2 |
1 comment
Looks to me like the GPU tests are flaky on Pixel 6. Trying to submit again.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anand Ravi (Gerrit)
Anand Ravi voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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![]( "Open in Gerrit")
Change information
[blink] Add unit tests for BusyLoopOnRendererMain
- M cc/base/features.cc
- M third_party/blink/renderer/platform/scheduler/common/features.cc
- M third_party/blink/renderer/platform/scheduler/common/features.h
- M third_party/blink/renderer/platform/scheduler/main_thread/DEPS
- 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 Scott Haseley, +1 by Victor Miura
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |