Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(166)

Issue 999623002: metrics/base: log whether drives have seek penalties. (Closed)

Created:
5 years, 9 months ago by Dan Beam
Modified:
5 years, 9 months ago
CC:
chromium-reviews, erikwright+watch_chromium.org, asvitkine+watch_chromium.org, Lei Zhang
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

metrics/base: log whether drives have seek penalties. Only on Win 7+ for now. R=rvargas@chromium.org,mpearson@chromium.org BUG=463209 Committed: https://crrev.com/64c37847c51804dc237875e691c8ff95b6d25edd Cr-Commit-Position: refs/heads/master@{#321056}

Patch Set 1 #

Patch Set 2 : nit #

Patch Set 3 : FILE thread #

Patch Set 4 : nits #

Total comments: 30

Patch Set 5 : slightly after rush hour #

Total comments: 8

Patch Set 6 : whoops #

Patch Set 7 : base_unittests #

Patch Set 8 : OS_WIN #

Patch Set 9 : indent #

Total comments: 18

Patch Set 10 : asvitkine@ review #

Total comments: 1

Patch Set 11 : asvitkine@ review #

Total comments: 3

Patch Set 12 : defer differently #

Patch Set 13 : remove debugging #

Patch Set 14 : restore review-worthy code #

Patch Set 15 : GetLastError #

Patch Set 16 : remove unit test #

Patch Set 17 : add back crash test #

Patch Set 18 : DONT_ASSERT_ANYTHING #

Unified diffs Side-by-side diffs Delta from patch set Stats (+249 lines, -13 lines) Patch
M base/sys_info.h View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M base/sys_info_posix.cc View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M base/sys_info_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +7 lines, -0 lines 0 comments Download
M base/sys_info_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +44 lines, -9 lines 0 comments Download
M chrome/browser/metrics/chrome_metrics_service_client.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/metrics/chrome_metrics_service_client.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +17 lines, -3 lines 0 comments Download
A chrome/browser/metrics/drive_metrics_provider.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +74 lines, -0 lines 0 comments Download
A chrome/browser/metrics/drive_metrics_provider.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +79 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M components/metrics/proto/system_profile.proto View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +13 lines, -1 line 0 comments Download

Messages

Total messages: 28 (11 generated)
Dan Beam
rvargas@ for base/ (mark@ mentioned you might be a good reviewer for windows-y code) mpearson@ ...
5 years, 9 months ago (2015-03-12 01:26:43 UTC) #2
Mark P
On 2015/03/12 01:26:43, Dan Beam wrote: > rvargas@ for base/ (mark@ mentioned you might be ...
5 years, 9 months ago (2015-03-12 16:30:01 UTC) #3
Alexei Svitkine (slow)
I can review the metrics bits.
5 years, 9 months ago (2015-03-12 16:31:27 UTC) #5
Alexei Svitkine (slow)
https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/disk_metrics_provider.cc File chrome/browser/metrics/disk_metrics_provider.cc (right): https://codereview.chromium.org/999623002/diff/60001/chrome/browser/metrics/disk_metrics_provider.cc#newcode43 chrome/browser/metrics/disk_metrics_provider.cc:43: content::BrowserThread::FILE, FROM_HERE, Any reason to use FILE thread explicitly ...
5 years, 9 months ago (2015-03-12 16:41:07 UTC) #7
rvargas (doing something else)
https://codereview.chromium.org/999623002/diff/60001/base/sys_info.cc File base/sys_info.cc (right): https://codereview.chromium.org/999623002/diff/60001/base/sys_info.cc#newcode56 base/sys_info.cc:56: #if !defined(OS_WIN) Given that this would have to be ...
5 years, 9 months ago (2015-03-12 22:01:12 UTC) #8
Dan Beam
let me know how long after "rush hour" we should collect these metrics so I ...
5 years, 9 months ago (2015-03-13 19:31:32 UTC) #10
rvargas (doing something else)
https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc File base/sys_info_win.cc (right): https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#newcode74 base/sys_info_win.cc:74: if (!path.IsAbsolute() || win::GetVersion() < win::VERSION_WIN7) I have not ...
5 years, 9 months ago (2015-03-13 21:03:09 UTC) #11
Dan Beam
https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc File base/sys_info_win.cc (right): https://codereview.chromium.org/999623002/diff/100001/base/sys_info_win.cc#newcode74 base/sys_info_win.cc:74: if (!path.IsAbsolute() || win::GetVersion() < win::VERSION_WIN7) On 2015/03/13 21:03:09, ...
5 years, 9 months ago (2015-03-13 21:15:51 UTC) #14
rvargas (doing something else)
base lgtm on ps9.
5 years, 9 months ago (2015-03-14 00:51:06 UTC) #15
Alexei Svitkine (slow)
https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/drive_metrics_provider.cc File chrome/browser/metrics/drive_metrics_provider.cc (right): https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/drive_metrics_provider.cc#newcode20 chrome/browser/metrics/drive_metrics_provider.cc:20: void TestSeekPenalty(int path_service_key, Nit: Test* can be confusing when ...
5 years, 9 months ago (2015-03-16 16:49:36 UTC) #16
Dan Beam
https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/drive_metrics_provider.cc File chrome/browser/metrics/drive_metrics_provider.cc (right): https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/drive_metrics_provider.cc#newcode20 chrome/browser/metrics/drive_metrics_provider.cc:20: void TestSeekPenalty(int path_service_key, On 2015/03/16 16:49:36, Alexei Svitkine (slow) ...
5 years, 9 months ago (2015-03-16 19:20:24 UTC) #19
Alexei Svitkine (slow)
https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/drive_metrics_provider.cc File chrome/browser/metrics/drive_metrics_provider.cc (right): https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/drive_metrics_provider.cc#newcode55 chrome/browser/metrics/drive_metrics_provider.cc:55: if (!got_metrics_) { On 2015/03/16 19:20:23, Dan Beam wrote: ...
5 years, 9 months ago (2015-03-16 19:29:07 UTC) #20
Dan Beam
https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/drive_metrics_provider.cc File chrome/browser/metrics/drive_metrics_provider.cc (right): https://codereview.chromium.org/999623002/diff/220001/chrome/browser/metrics/drive_metrics_provider.cc#newcode55 chrome/browser/metrics/drive_metrics_provider.cc:55: if (!got_metrics_) { On 2015/03/16 19:29:07, Alexei Svitkine (slow) ...
5 years, 9 months ago (2015-03-16 22:25:01 UTC) #22
Alexei Svitkine (slow)
Thanks, LGTM! Please wait for the internal protobuf review CL to be submitted before committing.
5 years, 9 months ago (2015-03-17 00:15:47 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/999623002/450001
5 years, 9 months ago (2015-03-17 23:47:23 UTC) #26
commit-bot: I haz the power
Committed patchset #18 (id:450001)
5 years, 9 months ago (2015-03-18 02:23:41 UTC) #27
commit-bot: I haz the power
5 years, 9 months ago (2015-03-18 02:24:24 UTC) #28
Message was sent while issue was closed.
Patchset 18 (id:??) landed as
https://crrev.com/64c37847c51804dc237875e691c8ff95b6d25edd
Cr-Commit-Position: refs/heads/master@{#321056}

Powered by Google App Engine
This is Rietveld 408576698