Added support for 'spare' file that can be used at startup.

Added support for 'spare' file that can be used at startup.

Memory-mapped files make I/O errors into bus errors if something
is wrong with the file and cannot be mapped, including if the file
is sparse and the disk is full.  To counter this, such files are now
always pre-allocated to their full size but this has a cost.

To counter the cost, create a "spare" file some time after startup
that can be used for persistent metrics during the following run,
thus eliminating the cost of pre-allocating that file during browser
startup.

This is especially important on Android where the cost of allocating
the file is significant and yet persisting metrics is the most useful.

Performance tests indicate this reverses both android regressions
but it won't be until real metrics start being collected that an
accurate assessment can be made.

BUG=721806, 694565
Review-Url: https://codereview.chromium.org/2888563005
Cr-Commit-Position: refs/heads/master@{#474414}
Committed: https://chromium.googlesource.com/chromium/src/+/9963f7ea173756bc0b8014ed0dcd9bd48a6d3598

[bcwhite, 2017-05-17 19:39:02]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 19:40:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 19:51:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 19:51:14]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bcwhite, 2017-05-17 20:47:14]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 20:48:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 21:01:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 21:01:57]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[bcwhite, 2017-05-18 13:24:21]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 13:24:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-05-18 13:30:52]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-05-18 13:33:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 13:33:56]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bcwhite, 2017-05-18 13:45:14]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2017-05-18 13:45:28]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-05-18 13:45:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 13:56:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 13:56:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bcwhite, 2017-05-18 14:03:10]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 14:03:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-05-18 14:13:54]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-05-18 15:14:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 15:14:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bcwhite, 2017-05-19 16:07:22]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 16:08:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-05-19 16:13:06]

Description was changed from

==========
Added support for 'spare' file that can be used at startup.

Memory-mapped files make I/O errors into bus errors if something
is wrong with the file and cannot be mapped, including if the file
is sparse and the disk is full.  To counter this, such files are now
always pre-allocated to their full size but this has a cost.

To counter the cost, create a "spare" file some time after startup
that can be used for persistent metrics during the following run,
thus eliminating the cost of pre-allocating that file during browser
startup.

This is especially important on Android where the cost of allocating
the file is significant and yet persisting metrics is the most useful.

BUG=721806, 694565
==========

to

==========
Added support for 'spare' file that can be used at startup.

Memory-mapped files make I/O errors into bus errors if something
is wrong with the file and cannot be mapped, including if the file
is sparse and the disk is full.  To counter this, such files are now
always pre-allocated to their full size but this has a cost.

To counter the cost, create a "spare" file some time after startup
that can be used for persistent metrics during the following run,
thus eliminating the cost of pre-allocating that file during browser
startup.

This is especially important on Android where the cost of allocating
the file is significant and yet persisting metrics is the most useful.

Performance tests indicate this reverses both android regressions but it won't
be until real metrics start being collected that an accurate assessment can be
made.

BUG=721806, 694565
==========

[bcwhite, 2017-05-19 16:51:13]

Description was changed from

==========
Added support for 'spare' file that can be used at startup.

Memory-mapped files make I/O errors into bus errors if something
is wrong with the file and cannot be mapped, including if the file
is sparse and the disk is full.  To counter this, such files are now
always pre-allocated to their full size but this has a cost.

To counter the cost, create a "spare" file some time after startup
that can be used for persistent metrics during the following run,
thus eliminating the cost of pre-allocating that file during browser
startup.

This is especially important on Android where the cost of allocating
the file is significant and yet persisting metrics is the most useful.

Performance tests indicate this reverses both android regressions but it won't
be until real metrics start being collected that an accurate assessment can be
made.

BUG=721806, 694565
==========

to

==========
Added support for 'spare' file that can be used at startup.

Memory-mapped files make I/O errors into bus errors if something
is wrong with the file and cannot be mapped, including if the file
is sparse and the disk is full.  To counter this, such files are now
always pre-allocated to their full size but this has a cost.

To counter the cost, create a "spare" file some time after startup
that can be used for persistent metrics during the following run,
thus eliminating the cost of pre-allocating that file during browser
startup.

This is especially important on Android where the cost of allocating
the file is significant and yet persisting metrics is the most useful.

Performance tests indicate this reverses both android regressions
but it won't be until real metrics start being collected that an
accurate assessment can be made.

BUG=721806, 694565
==========

[commit-bot: I haz the power, 2017-05-19 17:22:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 17:22:51]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bcwhite, 2017-05-23 15:10:10]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 15:10:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-05-23 17:05:41]

Patchset #3 (id:100001) has been deleted

[bcwhite, 2017-05-23 17:08:09]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-05-23 17:27:58]

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:844: File
spare_file(temp_spare_path, File::FLAG_CREATE_ALWAYS |
Nit: I would have wrapped after the , and aligned all the flags on the same line
with temp_spare_path. But ignore if git cl format reformats this way instead.

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:445: StringPiece name);
Nit: name -> file_name if this is the intention of the param.

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator_unittest.cc (right):

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator_unittest.cc:171:
EXPECT_EQ(static_cast<int>(temp_size), file.GetLength());
Nit: int64_t since that's the type of GetLength()

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:50: constexpr int
kSpareFileCreateDelaySeconds = 90;
If spare file is not required, why should we even create it on non-Android?

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:57: void
InstantiatePersistentHistograms() {
This function is getting more and more complicated. I'm OK with not doing it in
this CL, but let's move this code out of chrome_browser_field_trials.cc soon
(e.g. after this CL) - looks like there's already a TODO about it.

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:62: base::FilePath metrics_file,
active_file, spare_file;
Nit: 1 per line.

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:125: }  // clang-format on
What's this comment for?

[commit-bot: I haz the power, 2017-05-23 17:29:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 17:30:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bcwhite, 2017-05-24 16:07:57]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 16:08:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-05-24 16:49:53]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 16:50:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-05-24 17:01:50]

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:844: File
spare_file(temp_spare_path, File::FLAG_CREATE_ALWAYS |
On 2017/05/23 17:27:58, Alexei Svitkine (slow) wrote:
> Nit: I would have wrapped after the , and aligned all the flags on the same
line
> with temp_spare_path. But ignore if git cl format reformats this way instead.

Yes, this is git cl format.

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:445: StringPiece name);
On 2017/05/23 17:27:58, Alexei Svitkine (slow) wrote:
> Nit: name -> file_name if this is the intention of the param.

It's the name of the allocator as with all the other Create*() methods.  The
filename will be based off of it.  Comment updated.

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator_unittest.cc (right):

https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator_unittest.cc:171:
EXPECT_EQ(static_cast<int>(temp_size), file.GetLength());
On 2017/05/23 17:27:58, Alexei Svitkine (slow) wrote:
> Nit: int64_t since that's the type of GetLength()

Done.

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:50: constexpr int
kSpareFileCreateDelaySeconds = 90;
On 2017/05/23 17:27:58, Alexei Svitkine (slow) wrote:
> If spare file is not required, why should we even create it on non-Android?

It's still a performance boost on all platforms, just not as significant.

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:57: void
InstantiatePersistentHistograms() {
On 2017/05/23 17:27:58, Alexei Svitkine (slow) wrote:
> This function is getting more and more complicated. I'm OK with not doing it
in
> this CL, but let's move this code out of chrome_browser_field_trials.cc soon
> (e.g. after this CL) - looks like there's already a TODO about it.

Sure.  I tried to split it into two methods but the data overlap was such that
pretty much everything had to be passed as a parameter or recalculated.

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:62: base::FilePath metrics_file,
active_file, spare_file;
On 2017/05/23 17:27:58, Alexei Svitkine (slow) wrote:
> Nit: 1 per line.

Done.

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:125: }  // clang-format on
On 2017/05/23 17:27:58, Alexei Svitkine (slow) wrote:
> What's this comment for?

Leftover from previous organization where clang-format couldn't get it right. 
Removed.

[Alexei Svitkine (slow), 2017-05-24 17:09:53]

lgtm

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:50: constexpr int
kSpareFileCreateDelaySeconds = 90;
On 2017/05/24 17:01:50, bcwhite wrote:
> On 2017/05/23 17:27:58, Alexei Svitkine (slow) wrote:
> > If spare file is not required, why should we even create it on non-Android?
> 
> It's still a performance boost on all platforms, just not as significant.

OK, please add this to the comment in the #else to provide this context.

[bcwhite, 2017-05-24 18:07:46]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 18:08:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-05-24 18:23:29]

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:50: constexpr int
kSpareFileCreateDelaySeconds = 90;
On 2017/05/24 17:09:53, Alexei Svitkine (slow) wrote:
> On 2017/05/24 17:01:50, bcwhite wrote:
> > On 2017/05/23 17:27:58, Alexei Svitkine (slow) wrote:
> > > If spare file is not required, why should we even create it on
non-Android?
> > 
> > It's still a performance boost on all platforms, just not as significant.
> 
> OK, please add this to the comment in the #else to provide this context.

Done.

[commit-bot: I haz the power, 2017-05-24 19:44:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 19:44:08]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-05-24 19:57:10]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2017-05-24 19:57:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2888563005/#ps170001
(title: "added comment about spare file on non-android")

[commit-bot: I haz the power, 2017-05-24 19:57:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 20:06:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 20:06:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[bcwhite, 2017-05-24 20:45:40]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2017-05-24 20:46:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 20:52:41]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1495658740144130,
"parent_rev": "ad35cf4c798788d0d17f025e2ff5b15c61ce7985", "commit_rev":
"9963f7ea173756bc0b8014ed0dcd9bd48a6d3598"}

[commit-bot: I haz the power, 2017-05-24 20:52:53]

Description was changed from

==========
Added support for 'spare' file that can be used at startup.

Memory-mapped files make I/O errors into bus errors if something
is wrong with the file and cannot be mapped, including if the file
is sparse and the disk is full.  To counter this, such files are now
always pre-allocated to their full size but this has a cost.

To counter the cost, create a "spare" file some time after startup
that can be used for persistent metrics during the following run,
thus eliminating the cost of pre-allocating that file during browser
startup.

This is especially important on Android where the cost of allocating
the file is significant and yet persisting metrics is the most useful.

Performance tests indicate this reverses both android regressions
but it won't be until real metrics start being collected that an
accurate assessment can be made.

BUG=721806, 694565
==========

to

==========
Added support for 'spare' file that can be used at startup.

Memory-mapped files make I/O errors into bus errors if something
is wrong with the file and cannot be mapped, including if the file
is sparse and the disk is full.  To counter this, such files are now
always pre-allocated to their full size but this has a cost.

To counter the cost, create a "spare" file some time after startup
that can be used for persistent metrics during the following run,
thus eliminating the cost of pre-allocating that file during browser
startup.

This is especially important on Android where the cost of allocating
the file is significant and yet persisting metrics is the most useful.

Performance tests indicate this reverses both android regressions
but it won't be until real metrics start being collected that an
accurate assessment can be made.

BUG=721806, 694565

Review-Url: https://codereview.chromium.org/2888563005
Cr-Commit-Position: refs/heads/master@{#474414}
Committed:
https://chromium.googlesource.com/chromium/src/+/9963f7ea173756bc0b8014ed0dcd...
==========

[commit-bot: I haz the power, 2017-05-24 20:52:54]

Committed patchset #6 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/9963f7ea173756bc0b8014ed0dcd...