Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week ago by bcwhite
Modified:
44 minutes ago
CC:
chromium-reviews, danakj+watch_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : fixed build issues #

Patch Set 3 : rebased #

Total comments: 15

Patch Set 4 : addressed review comments by asvitkine #

Patch Set 5 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+149 lines, -23 lines) Patch
M base/metrics/persistent_histogram_allocator.h View 1 2 3 2 chunks +21 lines, -6 lines 0 comments Download
M base/metrics/persistent_histogram_allocator.cc View 1 3 chunks +55 lines, -4 lines 0 comments Download
M base/metrics/persistent_histogram_allocator_unittest.cc View 1 2 3 4 chunks +27 lines, -3 lines 0 comments Download
M chrome/browser/chrome_browser_field_trials.cc View 1 2 3 6 chunks +41 lines, -6 lines 0 comments Download
M chrome/browser/metrics/chrome_metrics_service_client.cc View 1 2 2 chunks +4 lines, -4 lines 0 comments Download
M tools/metrics/histograms/enums.xml View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 44 (39 generated)
bcwhite
1 day ago (2017-05-23 17:08:10 UTC) #34
bcwhite
1 day ago (2017-05-23 17:08:18 UTC) #35
Alexei Svitkine (slow)
https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persistent_histogram_allocator.cc File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persistent_histogram_allocator.cc#newcode844 base/metrics/persistent_histogram_allocator.cc:844: File spare_file(temp_spare_path, File::FLAG_CREATE_ALWAYS | Nit: I would have wrapped ...
1 day ago (2017-05-23 17:27:58 UTC) #36
bcwhite
https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persistent_histogram_allocator.cc File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/2888563005/diff/120001/base/metrics/persistent_histogram_allocator.cc#newcode844 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 ...
52 minutes ago (2017-05-24 17:01:50 UTC) #43
Alexei Svitkine (slow)
44 minutes ago (2017-05-24 17:09:53 UTC) #44
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06