Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7582027:
Refine UMA stats of QuotaTemporaryStorageEvictor (Closed)
|
Created:
9 years, 4 months ago by tzik Modified:
9 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefine UMA stats of QuotaTemporaryStorageEvictor
BUG=86993
TEST='QuotaTemporaryStorageEvictorTest.*'
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96934
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 2
Patch Set 4 : '' #
Total comments: 13
Patch Set 5 : '' #
Total comments: 2
Patch Set 6 : '' #
Messages
Total messages: 14 (0 generated)
Hi. kinuko-san, mikurube-san. I'm refining and cleaning up the UMA statistics around Evictor. In this patch, I split EvictedBytesPerRound to AmountOfOverageOfTemporaryGlobalStorage and AmountOfShortageOfDiskspace. Also, previously when we encountered a error on a eviction round, we aborted the round and started next eviction round. I count them as a end of eviction round in this patchset. Could you review the patch?
Hi, Tsuiki-san, Could you change my e-mail address to dmikurube@chromium.org? 2011/8/8 <tzik@chromium.org> > Reviewers: dmikurube, kinuko, > > Message: > Hi. kinuko-san, mikurube-san. > > I'm refining and cleaning up the UMA statistics around Evictor. > In this patch, I split EvictedBytesPerRound to > AmountOfOverageOfTemporaryGlob**alStorage and AmountOfShortageOfDiskspace. > > Also, previously when we encountered a error on a eviction round, we > aborted the > round and started next eviction round. > I count them as a end of eviction round in this patchset. > > Could you review the patch? > > Description: > Refine UMA stats of QuotaTemporaryStorageEvictor > > > BUG=86993 > TEST='**QuotaTemporaryStorageEvictorTe**st.*' > > > Please review this at http://codereview.chromium.**org/7582027/<http://codereview.chromium.org/7582... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M webkit/quota/quota_temporary_**storage_evictor.h > M webkit/quota/quota_temporary_**storage_evictor.cc > M webkit/quota/quota_temporary_**storage_evictor_unittest.cc > > > -- Dai MIKURUBE dmikurube@acm.org
Oops! I missed. Thanks. On 2011/08/08 15:01:32, Dai (Gmail) Mikurube wrote: > Hi, Tsuiki-san, > > Could you change my e-mail address to dmikurube@chromium.org? > > 2011/8/8 <mailto:tzik@chromium.org> > > > Reviewers: dmikurube, kinuko, > > > > Message: > > Hi. kinuko-san, mikurube-san. > > > > I'm refining and cleaning up the UMA statistics around Evictor. > > In this patch, I split EvictedBytesPerRound to > > AmountOfOverageOfTemporaryGlob**alStorage and AmountOfShortageOfDiskspace. > > > > Also, previously when we encountered a error on a eviction round, we > > aborted the > > round and started next eviction round. > > I count them as a end of eviction round in this patchset. > > > > Could you review the patch? > > > > Description: > > Refine UMA stats of QuotaTemporaryStorageEvictor > > > > > > BUG=86993 > > TEST='**QuotaTemporaryStorageEvictorTe**st.*' > > > > > > Please review this at > http://codereview.chromium.**org/7582027/%3Chttp://codereview.chromium.org/75...> > > > > SVN Base: > svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > > > Affected files: > > M webkit/quota/quota_temporary_**storage_evictor.h > > M webkit/quota/quota_temporary_**storage_evictor.cc > > M webkit/quota/quota_temporary_**storage_evictor_unittest.cc > > > > > > > > > -- > Dai MIKURUBE > mailto:dmikurube@acm.org
Mikurube-san, would you be able to take a look at this one? On 2011/08/09 01:18:02, tzik wrote: > Oops! I missed. Thanks. > > On 2011/08/08 15:01:32, Dai (Gmail) Mikurube wrote: > > Hi, Tsuiki-san, > > > > Could you change my e-mail address to dmikurube@chromium.org? > > > > 2011/8/8 <mailto:tzik@chromium.org> > > > > > Reviewers: dmikurube, kinuko, > > > > > > Message: > > > Hi. kinuko-san, mikurube-san. > > > > > > I'm refining and cleaning up the UMA statistics around Evictor. > > > In this patch, I split EvictedBytesPerRound to > > > AmountOfOverageOfTemporaryGlob**alStorage and AmountOfShortageOfDiskspace. > > > > > > Also, previously when we encountered a error on a eviction round, we > > > aborted the > > > round and started next eviction round. > > > I count them as a end of eviction round in this patchset. > > > > > > Could you review the patch? > > > > > > Description: > > > Refine UMA stats of QuotaTemporaryStorageEvictor > > > > > > > > > BUG=86993 > > > TEST='**QuotaTemporaryStorageEvictorTe**st.*' > > > > > > > > > Please review this at > > > http://codereview.chromium.**org/7582027/%253Chttp://codereview.chromium.org/...> > > > > > > SVN Base: > > > svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > > > > > Affected files: > > > M webkit/quota/quota_temporary_**storage_evictor.h > > > M webkit/quota/quota_temporary_**storage_evictor.cc > > > M webkit/quota/quota_temporary_**storage_evictor_unittest.cc > > > > > > > > > > > > > > > -- > > Dai MIKURUBE > > mailto:dmikurube@acm.org
A short comment. http://codereview.chromium.org/7582027/diff/5001/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7582027/diff/5001/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:97: statistics_ = Statistics(); Values in statistics_ are used not only for histogram but also for error handling and quota-internals. Computing delta in an hour looks like better than initializing statistics_.
http://codereview.chromium.org/7582027/diff/5001/webkit/quota/quota_temporary... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7582027/diff/5001/webkit/quota/quota_temporary... webkit/quota/quota_temporary_storage_evictor.cc:97: statistics_ = Statistics(); On 2011/08/11 09:17:19, Dai Mikurube wrote: > Values in statistics_ are used not only for histogram but also for error > handling and quota-internals. Computing delta in an hour looks like better than > initializing statistics_. Done.
http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:42: Statistics& operator -=(const Statistics& rhs) { As we chatted locally, unfortunately operator overloading is in general not recommended in the style guide.. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overl...
Thank you, Tsuiki-san! The logic is looking like good to me. Some nits. http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:63: void put_usage_overage(int64 overage) { Maybe "set" looks better? http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:83: int64 amount_of_diskspace_shortage; In my understanding, overage and shortage include the meaning of "amount". Just usage_overage (maybe in_this_round) might sound better. http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:87: int64 num_evicted_origins; Adding "in_round" could be better though it looks like redundant since Statistics has the same variable. http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:132: Statistics statistics_, reported_hourly_statistics_; Declare them in two lines. Does reported_hourly_statistics_ mean the accumulated statistics until 1 hour before? "hourly statistics" sounds like statistics in an hour. How about simply "last_statistics_" or "previous_*"? http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:134: base::Time time_of_last_nonskipped_round_; Does this "time" point the beginning of last non-skipped round? or the end of the round?
http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:42: Statistics& operator -=(const Statistics& rhs) { On 2011/08/12 03:57:45, kinuko wrote: > As we chatted locally, unfortunately operator overloading is in general not > recommended in the style guide.. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overl... Done. http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:63: void put_usage_overage(int64 overage) { On 2011/08/12 04:19:03, Dai Mikurube wrote: > Maybe "set" looks better? As we chatted, I added |is_initialized| flag. So we no longer need setters. http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:83: int64 amount_of_diskspace_shortage; On 2011/08/12 04:19:03, Dai Mikurube wrote: > In my understanding, overage and shortage include the meaning of "amount". Just > usage_overage (maybe in_this_round) might sound better. Done. I feel, "in this round" sounds as if the shortage/overage were made in this round. How about using "at this round" like this? http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:87: int64 num_evicted_origins; On 2011/08/12 04:19:03, Dai Mikurube wrote: > Adding "in_round" could be better though it looks like redundant since > Statistics has the same variable. Done. http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:132: Statistics statistics_, reported_hourly_statistics_; On 2011/08/12 04:19:03, Dai Mikurube wrote: > Declare them in two lines. > > Does reported_hourly_statistics_ mean the accumulated statistics until 1 hour > before? "hourly statistics" sounds like statistics in an hour. > > How about simply "last_statistics_" or "previous_*"? Done. http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:134: base::Time time_of_last_nonskipped_round_; On 2011/08/12 04:19:03, Dai Mikurube wrote: > Does this "time" point the beginning of last non-skipped round? or the end of > the round? Done.
LGTM with a nit. http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.h (right): http://codereview.chromium.org/7582027/diff/10001/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.h:83: int64 amount_of_diskspace_shortage; On 2011/08/12 07:12:06, tzik wrote: > On 2011/08/12 04:19:03, Dai Mikurube wrote: > > In my understanding, overage and shortage include the meaning of "amount". > Just > > usage_overage (maybe in_this_round) might sound better. > > Done. > I feel, "in this round" sounds as if the shortage/overage were made in this > round. > How about using "at this round" like this? Sounds good. http://codereview.chromium.org/7582027/diff/10002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7582027/diff/10002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:89: Statistics stats(statistics_); Can we have more appropriate name for this variable? (like delta, difference, or in hour...)
if it looks good to mikurube-san it'd lgtm.
Thanks. If there's no problem, I'll commit it after try. http://codereview.chromium.org/7582027/diff/10002/webkit/quota/quota_temporar... File webkit/quota/quota_temporary_storage_evictor.cc (right): http://codereview.chromium.org/7582027/diff/10002/webkit/quota/quota_temporar... webkit/quota/quota_temporary_storage_evictor.cc:89: Statistics stats(statistics_); On 2011/08/12 07:32:39, Dai Mikurube wrote: > Can we have more appropriate name for this variable? (like delta, difference, > or in hour...) Done.
On 2011/08/16 04:31:54, tzik wrote: > Thanks. > > If there's no problem, I'll commit it after try. > > http://codereview.chromium.org/7582027/diff/10002/webkit/quota/quota_temporar... > File webkit/quota/quota_temporary_storage_evictor.cc (right): > > http://codereview.chromium.org/7582027/diff/10002/webkit/quota/quota_temporar... > webkit/quota/quota_temporary_storage_evictor.cc:89: Statistics > stats(statistics_); > On 2011/08/12 07:32:39, Dai Mikurube wrote: > > Can we have more appropriate name for this variable? (like delta, difference, > > or in hour...) > > Done. Still LGTM. Thanks!
Change committed as 96934 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
