Delay BEST_EFFORT tasks during startup until tabs are loaded/idle [chromium/src : main]
Joe Mason (Gerrit)
Joe Mason added 1 comment![]( "Open in Gerrit")
AfterStartupTaskUtils::RegisterStartupInProgressRef();Microsoft has a patch at https://crrev.com/c/7982644 that adds a BEST_EFFORT task to session restore, which would conflict with this.
I've asked them to put it behind a feature flag. Can you wrap this in a flag too, so we can make sure they aren't both enabled at the same time?
Related details
- Gabriel Charette
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. |
Gabriel Charette (Gerrit)
Gabriel Charette added 2 comments![]( "Open in Gerrit")
Microsoft has a patch at https://crrev.com/c/7982644 that adds a BEST_EFFORT task to session restore, which would conflict with this.
I've asked them to put it behind a feature flag. Can you wrap this in a flag too, so we can make sure they aren't both enabled at the same time?
Thanks, I actually put everything behind a feature after all as shifting guardrails (PPM guardrails even more so) should be done carefully.
Related details
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason added 13 comments![]( "Open in Gerrit")
DCHECK_GT(g_ref_count, 0);Nit: prefer CHECK_GT here, since this doesn't run in a performance-sensitive loop. (At least, I assume there won't be thousands of these refs churning at startup...)
Also please #include "base/check_op.h" for IWYU.
void OnStartupComplete() {Nit: rename this to `OnFirstVisiblePageLoadComplete`? It's confusing now because `OnStartupComplete` feels like it should be triggered when the refcount reaches 0.
!base::FeatureList::IsEnabled(Nit: please #include "base/feature_list.h" for IWYU.
return;Nit: I would CHECK here, since logically this should only be called once. (If it's called multiple times in tests that aren't worth refactoring, add a `BeginMonitoringStartupCompletionForTesting` that skips the CHECK?)
if (performance_manager::PerformanceManager::IsAvailable()) {Why is this added? Seems valuable to CHECK that PerformanceManager is available at this point.
If it's for tests, I'd do something like:
```
void BeginMonitoringStartupCompletionImpl(bool install_pm_observer) {
CHECK(!g_is_monitoring_started);
g_is_monitoring_started = true;
...
if (install_pm_observer) {
StartupObserver::Start(); // CHECKs PerformanceManager::IsAvailable()
}
...
}
void BeginMonitoringStartupCompletion() {
BeginMonitoringStartupCompletionImpl(true);
}
void BeginMonitoringStartupCompletionForTesting() {
if (g_is_monitoring_started) {
return;
}
BeginMonitoringStartupCompletionImpl(PerformanceManager::IsAvailable());
}
``` : release_runner_(std::move(release_callback)) {}Nit: please #include <utility> for IWYU.
base::BindOnce(&ReleaseRef));Nit: please #include "base/functional/bind.h" and "base/functional/callback.h" for IWYU.
if (base::FeatureList::IsEnabled(features::kImprovedStartupBestEffortDelay)) {Nit: please #include "base/feature_list.h" for IWYU.
if (base::FeatureList::IsEnabled(features::kImprovedStartupBestEffortDelay)) {Nit: please #include "base/feature_list.h" for IWYU.
if (base::FeatureList::IsEnabled(features::kImprovedStartupBestEffortDelay)) {This should have a separate feature, or an "include session restore" FeatureParam, so that we can turn it off separately if Microsoft's patch to use BEST_EFFORT tasks in session restore is enabled.
Related details
- Gabriel Charette
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. |
Gabriel Charette (Gerrit)
Message from Gabriel Charette![]( "Open in Gerrit")
All code review comments have been addressed, including making the StartupObserver header block continuous inside #if !IS_ANDROID. CQ Dry Run on Patch Set 25 passed across all builders. @Joe PTaL!
Related details
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. |
Gabriel Charette (Gerrit)
Gabriel Charette added 14 comments![]( "Open in Gerrit")
Nit: prefer CHECK_GT here, since this doesn't run in a performance-sensitive loop. (At least, I assume there won't be thousands of these refs churning at startup...)
Also please #include "base/check_op.h" for IWYU.
Done
Nit: rename this to `OnFirstVisiblePageLoadComplete`? It's confusing now because `OnStartupComplete` feels like it should be triggered when the refcount reaches 0.
Done
Nit: please #include "base/feature_list.h" for IWYU.
Done
Nit: I would CHECK here, since logically this should only be called once. (If it's called multiple times in tests that aren't worth refactoring, add a `BeginMonitoringStartupCompletionForTesting` that skips the CHECK?)
Done
if (performance_manager::PerformanceManager::IsAvailable()) {Why is this added? Seems valuable to CHECK that PerformanceManager is available at this point.
If it's for tests, I'd do something like:
```
void BeginMonitoringStartupCompletionImpl(bool install_pm_observer) {
CHECK(!g_is_monitoring_started);
g_is_monitoring_started = true;
...
if (install_pm_observer) {
StartupObserver::Start(); // CHECKs PerformanceManager::IsAvailable()
}
...
}void BeginMonitoringStartupCompletion() {
BeginMonitoringStartupCompletionImpl(true);
}
void BeginMonitoringStartupCompletionForTesting() {
if (g_is_monitoring_started) {
return;
}
BeginMonitoringStartupCompletionImpl(PerformanceManager::IsAvailable());
}
```
Good point. Tweaked ifdefs and test-only paths to make this cleaner.
Nit: please #include <utility> for IWYU.
Done
Nit: please #include "base/functional/bind.h" and "base/functional/callback.h" for IWYU.
Done
if (base::FeatureList::IsEnabled(features::kImprovedStartupBestEffortDelay)) {Nit: please #include "base/feature_list.h" for IWYU.
Done
if (base::FeatureList::IsEnabled(features::kImprovedStartupBestEffortDelay)) {Nit: please #include "base/feature_list.h" for IWYU.
Done
if (base::FeatureList::IsEnabled(features::kImprovedStartupBestEffortDelay)) {This should have a separate feature, or an "include session restore" FeatureParam, so that we can turn it off separately if Microsoft's patch to use BEST_EFFORT tasks in session restore is enabled.
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Gabriel Charette voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Joe Mason
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Mason (Gerrit)
Joe Mason added 2 comments![]( "Open in Gerrit")
int g_ref_count = 1;Nit: There's also a hidden side effect of this on Android: since the ReleaseRef() is inside `!BUILDFLAG(IS_ANDROID)`, NONE of the StartupInProgressRefs will ever do anything.
Which is good, because AfterStartupTaskUtils.java calls `SetBrowserStartupIsComplete` directly (at https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/after_startup_task_utils_android.cc;l=24;drc=fa29eb59b7b46bda1557299a1645ddf3911c69bf) so this patch has no behaviour change on Android.
That's worth a comment because otherwise it would be hard to notice the importance of skipping ReleaseRef() on Android.
As a followup, we might want to make AfterStartupTaskUtils.java create a StartupInProgressRef instead of calling `SetBrowserStartupIsComplete`, and enabling that `ReleaseRef()` call on Android too. Would need some refactoring though.
base::Minutes(3));Don't we still want the failsafe timeout on Android?
Related details
- Gabriel Charette
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. |
Gabriel Charette (Gerrit)
Message from Gabriel Charette![]( "Open in Gerrit")
Patch Set 27 isolates all startup ref concepts (StartupInProgressRef, RegisterStartupInProgressRef, g_ref_count, ReleaseRef, StartupObserver) inside #if !BUILDFLAG(IS_ANDROID) with a TODO(crbug.com/528419929) to refactor AfterStartupTaskUtils.java on Android, and preserves the 3-minute failsafe timer in BeginMonitoringStartupCompletion on Android. CQ Dry Run on PS27 passed across all builders. @Joe PTaL!
Related details
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. |