Reland "windows startup: OVERRIDE_PREFETCH_PARAMETER on singleton"
This has been fixed by replacing DumpWithoutCrashing with a UMA
histogram.
Fixed: 381384992, 380088804
Bug: 325307453
Original change's description:
> Revert "windows startup: OVERRIDE_PREFETCH_PARAMETER on acquiring singleton"
>
> This reverts commit 5693c9daaeb9ad34a00575195caf7f8a16342c35.
>
> Reason for revert: Broken GPU tests. 381384992
>
> See three candidate CLs for fixes here:
> - crrev.com/c/6055287
> - crrev.com/c/6055311
> - crrev.com/c/6056175
>
> Original change's description:
> > windows startup: OVERRIDE_PREFETCH_PARAMETER on acquiring singleton
> >
> > With this change, run a field trial to determine the impact of
> > OVERRIDE_PREFETCH_PARAMETER on the acquiry of the singleton.
> >
> > This should effectively solve the case where short-lived browser
> > processes are polluting the App Launch Prefetch history. Local tracing
> > leads us to believe that recent versions of Windows implicitly support
> > this but we decided to experiment with this newly discovered API
> > nonetheless to verify how this fares in the wild.
> >
> > Bug: 325307453, 380088804
> > Change-Id: I515d983c6f558e4e896aadf4796b48487236f5b8
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5939457
> > Reviewed-by: Etienne Bergeron <[email protected]>
> > Reviewed-by: Gabriel Charette <[email protected]>
> > Reviewed-by: Greg Thompson <[email protected]>
> > Commit-Queue: Sean Maher <[email protected]>
> > Cr-Commit-Position: refs/heads/main@{#1389076}
>
> Bug: 325307453, 380088804
> Change-Id: I36945c81352bf4eae35d982c17db40bae0e8b2e5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6055288
> Owners-Override: Kenichi Ishibashi <[email protected]>
> Bot-Commit: Rubber Stamper <[email protected]>
> Reviewed-by: Kenichi Ishibashi <[email protected]>
> Commit-Queue: Sean Maher <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1389655}
Bug: 325307453, 380088804
Change-Id: I4d14ad4b59c34ba747d01f6d4c66a3018284673a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6059052
Reviewed-by: Greg Thompson <[email protected]>
Reviewed-by: Sean Maher <[email protected]>
Reviewed-by: Etienne Bergeron <[email protected]>
Commit-Queue: Sean Maher <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1391721}
diff --git a/chrome/app/chrome_main_delegate.cc b/chrome/app/chrome_main_delegate.cc
index f39778a..9018092 100644
--- a/chrome/app/chrome_main_delegate.cc
+++ b/chrome/app/chrome_main_delegate.cc
@@ -957,6 +957,11 @@
}
#endif // BUILDFLAG(IS_CHROMEOS_LACROS)
+#if BUILDFLAG(IS_WIN)
+ ChromeProcessSingleton::GetInstance()
+ ->ChromeProcessSingleton::InitializeFeatures();
+#endif
+
CommonEarlyInitialization(invoked_in);
// Initializes the resource bundle and determines the locale.
diff --git a/chrome/browser/chrome_process_singleton.cc b/chrome/browser/chrome_process_singleton.cc
index 89f5198..166dd73 100644
--- a/chrome/browser/chrome_process_singleton.cc
+++ b/chrome/browser/chrome_process_singleton.cc
@@ -6,6 +6,16 @@
#include <utility>
+#include "build/build_config.h"
+
+#if BUILDFLAG(IS_WIN)
+#include <windows.h>
+
+#include "base/metrics/histogram_functions.h"
+#include "chrome/common/chrome_features.h"
+#include "components/app_launch_prefetch/app_launch_prefetch.h"
+#endif
+
namespace {
ChromeProcessSingleton* g_chrome_process_singleton_ = nullptr;
@@ -49,6 +59,50 @@
startup_lock_.Unlock();
}
+#if BUILDFLAG(IS_WIN)
+void ChromeProcessSingleton::InitializeFeatures() {
+ // On Windows, App Launch Prefetch (ALPF) will monitor the disk accesses done
+ // by processes launched, and load the resources used into memory before the
+ // process needs them, if possible. Different Chrome process types use
+ // different resources, and this is signaled to ALPF via the command line:
+ // passing "/prefetch:N" on the command line with different numbers causes
+ // ALPF to treat two launches placed in different buckets as separarate
+ // applications for its purposes.
+ //
+ // Short lived browser processes occur on notification and rendezvous, both
+ // cases where nearly nothing will be used from disk, and which are not
+ // launched with `/prefetch`, and are thus considered "browser" processes
+ // according to ALPF. This may be polluting the ALPF cache.
+ //
+ // The `::ProcessOverrideSubsequentPrefetchParameter` process information
+ // attribute will change the behavior of ALPF to explicitly consider
+ // subsequent launches while this singleton process is running (rendezvous,
+ // toast) of Chrome which do not specify a prefetch bucket as if they had
+ // specified the `kCatchAll` bucket.
+ //
+ // It is expected that this will overall improve the behavior of ALPF on
+ // Windows, which should decrease startup time for ordinary browser processes.
+ if (is_singleton_instance_ &&
+ (wcsstr(::GetCommandLineW(), L"/prefetch:") == nullptr) &&
+ base::FeatureList::IsEnabled(features::kOverridePrefetchOnSingleton)) {
+ OVERRIDE_PREFETCH_PARAMETER prefetch_parameter = {};
+ prefetch_parameter.Value = app_launch_prefetch::GetPrefetchBucket(
+ app_launch_prefetch::SubprocessType::kCatchAll);
+ // This is not fatal because it is an optimization and has no bearing on the
+ // functionality of the browser. See crbug.com/380088804 for details. It has
+ // been seen that occasionally (in CQ), this call fails with
+ // ERROR_INTERNAL_ERROR.
+ base::UmaHistogramSparse(
+ "Startup.PrefetchOverrideErrorCode",
+ ::SetProcessInformation(::GetCurrentProcess(),
+ ::ProcessOverrideSubsequentPrefetchParameter,
+ &prefetch_parameter, sizeof(prefetch_parameter))
+ ? ERROR_SUCCESS
+ : ::GetLastError());
+ }
+}
+#endif
+
// static
void ChromeProcessSingleton::CreateInstance(
const base::FilePath& user_data_dir) {
diff --git a/chrome/browser/chrome_process_singleton.h b/chrome/browser/chrome_process_singleton.h
index 7567e5b9..dc79446 100644
--- a/chrome/browser/chrome_process_singleton.h
+++ b/chrome/browser/chrome_process_singleton.h
@@ -48,6 +48,12 @@
bool IsSingletonInstanceForTesting() const { return is_singleton_instance_; }
+#if BUILDFLAG(IS_WIN)
+ // Must only be called both after initialization of FeatureList and
+ // NotifyOtherProcessOrCreate().
+ void InitializeFeatures();
+#endif
+
// Create the chrome process singleton instance for the current process.
static void CreateInstance(const base::FilePath& user_data_dir);
// Delete the chrome process singleton instance.
diff --git a/chrome/browser/chrome_process_singleton_win_unittest.cc b/chrome/browser/chrome_process_singleton_win_unittest.cc
index 0708e48..016fa9db 100644
--- a/chrome/browser/chrome_process_singleton_win_unittest.cc
+++ b/chrome/browser/chrome_process_singleton_win_unittest.cc
@@ -4,13 +4,20 @@
#include "chrome/browser/chrome_process_singleton.h"
+#include <windows.h>
+
+#include <processthreadsapi.h>
+
#include "base/command_line.h"
#include "base/compiler_specific.h"
#include "base/files/file_path.h"
#include "base/files/scoped_temp_dir.h"
#include "base/functional/bind.h"
#include "base/functional/callback.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
#include "build/build_config.h"
+#include "chrome/common/chrome_features.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace {
@@ -90,3 +97,21 @@
EXPECT_TRUE(ps1.IsSingletonInstanceForTesting());
EXPECT_FALSE(ps2.IsSingletonInstanceForTesting());
}
+
+TEST(ChromeProcessSingletonTest, OverridePrefetch) {
+ base::test::ScopedFeatureList scoped_feature(
+ features::kOverridePrefetchOnSingleton);
+ base::ScopedTempDir profile_dir;
+ ASSERT_TRUE(profile_dir.CreateUniqueTempDir());
+ ChromeProcessSingleton ps(profile_dir.GetPath());
+ base::HistogramTester tester;
+
+ ProcessSingleton::NotifyResult result = ps.NotifyOtherProcessOrCreate();
+ ASSERT_EQ(ProcessSingleton::PROCESS_NONE, result);
+
+ ps.ChromeProcessSingleton::InitializeFeatures();
+
+ auto buckets = tester.GetAllSamples("Startup.PrefetchOverrideErrorCode");
+ EXPECT_EQ(1UL, buckets.size());
+ EXPECT_EQ(1, buckets[0].count);
+}
diff --git a/chrome/common/chrome_features.cc b/chrome/common/chrome_features.cc
index d8db5c5..520f168b 100644
--- a/chrome/common/chrome_features.cc
+++ b/chrome/common/chrome_features.cc
@@ -890,6 +890,14 @@
base::FEATURE_ENABLED_BY_DEFAULT);
#endif
+#if BUILDFLAG(IS_WIN)
+// Changes behavior of App Launch Prefetch to ignore chrome browser launches
+// after acquiry of the singleton.
+BASE_FEATURE(kOverridePrefetchOnSingleton,
+ "OverridePrefetchOnSingleton",
+ base::FEATURE_DISABLED_BY_DEFAULT);
+#endif
+
// Enables or disables the page content opt-in and setting.
BASE_FEATURE(kPageContentOptIn,
"PageContentOptIn",
diff --git a/chrome/common/chrome_features.h b/chrome/common/chrome_features.h
index 94e62e3..e0ae9c9 100644
--- a/chrome/common/chrome_features.h
+++ b/chrome/common/chrome_features.h
@@ -528,6 +528,11 @@
COMPONENT_EXPORT(CHROME_FEATURES) BASE_DECLARE_FEATURE(kOomIntervention);
#endif
+#if BUILDFLAG(IS_WIN)
+COMPONENT_EXPORT(CHROME_FEATURES)
+BASE_DECLARE_FEATURE(kOverridePrefetchOnSingleton);
+#endif
+
COMPONENT_EXPORT(CHROME_FEATURES) BASE_DECLARE_FEATURE(kPageContentOptIn);
#if BUILDFLAG(IS_CHROMEOS_ASH)
