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

Use valid /prefetch arguments for process launches on Windows.

The /prefetch:# argument that was used by Chrome didn't work because #
has to be in [1, 8].

With this CL, valid /prefetch arguments are used. Windows will create
a separate profile for:
- browser process in foreground (no /prefetch argument)
- browser process in background (/prefetch:1)
- renderer (/prefetch:2)
- gpu (/prefetch:3)
- ppapi (/prefetch:4)
- all other process types (/prefetch:8)

BUG=577698

[fdoray, 2016-01-15 20:30:47]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org

[fdoray, 2016-01-15 20:30:48]

gab@: Can you review this CL? I'll add owners once I get your lgtm.

Info:
- The prefetch switch is added by
ChromeContentBrowserClient::AppendExtraCommandLineSwitches for many processes,
including renderer+gpu.
- Otherwise, there is call to chrome::AddWindowsPrefetchArgument where the
command line is created.

Questions:
- Is there other process types that deserve their own /prefetch:# switch?
- I chose a simple solution for nacl_broker_listener.cc. Would it be better if I
added a way to call chrome::AddWindowsPrefetchArgument from a component through
content::ContentClient?

[fdoray, 2016-01-15 20:31:37]

fdoray@chromium.org changed reviewers:
+ grt@chromium.org

[fdoray, 2016-01-15 20:31:37]

+grt@

Info:
- The prefetch switch is added by
ChromeContentBrowserClient::AppendExtraCommandLineSwitches for many processes,
including renderer+gpu.
- Otherwise, there is call to chrome::AddWindowsPrefetchArgument where the
command line is created.

Questions:
- Is there other process types that deserve their own /prefetch:# switch?
- I chose a simple solution for nacl_broker_listener.cc. Would it be better if I
added a way to call chrome::AddWindowsPrefetchArgument from a component through
content::ContentClient?

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

lg, very excited to see this! Comments/suggestions below.

Cheers!
Gab

PS: Suggest reading all comments before taking action, it's not impossible some
of them are voided by top-level changes I ended up suggesting later...

https://codereview.chromium.org/1595633002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1595633002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1276:
chrome::AddWindowsPrefetchArgument(new_cl.get());
Move to line 1271 (i.e. where |new_cl| is being built instead of after the exit
call).

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
File chrome/common/prefetch_argument_win.cc (right):

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:26: &ignored, &ignored, &ignored,
&trial_prefetch_argument);
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.

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:35: // is [1, 8].
"prefetch argument" -> "/prefetch argument"

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:36: int prefetch_argument;
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;

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:42: prefetch_argument = 1;
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)

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:49: } else {
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)

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:51: // one specified explicitly. It is
likely that the prefetcher will never
s/process types that don't have one specified explicitly/other process types/

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:52: // prefetch anything for these
process types, as it won't be able to observe
no ","

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:53: // consistent file reads across
launches.
s/./, but at least by using a custom profile they won't interfere with the
default profile used by the browser process./

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:56: 
DCHECK_GE(prefetch_id, 0);
DCHECK_LE(prefetch_id, 8);

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:57: command_line->AppendArg(
if (prefetch_id != 0)

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.cc:59: }
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:#"));

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
File chrome/common/prefetch_argument_win.h (right):

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.h:14: // --no-startup-window switches must
already have been added, if applicable, as
I find it weird to have to document which flags have to have been added already.

And since I'm suggesting somewhere else that we move this to components you
won't have access to those switches constants anyways...

So how about we instead have an enum here of every process type we handle and
the caller tells us which one it is?

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.h:19: // time on-demand. The prefetcher
wouldn't be able to observe consistent file
s/at the time/at a time/

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.h:19: // time on-demand. The prefetcher
wouldn't be able to observe consistent file
s/wouldn't/isn't/

https://codereview.chromium.org/1595633002/diff/20001/chrome/common/prefetch_...
chrome/common/prefetch_argument_win.h:22: void
AddWindowsPrefetchArgument(base::CommandLine* command_line);
To ensure this is called by everyone creating a chrome.exe process how about we
add a DCHECK in chrome.exe's main that it either identifies itself as a browser
process or there is a /prefetch flag on its command-line?

That should catch any new process that forgets to calls this when forging its
command-line?

https://codereview.chromium.org/1595633002/diff/20001/chrome/installer/util/a...
File chrome/installer/util/auto_launch_util.cc (right):

https://codereview.chromium.org/1595633002/diff/20001/chrome/installer/util/a...
chrome/installer/util/auto_launch_util.cc:62:
chrome::AddWindowsPrefetchArgument(&cmd_line);
Most prefetch IDs will be flexible across versions but this one isn't as it's a
command-line written in the registry. We should note that in the comments where
id 1 is being assigned in AddWindowsPrefetchArgument().

Also, would be nice to retroactively fix already registered command-line
(otherwise this change will initially be a no-op and will very slowly propagate
over-time as people somehow switch out&in of background mode -- or
re-install..).

https://codereview.chromium.org/1595633002/diff/20001/components/nacl/broker/...
File components/nacl/broker/nacl_broker_listener.cc (right):

https://codereview.chromium.org/1595633002/diff/20001/components/nacl/broker/...
components/nacl/broker/nacl_broker_listener.cc:110:
cmd_line->AppendArg("/prefetch:8");
Would be nice to have all code use the actual code path, i.e. maybe
AddWindowsPrefetchArgument() should be in components/startup_metric_utils too.

Also, this should be OS_WIN (won't have a choice anyways once using the actual
Windows-only call)

(now seeing that this was in your original post's questions: answer = yes!)

https://codereview.chromium.org/1595633002/diff/20001/components/startup_metr...
File components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc
(right):

https://codereview.chromium.org/1595633002/diff/20001/components/startup_metr...
components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:75:
{kPrefetchArgumentVariationName, prefetch_argument},
Actually, /prefetch applies only to processes launched after the initial startup
phase during which we can't use Finch... so we could use the regular Finch
groups (no registry) for this one... WDYT?

I guess since we want to mix it with other prefetch experiments it's still best
to keep it here even though it's not necessary for this specific variation,
it'll make for easier analysis of variation mixes.

https://codereview.chromium.org/1595633002/diff/20001/components/startup_metr...
components/startup_metric_utils/common/pre_read_field_trial_utils_win.cc:80: //
the registry. Set to false otherwise (default behavior).
Would be nice to see /prefetch in action on the waterfall, there is a config
somewhere to specify what the Finch defaults should be on the waterfall, let's
try to enable /prefetch on the waterfall as well as part of this change?

https://codereview.chromium.org/1595633002/diff/20001/components/startup_metr...
File components/startup_metric_utils/common/pre_read_field_trial_utils_win.h
(right):

https://codereview.chromium.org/1595633002/diff/20001/components/startup_metr...
components/startup_metric_utils/common/pre_read_field_trial_utils_win.h:36:
bool* prefetch_argument);
As suggested in caller above I'd say let's do:

enum PreReadOptions {
  NO_PRE_READ = 1 << 0,
  HIGH_PRIORITY = 1 << 1,
  ONLY_IF_COLD = 1 << 2,
  PREFETCH_VIRTUAL_MEMORY = 1 << 3,
  PREFETCH_ARGUMENT = 1 << 4,
}

int GetPreReadOptions(const base::string16& product_registry_path);
  

+comments

https://codereview.chromium.org/1595633002/diff/20001/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1595633002/diff/20001/content/common/sandbox_...
content/common/sandbox_win.cc:15: #include "base/hash.h"
Also remove this.

[grt (UTC plus 2), 2016-01-19 15:40:15]

On 2016/01/15 20:31:37, fdoray wrote:
> +grt@

Let me know if you'd like me to look before Gab's comments are addressed.
Otherwise I'll hold off. Thanks.

[fdoray, 2016-01-21 01:22:13]

Description was changed from

==========
Use valid /prefetch arguments for process launches on Windows.

The /prefetch:# argument that was used by Chrome didn't work because #
has to be in [1, 8].

With this CL, valid /prefetch arguments are used. Windows will create
a separate profile for:
- browser process in foreground (no /prefetch argument)
- browser process in background (/prefetch:1)
- renderer (/prefetch:2)
- gpu (/prefetch:3)
- ppapi (/prefetch:4)
- all other process types (/prefetch:8)

BUG=577698
==========

to

==========
Use valid /prefetch arguments for process launches on Windows.

The /prefetch:# argument that was used by Chrome didn't work because #
has to be in [1, 8].

With this CL, valid /prefetch arguments are used. Windows will create
a separate profile for:
- browser process in foreground (no /prefetch argument)
- browser process in background (/prefetch:1)
- renderer (/prefetch:2)
- gpu (/prefetch:3)
- ppapi (/prefetch:4)
- all other process types (/prefetch:8)

BUG=577698
==========