Refine UMA stats of QuotaTemporaryStorageEvictor

webkit/quota/quota_temporary_storage_evictor.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef WEBKIT_QUOTA_QUOTA_TEMPORARY_STORAGE_EVICTOR_H_
 #define WEBKIT_QUOTA_QUOTA_TEMPORARY_STORAGE_EVICTOR_H_
 #pragma once
 
 #include <map>
 #include <string>
@@ skipping 20 matching lines @@
         : num_errors_on_evicting_origin(0),
           num_errors_on_getting_usage_and_quota(0),
           num_evicted_origins(0),
           num_eviction_rounds(0),
           num_skipped_eviction_rounds(0) {}
     int64 num_errors_on_evicting_origin;
     int64 num_errors_on_getting_usage_and_quota;
     int64 num_evicted_origins;
     int64 num_eviction_rounds;
     int64 num_skipped_eviction_rounds;
+
+    Statistics& operator -=(const Statistics& rhs) {

[kinuko, 2011/08/12 03:57:45]

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...

[tzik, 2011/08/12 07:12:06]

Done.

+      num_errors_on_evicting_origin -= rhs.num_errors_on_evicting_origin;
+      num_errors_on_getting_usage_and_quota -=
+          rhs.num_errors_on_getting_usage_and_quota;
+      num_evicted_origins -= rhs.num_evicted_origins;
+      num_eviction_rounds -= rhs.num_eviction_rounds;
+      num_skipped_eviction_rounds -= rhs.num_skipped_eviction_rounds;
+      return *this;
+    }
+  };
+
+  struct EvictionRoundStatistics {
+    EvictionRoundStatistics()
+        : started(false),
+          amount_of_usage_overage(-1),
+          amount_of_diskspace_shortage(-1),
+          usage_on_beginning_of_round(-1),
+          usage_on_end_of_round(-1),
+          num_evicted_origins(0) {
+    }
+
+    void put_usage_overage(int64 overage) {

[Dai Mikurube (NOT FULLTIME), 2011/08/12 04:19:03]

Maybe "set" looks better?

[tzik, 2011/08/12 07:12:06]

As we chatted, I added |is_initialized| flag. So we no longer need setters.

+      if (amount_of_usage_overage < 0)
+        amount_of_usage_overage = overage;
+    }
+
+    void put_diskspace_shortage(int64 diskspace_shortage) {
+      if (amount_of_diskspace_shortage < 0)
+        amount_of_diskspace_shortage = diskspace_shortage;
+    }
+
+    void put_usage(int64 usage) {
+      if (usage_on_beginning_of_round < 0)
+        usage_on_beginning_of_round = usage;
+      usage_on_end_of_round = usage;
+    }
+
+    bool started;
+
+    base::Time start_time;
+    int64 amount_of_usage_overage;
+    int64 amount_of_diskspace_shortage;

[Dai Mikurube (NOT FULLTIME), 2011/08/12 04:19:03]

In my understanding, overage and shortage include the meaning of "amount".  Just
usage_overage (maybe in_this_round) might sound better.

[tzik, 2011/08/12 07:12:06]

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?

[Dai Mikurube (NOT FULLTIME), 2011/08/12 07:32:39]

Sounds good.

+
+    int64 usage_on_beginning_of_round;
+    int64 usage_on_end_of_round;
+    int64 num_evicted_origins;

[Dai Mikurube (NOT FULLTIME), 2011/08/12 04:19:03]

Adding "in_round" could be better though it looks like redundant since
Statistics has the same variable.

[tzik, 2011/08/12 07:12:06]

Done.

   };
 
   QuotaTemporaryStorageEvictor(
       QuotaEvictionHandler* quota_eviction_handler,
       int64 interval_ms);
   virtual ~QuotaTemporaryStorageEvictor();
 
   void GetStatistics(std::map<std::string, int64>* statistics);
-  void ReportHistogram();
+  void ReportPerRoundHistogram();
+  void ReportPerHourHistogram();
   void Start();
 
  private:
   friend class QuotaTemporaryStorageEvictorTest;
 
   void StartEvictionTimerWithDelay(int delay_ms);
   void ConsiderEviction();
   void OnGotUsageAndQuotaForEviction(
       QuotaStatusCode status,
       int64 usage,
       int64 unlimited_usage,
       int64 quota,
       int64 available_disk_space);
   void OnGotLRUOrigin(const GURL& origin);
   void OnEvictionComplete(QuotaStatusCode status);
 
+  void OnEvictionRoundStarted();
+  void OnEvictionRoundFinished();
+
   // This is only used for tests.
   void set_repeated_eviction(bool repeated_eviction) {
     repeated_eviction_ = repeated_eviction;
   }
 
   static const double kUsageRatioToStartEviction;
   static const int64 kDefaultMinAvailableDiskSpaceToStartEviction;
   static const int kThresholdOfErrorsToStopEviction;
   static const base::TimeDelta kHistogramReportInterval;
 
   const int64 min_available_disk_space_to_start_eviction_;
 
   // Not owned; quota_eviction_handler owns us.
   QuotaEvictionHandler* quota_eviction_handler_;
 
-  Statistics statistics_;
+  Statistics statistics_, reported_hourly_statistics_;

[Dai Mikurube (NOT FULLTIME), 2011/08/12 04:19:03]

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_*"?

[tzik, 2011/08/12 07:12:06]

Done.

+  EvictionRoundStatistics round_statistics_;
+  base::Time time_of_last_nonskipped_round_;

[Dai Mikurube (NOT FULLTIME), 2011/08/12 04:19:03]

Does this "time" point the beginning of last non-skipped round? or the end of
the round?

[tzik, 2011/08/12 07:12:06]

Done.

+  base::Time time_of_end_of_last_round_;
 
   int64 interval_ms_;
   bool repeated_eviction_;
-  int num_evicted_origins_in_round_;
-
-  int64 usage_on_beginning_of_round_;
-  base::Time time_of_beginning_of_round_;
-  base::Time time_of_end_of_last_round_;
 
   base::OneShotTimer<QuotaTemporaryStorageEvictor> eviction_timer_;
   base::RepeatingTimer<QuotaTemporaryStorageEvictor> histogram_timer_;
 
   base::ScopedCallbackFactory<QuotaTemporaryStorageEvictor> callback_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(QuotaTemporaryStorageEvictor);
 };
 
 }  // namespace quota
 
 #endif  // WEBKIT_QUOTA_QUOTA_TEMPORARY_STORAGE_EVICTOR_H_