Use valid /prefetch arguments for process launches on Windows. - do not submit

chrome/common/prefetch_argument_win.cc

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/common/prefetch_argument_win.h"
+
+#include <string>
+
+#include "base/command_line.h"
+#include "base/logging.h"
+#include "base/strings/stringprintf.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/installer/util/browser_distribution.h"
+#include "components/startup_metric_utils/common/pre_read_field_trial_utils_win.h"
+#include "content/public/common/content_switches.h"
+
+namespace chrome {
+
+void AddWindowsPrefetchArgument(base::CommandLine* command_line) {
+  DCHECK(command_line);
+
+  bool ignored;
+  bool trial_prefetch_argument = false;
+  startup_metric_utils::GetPreReadOptions(
+      BrowserDistribution::GetDistribution()->GetRegistryPath(), &ignored,
+      &ignored, &ignored, &ignored, &trial_prefetch_argument);

[gab, 2016/01/18 19:10:51]

Make these parameters optional in GetPreReadOptions() instead of using
|&ignored| here.

Alternatively, return an int which is a bit-field of all the trials, e.g.:

---------------
int pre_read_options = startup_metric_utils::GetPreReadOptions(
    BrowserDistribution::GetDistribution()->GetRegistryPath());

if (!(pre_read_options &
      startup_metric_utils::TRIAL_USE_PREFETCH)) {
  return;
}
---------------

I think it may be time to prefer the latter (makes it clearer then a bunch of
bools which forces a double-check GetPreReadOptions()'s definition when reading
this code.

+  if (!trial_prefetch_argument)
+    return;
+
+  const std::string process_type =
+      command_line->GetSwitchValueASCII(switches::kProcessType);
+
+  // There can be 9 prefetch profiles per executable: a profile for launches
+  // without a prefetch argument and a profile per /prefetch:# argument, where #
+  // is [1, 8].

[gab, 2016/01/18 19:10:51]

"prefetch argument" -> "/prefetch argument"

+  int prefetch_argument;

[gab, 2016/01/18 19:10:52]

Default initialize to 0 and add comment (and also I prefer |prefetch_id| if only
for being shorter):

// S /prefetch:|prefetch_id| be added to the command-line, if |prefetch_id| is
// updated away from 0 below.
int prefetch_id = 0;

+
+  if (process_type.empty()) {
+    // No prefetch argument when the browser process is launched in foreground.
+    if (!command_line->HasSwitch(switches::kNoStartupWindow))
+      return;
+    prefetch_argument = 1;

[gab, 2016/01/18 19:10:51]

Now that there is a default value above I think this code reads better as:

if (command_line->HasSwitch(switches::kNoStartupWindow))
  prefetch_argument = 1

(don't even think it warrants a comment as it's pretty self-explanatory)

+  } else if (process_type == switches::kRendererProcess) {
+    prefetch_argument = 2;
+  } else if (process_type == switches::kGpuProcess) {
+    prefetch_argument = 3;
+  } else if (process_type == switches::kPpapiPluginProcess) {
+    prefetch_argument = 4;
+  } else {

[gab, 2016/01/18 19:10:52]

Worth adding one for the watcher process?

(don't see a reason not to use all profiles actually, we can always replace some
in later versions if new types become more important -- I'd say add a comment
one marginal types that are just there because there is room but could be
replaced)

+    // Default prefetch argument, shared by all process types that don't have
+    // one specified explicitly. It is likely that the prefetcher will never

[gab, 2016/01/18 19:10:51]

s/process types that don't have one specified explicitly/other process types/

+    // prefetch anything for these process types, as it won't be able to observe

[gab, 2016/01/18 19:10:51]

no ","

+    // consistent file reads across launches.

[gab, 2016/01/18 19:10:52]

s/./, but at least by using a custom profile they won't interfere with the
default profile used by the browser process./

+    prefetch_argument = 8;
+  }
+

[gab, 2016/01/18 19:10:52]

DCHECK_GE(prefetch_id, 0);
DCHECK_LE(prefetch_id, 8);

+  command_line->AppendArg(

[gab, 2016/01/18 19:10:52]

if (prefetch_id != 0)

+      base::StringPrintf("/prefetch:%d", prefetch_argument));
+}

[gab, 2016/01/18 19:10:51]

At the end (and also mention this as a requirement in the header comment):

// It has been observed that the /prefetch parameter is ignored by Windows
// if it isn't in the first 256 chars of the command-line.
DCHECK_LE(command_line->GetCommandLineString().size(), 256U);

(or whatever the actual length requirement is...)

Actually, the DCHECK should probably be at the top if it's an API requirement,
e.g. DCHECK_LE(command_line->GetCommandLineString().size(), 256U -
arraysize("/prefetch:#"));

+
+}  // namespace chrome