Record resource scheduler UMA

content/browser/loader/resource_scheduler.cc

Index: content/browser/loader/resource_scheduler.cc
diff --git a/content/browser/loader/resource_scheduler.cc b/content/browser/loader/resource_scheduler.cc
index 774f698c066db9ea37c4ba26caef154d2667ece8..0f4048919472b0e99c68db21b0c4cbd2cf88ec84 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -227,6 +227,7 @@ class ResourceScheduler::ScheduledResourceRequest : public ResourceThrottle {
         scheduler_(scheduler),
         priority_(priority),
         fifo_ordering_(0),
+        delayable_requests_in_flight_(0u),
         host_port_pair_(net::HostPortPair::FromURL(request->url())),
         weak_ptr_factory_(this) {
     DCHECK(!request_->GetUserData(kUserDataKey));
@@ -234,6 +235,16 @@ class ResourceScheduler::ScheduledResourceRequest : public ResourceThrottle {
   }
 
   ~ScheduledResourceRequest() override {
+    if ((attributes_ & kAttributeLayoutBlocking) == kAttributeLayoutBlocking) {
+      UMA_HISTOGRAM_COUNTS_100(
+          "ResourceScheduler.PeakDelayableRequestsInFlight.LayoutBlocking",
+          delayable_requests_in_flight_);
+    }
+    if (!((attributes_ & kAttributeDelayable) == kAttributeDelayable)) {
+      UMA_HISTOGRAM_COUNTS_100(
+          "ResourceScheduler.PeakDelayableRequestsInFlight.NonDelayable",
+          delayable_requests_in_flight_);
+    }
     request_->RemoveUserData(kUserDataKey);
     scheduler_->RemoveRequest(this);
   }
@@ -274,6 +285,11 @@ class ResourceScheduler::ScheduledResourceRequest : public ResourceThrottle {
     ready_ = true;
   }
 
+  void UpdateDelayableRequestsInFlight(size_t delayable_requests_in_flight) {
+    delayable_requests_in_flight_ =
+        std::max(delayable_requests_in_flight_, delayable_requests_in_flight);
+  }
+
   void set_request_priority_params(const RequestPriorityParams& priority) {
     priority_ = priority;
   }
@@ -328,6 +344,10 @@ class ResourceScheduler::ScheduledResourceRequest : public ResourceThrottle {
   ResourceScheduler* scheduler_;
   RequestPriorityParams priority_;
   uint32_t fifo_ordering_;
+
+  // True if a delayable request was in-flight at the same when |this| was
+  // in-flight.

[Randy Smith (Not in Mondays), 2017/05/21 21:43:56]

I think the comment's out of date?  Sounds like it's talking about a boolean.

[tbansal1, 2017/05/25 01:07:46]

Thanks. Fixed.

+  size_t delayable_requests_in_flight_;
   // Cached to excessive recomputation in ShouldKeepSearching.
   const net::HostPortPair host_port_pair_;
 
@@ -494,9 +514,55 @@ class ResourceScheduler::Client {
     YIELD_SCHEDULER
   };
 
+  // Records the metrics related to number of requests in flight.
+  void RecordRequestCountMetrics() const {
+    UMA_HISTOGRAM_COUNTS_100("ResourceScheduler.RequestsCount.All",
+                             in_flight_requests_.size());
+    UMA_HISTOGRAM_COUNTS_100("ResourceScheduler.RequestsCount.Delayable",
+                             in_flight_delayable_count_);
+
+    size_t non_delayable_in_flight_count =
+        in_flight_requests_.size() - in_flight_delayable_count_;
+
+    UMA_HISTOGRAM_COUNTS_100("ResourceScheduler.RequestsCount.NonDelayable",
+                             non_delayable_in_flight_count);

[Randy Smith (Not in Mondays), 2017/05/21 21:43:56]

I don't have strong objections, but histograms do cost (memory, bandwidth).  Do
you need a histogram that duplicates the information available in other
histograms?  Why?

[tbansal1, 2017/05/25 01:07:46]

I am not sure if this is duplicate of some other histogram. This is recording
the difference between two values, however it is not possible to compute this
from the data from the other 2 histograms.

[Randy Smith (Not in Mondays), 2017/05/25 17:40:12]

Good point, though it's possible with some serious dremeling.  

Ok, I'll just push back a little bit more than yield: Could you confirm for
yourself that you'll actually use all three of these histograms, rather just
needing the data you can get from two of them in the third context (e.g. average
for the distribution)?  If so, I'm good with them.

[tbansal1, 2017/05/31 06:31:21]

Yes, for now I think it is useful to record the difference separately so that it
can be analyzed as a function different network qualities.

+    UMA_HISTOGRAM_COUNTS_100(
+        "ResourceScheduler.RequestsCount.TotalLayoutBlocking",
+        total_layout_blocking_count_);
+
+    if (total_layout_blocking_count_ > 0) {
+      UMA_HISTOGRAM_COUNTS_100(
+          "ResourceScheduler.RequestsCount.DelayableWhenLayoutBlocking",
+          in_flight_delayable_count_);
+    }
+
+    if (non_delayable_in_flight_count > 0) {
+      UMA_HISTOGRAM_COUNTS_100(
+          "ResourceScheduler.RequestsCount.DelayableWhenNonDelayable",
+          in_flight_delayable_count_);
+    }
+  }
+
   void InsertInFlightRequest(ScheduledResourceRequest* request) {
     in_flight_requests_.insert(request);
     SetRequestAttributes(request, DetermineRequestAttributes(request));
+    RecordRequestCountMetrics();

[Randy Smith (Not in Mondays), 2017/05/21 21:43:56]

I remain uncertain that you're going to get good statistics from this.  I'm
willing to defer to the histograms.xml owner if you'd like (if you specifically
raise the issue with them) but to me, this means that you're going to way
overcount situations in the above histograms when you've got a lot of requests,
and undercount when you have fewer.  I'd instead suggest that you have
RecordRequestCountMetrics on a timer that goes off ever n ms while requests are
in-flight and records these metrics at those points.  Alternatively, you could
do something that integrates the values of the various above metrics over times
in between insertions of in flight requests (though you'll need something that
captures when the last in flight request completes).  But recording histograms
on what is, as best I can tell, the statistically arbitrary point of when a new
request gets inserted still strikes me as wrong.

[tbansal1, 2017/05/25 01:07:46]

I agree with the spirit of this concern. The goal of these metrics is to compare
different resource scheduling algorithms, and see how frequently different
algorithms allow a certain number of requests to go in flight. 

One problem with using time-based sampling is that different algorithms may
record different number of samples (if we restrict taking the sample to only
when there are at least 1 request in-flight). This would make it difficult to
compare different algorithms. Recording at request insertion/deletion has the
nice property that changing the scheduling algorithm does not change the number
of samples.

I would agree that this metric has problems (e.g., it is oblivious of the
duration of different states). A more comprehensive way (but with more histogram
bloat) of recording this might be to use histograms of the format:
ResourceScheduler.Count.{All|Delayable|NonDelayble|TotalLayoutBlocking|DelayableWhenLayoutBlocking|DelayableWhenNonDelayable}.{1..N}.
This would require ~6*N histograms.

Then every time a request is inserted or removed, we record for how long (=time
duration) there were X requests in flight. e.g., if there were 3 requests in
flight for 30 seconds, we record a 30 second sample in the histogram:
ResourceScheduler.Count.All.3.
Similarly, we record 5 more samples.

I am hesitant to go with that approach since it is not clear if we really need
to record the time dimension.

Thoughts?

[Randy Smith (Not in Mondays), 2017/05/25 17:40:12]

My problem is that I'm not a statistician, though I'm clearly trying to play one
on TV :-}.  If we take the goal as being only comparing scheduling algorithms, I
think a metric of "fewer at insertion of in flight request is better" is ok, and
this is a fine way to get that metric.  I'd recommending skipping the second
call on erasing in flights requests, though--I don't think we care much about
that value.  

An alternative metric to try and get would be "average number of in-flight
requests for the duration of any given request".  I'm inclined to think that's a
better metric than the value at insertion, as requests will be interfering with
each other for the full overlap of their lifetimes.  I'm happy to yield to your
preference.

If you'd prefer that metric, I don't think it would be hard to compute, and it
would only take one (per type being counted) histogram.  What you'd do is, for
each request, keep three variables: The amount of time the request has been
oustanding for, the average number of other requests active during that time,
and the last time the average was computed.  Whenever there was a change
(erasure or addition) of a request that wasn't the request in question, you
would recompute the average ((average * time outstanding + time since last
computed * other requests active since that time)/(time outstanding + time since
last compued)) and the time outstanding.  When the request completes, the
average gets entered into the histogram(s).

But I'm willing to accept what you currently have if you prefer it to that
(modulo removing the recording from the EraseInFlightRequest() method--if you
think that's a good idea you'll need to pitch it to me).

[tbansal1, 2017/05/31 06:31:21]

Done.  
Average is a fine metric but then I would argue for recording average of
different types of requests. For example, average of count of requests of type X
when a request of type Y is in flight.
where X,Y belong to {delayable, non-delayble, layout blocking};
This is because we do not want to lump metrics from different types of requests
in the same histogram.
Done.

+
+    if (RequestAttributesAreSet(request->attributes(), kAttributeDelayable)) {
+      // Notify all in-flight with the new count of in-flight delayable
+      // requests.
+      for (RequestSet::const_iterator it = in_flight_requests_.begin();
+           it != in_flight_requests_.end(); ++it) {
+        (*it)->UpdateDelayableRequestsInFlight(in_flight_delayable_count_);
+      }
+    }
+
+    if (RequestAttributesAreSet(request->attributes(),
+                                kAttributeLayoutBlocking) ||
+        !RequestAttributesAreSet(request->attributes(), kAttributeDelayable)) {
+      // |request| is either a layout blocking or a non-delayable request.
+      request->UpdateDelayableRequestsInFlight(in_flight_delayable_count_);
+    }
   }
 
   void EraseInFlightRequest(ScheduledResourceRequest* request) {
@@ -504,12 +570,14 @@ class ResourceScheduler::Client {
     DCHECK_EQ(1u, erased);
     // Clear any special state that we were tracking for this request.
     SetRequestAttributes(request, kAttributeNone);
+    RecordRequestCountMetrics();
   }
 
   void ClearInFlightRequests() {
     in_flight_requests_.clear();
     in_flight_delayable_count_ = 0;
     total_layout_blocking_count_ = 0;
+    RecordRequestCountMetrics();
   }
 
   size_t CountRequestsWithAttributes(