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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 1 week ago by bcwhite
Modified:
5 months 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 Review-Url: https://codereview.chromium.org/2888563005 Cr-Commit-Position: refs/heads/master@{#474414} Committed: https://chromium.googlesource.com/chromium/src/+/9963f7ea173756bc0b8014ed0dcd9bd48a6d3598

Patch Set 1 #

Patch Set 2 : fixed build issues #

Patch Set 3 : rebased #

Total comments: 16

Patch Set 4 : addressed review comments by asvitkine #

Patch Set 5 : rebased #

Patch Set 6 : added comment about spare file on non-android #

Unified diffs Side-by-side diffs Delta from patch set Stats (+150 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 4 5 6 chunks +42 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: 59 (49 generated)
bcwhite
5 months ago (2017-05-23 17:08:10 UTC) #34
bcwhite
5 months 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 ...
5 months 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 ...
5 months ago (2017-05-24 17:01:50 UTC) #43
Alexei Svitkine (slow)
lgtm https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_browser_field_trials.cc File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_browser_field_trials.cc#newcode50 chrome/browser/chrome_browser_field_trials.cc:50: constexpr int kSpareFileCreateDelaySeconds = 90; On 2017/05/24 17:01:50, ...
5 months ago (2017-05-24 17:09:53 UTC) #44
bcwhite
https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_browser_field_trials.cc File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/2888563005/diff/120001/chrome/browser/chrome_browser_field_trials.cc#newcode50 chrome/browser/chrome_browser_field_trials.cc:50: constexpr int kSpareFileCreateDelaySeconds = 90; On 2017/05/24 17:09:53, Alexei ...
5 months ago (2017-05-24 18:23:29 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888563005/170001
5 months ago (2017-05-24 19:57:44 UTC) #52
commit-bot: I haz the power
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_presubmit/builds/446582)
5 months ago (2017-05-24 20:06:20 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2888563005/170001
5 months ago (2017-05-24 20:46:25 UTC) #56
commit-bot: I haz the power
5 months ago (2017-05-24 20:52:54 UTC) #59
Message was sent while issue was closed.
Committed patchset #6 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/9963f7ea173756bc0b8014ed0dcd...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa