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

Issue 1083603002: Add histograms to record the number of writes to the prefs file (Closed)

Created:
5 years, 8 months ago by raymes
Modified:
5 years, 7 months ago
CC:
chromium-reviews, erikwright+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add histograms to record the number of writes to the prefs file This adds histograms that records how often we write to the prefs file. The number of writes is recorded for every 5 minute interval of time spent in Chrome. To avoid doing any work when Chrome is idle and not recording any writes, we only record metrics at the point when a write occurs. Whenever a write occurs we look to see if the last write occurred more than 5mins ago. If it did, we report the write count for the previous period of time. The histogram is called: Settings.JsonDataWriteCount.<pref filename> e.g. Settings.JsonDataWriteCount.Local_State This is in a similar vein to Settings.JsonDataReadSizeKilobytes.* BUG=476800 Committed: https://crrev.com/bfb910ab2937285271031b257f3ba248afff6fdc Cr-Commit-Position: refs/heads/master@{#327446}

Patch Set 1 #

Patch Set 2 : #

Total comments: 9

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 1

Patch Set 6 : #

Patch Set 7 : #

Total comments: 6

Patch Set 8 : #

Total comments: 10

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+330 lines, -3 lines) Patch
M base/metrics/statistics_recorder.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M base/metrics/statistics_recorder.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -1 line 0 comments Download
M base/prefs/json_pref_store.h View 1 2 3 4 5 6 7 8 9 10 4 chunks +67 lines, -0 lines 0 comments Download
M base/prefs/json_pref_store.cc View 1 2 3 4 5 6 7 8 9 5 chunks +96 lines, -2 lines 0 comments Download
M base/prefs/json_pref_store_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +157 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 2 chunks +9 lines, -0 lines 0 comments Download

Messages

Total messages: 39 (11 generated)
raymes
Mattias: I would like to write a test for this but I wanted to get ...
5 years, 8 months ago (2015-04-14 02:42:35 UTC) #2
Mattias Nissler (ping if slow)
Took a look at the code, but then ended up pondering high-level questions again that ...
5 years, 8 months ago (2015-04-14 11:58:27 UTC) #4
raymes
On 2015/04/14 11:58:27, Mattias Nissler wrote: > Took a look at the code, but then ...
5 years, 8 months ago (2015-04-15 01:29:01 UTC) #5
benwells
On 2015/04/15 01:29:01, raymes wrote: > On 2015/04/14 11:58:27, Mattias Nissler wrote: > > Took ...
5 years, 8 months ago (2015-04-15 01:45:14 UTC) #6
benwells
On 2015/04/15 01:45:14, benwells wrote: > On 2015/04/15 01:29:01, raymes wrote: > > On 2015/04/14 ...
5 years, 8 months ago (2015-04-15 01:48:37 UTC) #7
raymes
That sounds reasonable to me. Alternatively: what if we accumulate writes and report them every ...
5 years, 8 months ago (2015-04-15 06:18:19 UTC) #8
Alexei Svitkine (slow)
https://codereview.chromium.org/1083603002/diff/20001/base/prefs/json_pref_store.cc File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/1083603002/diff/20001/base/prefs/json_pref_store.cc#newcode536 base/prefs/json_pref_store.cc:536: ++write_count_; Can you add a DCHECK to ensure this ...
5 years, 8 months ago (2015-04-15 16:41:23 UTC) #9
benwells
On 2015/04/15 06:18:19, raymes wrote: > That sounds reasonable to me. > > Alternatively: what ...
5 years, 8 months ago (2015-04-16 00:41:55 UTC) #10
raymes
I think I was more concerned about having a metric which we can reason about ...
5 years, 8 months ago (2015-04-16 01:00:04 UTC) #11
raymes
Also I think writes/5mins would only be a little harder to implement (certainly not as ...
5 years, 8 months ago (2015-04-16 01:05:04 UTC) #12
Mattias Nissler (ping if slow)
On 2015/04/16 01:05:04, raymes wrote: > Also I think writes/5mins would only be a little ...
5 years, 8 months ago (2015-04-16 14:07:02 UTC) #13
raymes
I implemented thea idea of recording the writes at every 5 minute interval. This means ...
5 years, 8 months ago (2015-04-20 04:17:58 UTC) #14
Mattias Nissler (ping if slow)
LGTM w/ nit https://codereview.chromium.org/1083603002/diff/80001/base/prefs/json_pref_store.cc File base/prefs/json_pref_store.cc (right): https://codereview.chromium.org/1083603002/diff/80001/base/prefs/json_pref_store.cc#newcode197 base/prefs/json_pref_store.cc:197: write_count_histogram_.reset(new WriteCountHistogram( looks like there's no ...
5 years, 8 months ago (2015-04-20 09:43:48 UTC) #15
Alexei Svitkine (slow)
Not sure if you missed my comments on the earlier patch set, doesn't seem like ...
5 years, 8 months ago (2015-04-20 15:49:04 UTC) #16
raymes
asvitkine: ptal I'll write a test before landing this though. Thanks! https://codereview.chromium.org/1083603002/diff/20001/base/prefs/json_pref_store.cc File base/prefs/json_pref_store.cc (right): ...
5 years, 8 months ago (2015-04-21 07:22:38 UTC) #17
raymes
I just added a test :) PTAL Thanks! Raymes
5 years, 8 months ago (2015-04-24 07:15:51 UTC) #18
Alexei Svitkine (slow)
https://codereview.chromium.org/1083603002/diff/120001/base/metrics/statistics_recorder.h File base/metrics/statistics_recorder.h (right): https://codereview.chromium.org/1083603002/diff/120001/base/metrics/statistics_recorder.h#newcode38 base/metrics/statistics_recorder.h:38: static void InitializeForTest(); This sounds like Reset rather than ...
5 years, 8 months ago (2015-04-24 16:34:47 UTC) #19
raymes
https://codereview.chromium.org/1083603002/diff/120001/base/metrics/statistics_recorder.h File base/metrics/statistics_recorder.h (right): https://codereview.chromium.org/1083603002/diff/120001/base/metrics/statistics_recorder.h#newcode38 base/metrics/statistics_recorder.h:38: static void InitializeForTest(); On 2015/04/24 16:34:47, Alexei Svitkine wrote: ...
5 years, 8 months ago (2015-04-27 03:27:54 UTC) #20
Alexei Svitkine (slow)
https://codereview.chromium.org/1083603002/diff/140001/base/metrics/statistics_recorder.h File base/metrics/statistics_recorder.h (right): https://codereview.chromium.org/1083603002/diff/140001/base/metrics/statistics_recorder.h#newcode36 base/metrics/statistics_recorder.h:36: // Reset the StatisticsRecorder system for testing. All existing ...
5 years, 8 months ago (2015-04-27 20:21:20 UTC) #21
raymes
Thanks Alexei! https://codereview.chromium.org/1083603002/diff/140001/base/metrics/statistics_recorder.h File base/metrics/statistics_recorder.h (right): https://codereview.chromium.org/1083603002/diff/140001/base/metrics/statistics_recorder.h#newcode36 base/metrics/statistics_recorder.h:36: // Reset the StatisticsRecorder system for testing. ...
5 years, 8 months ago (2015-04-28 00:32:58 UTC) #22
Alexei Svitkine (slow)
LGTM!
5 years, 7 months ago (2015-04-28 14:45:23 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1083603002/180001
5 years, 7 months ago (2015-04-29 00:20:43 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/33750)
5 years, 7 months ago (2015-04-29 01:16:22 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1083603002/220001
5 years, 7 months ago (2015-04-29 01:38:48 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/19653)
5 years, 7 months ago (2015-04-29 02:27:35 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1083603002/240001
5 years, 7 months ago (2015-04-29 03:46:43 UTC) #37
commit-bot: I haz the power
Committed patchset #12 (id:240001)
5 years, 7 months ago (2015-04-29 07:43:27 UTC) #38
commit-bot: I haz the power
5 years, 7 months ago (2015-04-29 07:44:29 UTC) #39
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/bfb910ab2937285271031b257f3ba248afff6fdc
Cr-Commit-Position: refs/heads/master@{#327446}

Powered by Google App Engine
This is Rietveld 408576698