Add UMA histograms to AsyncResourceHandler to track content size.

content/browser/loader/async_resource_handler.cc

 // Copyright (c) 2012 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.
 
 #include "content/browser/loader/async_resource_handler.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/command_line.h"
@@ skipping 54 matching lines @@
   static bool did_init = false;
   if (did_init)
     return;
   did_init = true;
 
   GetNumericArg("resource-buffer-size", &kBufferSize);
   GetNumericArg("resource-buffer-min-allocation-size", &kMinAllocationSize);
   GetNumericArg("resource-buffer-max-allocation-size", &kMaxAllocationSize);
 }
 
+enum ExpectedContentSize {

[mmenke, 2017/01/11 20:25:19]

"enum class" should generally be preferred to just enums

[maksims (do not use this acc), 2017/01/12 09:03:03]

invalid operands to binary expression ('content::(anonymous
namespace)::ExpectedContentSize' and 'int')

Macro includes arithmetic operations, which cannot be done on enums.

[mmenke, 2017/01/12 18:04:10]

Ah, right, good point.

+  CORRESPONDS_TO_RESPONSE_BODY = 0,

[mmenke, 2017/01/11 20:25:20]

EQ_RESPONSE_BODY?

[maksims (do not use this acc), 2017/01/13 12:05:18]

Done.

+  LT_RESPONSE_BODY = 1,
+  MT_RESPONSE_BODY = 2,
+  EXPECTED_CONTENT_SIZE,

[mmenke, 2017/01/11 20:25:19]

nit:  EXPECTED_CONTENT_MAX is more common

[maksims (do not use this acc), 2017/01/13 12:05:17]

Done.

+};
+
 }  // namespace
 
 // Used when kOptimizeLoadingIPCForSmallResources is enabled.
 // The instance hooks the buffer allocation of AsyncResourceHandler, and
 // determine if we should use SharedMemory or should inline the data into
 // the IPC message.
 class AsyncResourceHandler::InliningHelper {
  public:
 
   InliningHelper() {}
@@ skipping 107 matching lines @@
 };
 
 AsyncResourceHandler::AsyncResourceHandler(
     net::URLRequest* request,
     ResourceDispatcherHostImpl* rdh)
     : ResourceHandler(request),
       ResourceMessageDelegate(request),
       rdh_(rdh),
       pending_data_count_(0),
       allocation_size_(0),
+      total_read_bytes_(0),
       did_defer_(false),
       has_checked_for_sufficient_resources_(false),
       sent_received_response_msg_(false),
       sent_data_buffer_msg_(false),
       inlining_helper_(new InliningHelper),
       reported_transfer_size_(0) {
   DCHECK(GetRequestInfo()->requester_info()->IsRenderer());
   InitializeResourceBufferConstants();
 }
 
@@ skipping 176 matching lines @@
   first_chunk_read_ = true;
 
   // Return early if InliningHelper handled the received data.
   if (inlining_helper_->SendInlinedDataIfApplicable(
           bytes_read, encoded_data_length, filter,
           GetRequestID()))
     return true;
 
   buffer_->ShrinkLastAllocation(bytes_read);
 
+  total_read_bytes_ += bytes_read;
+
   if (!sent_data_buffer_msg_) {
     base::SharedMemoryHandle handle = base::SharedMemory::DuplicateHandle(
         buffer_->GetSharedMemory().handle());
     if (!base::SharedMemory::IsHandleValid(handle))
       return false;
     filter->Send(new ResourceMsg_SetDataBuffer(
         GetRequestID(), handle, buffer_->GetSharedMemory().mapped_size(),
         filter->peer_pid()));
     sent_data_buffer_msg_ = true;
   }
@@ skipping 136 matching lines @@
     UMA_HISTOGRAM_CUSTOM_COUNTS(
         "Net.ResourceLoader.ResponseStartToEnd.LT_512kB",
         elapsed_time, 1, 100000, 100);
   } else {
     UMA_HISTOGRAM_CUSTOM_COUNTS(
         "Net.ResourceLoader.ResponseStartToEnd.Over_512kB",
         elapsed_time, 1, 100000, 100);
   }
 
   inlining_helper_->RecordHistogram(elapsed_time);
+
+  // Record if content size is known in advance.

[mmenke, 2017/01/11 20:25:20]

nit:  is -> was (Since this is recorded at the end of a request).

[maksims (do not use this acc), 2017/01/13 12:05:18]

Done.

+  int64_t expected_content_size = request()->GetExpectedContentSize();
+  if (expected_content_size > 0) {

[mmenke, 2017/01/11 20:25:19]

nit:  >= 0 (We use -1 to mean unknown, 0 means the body is known to be empty)

[maksims (do not use this acc), 2017/01/13 12:05:17]

Done.

+    UMA_HISTOGRAM_BOOLEAN(
+        "Net.ResourceLoader.ExpectedContentSize.IsKnown",
+        true);
+    // Compare response body size to expected content size.
+    if (expected_content_size == total_read_bytes_) {

[mmenke, 2017/01/11 20:25:19]

Should we separate out those where expected_content_size is >= kBufferSize?

[maksims (do not use this acc), 2017/01/12 09:03:02]

Do you think I should add another enum entry, which will be something like
"EQ_RESPONSE_BODY_MT_EQ_BUFFER_SIZE"?

[mmenke, 2017/01/12 18:04:10]

I'm fine with that.  I'm not sure how much it gets us, but it can't hurt, and
better to have it and not need it than need it and not have it.

[maksims (do not use this acc), 2017/01/13 12:05:17]

Done.

+      UMA_HISTOGRAM_ENUMERATION(

[mmenke, 2017/01/11 20:25:19]

These UMA_HISTOGRAM_ENUMERATION are deceivingly expensive.  It would reduce
binary size to decide on the correct value, and then only have one call to add
an entry to the Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize
histogram.

[maksims (do not use this acc), 2017/01/13 12:05:18]

I see. Do you think I should use something else?

[mmenke, 2017/01/13 16:01:36]

No, but you should only have one instance of "UMA_HISTOGRAM_ENUMERATION".

+          "Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize",
+          ExpectedContentSize::CORRESPONDS_TO_RESPONSE_BODY,
+          ExpectedContentSize::EXPECTED_CONTENT_SIZE);
+    } else if (expected_content_size < total_read_bytes_) {
+      UMA_HISTOGRAM_ENUMERATION(
+          "Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize",
+          ExpectedContentSize::LT_RESPONSE_BODY,
+          ExpectedContentSize::EXPECTED_CONTENT_SIZE);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION(
+          "Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize",
+          ExpectedContentSize::MT_RESPONSE_BODY,
+          ExpectedContentSize::EXPECTED_CONTENT_SIZE);
+    }
+  } else {
+    UMA_HISTOGRAM_BOOLEAN(
+        "Net.ResourceLoader.ExpectedContentSize.IsKnown",
+        false);

[mmenke, 2017/01/11 20:25:19]

Do we really need two histograms?  Seems like we could just make an
ExpectedContentSize::UNKNOWN and add it to the previous histogram.  This reducse
histogram upload size, and reduces data on the servers (Each recorded datapoint
is a database entry, I believe).  So unless we get something from additional
histograms we can't get from just using a calculator and a smaller number of
histograms, best to use fewer histograms.

[maksims (do not use this acc), 2017/01/12 09:03:03]

sounds reasonable.

[maksims (do not use this acc), 2017/01/13 12:05:17]

Done.

+  }
+
+  // Record how many times total size of read body is less or more/eq than
+  // buffer.
+  UMA_HISTOGRAM_BOOLEAN(
+      "Net.ResourceLoader.TotalBodySize.LT_AsyncHandlerBufferSize",
+      total_read_bytes_ < kBufferSize);

[mmenke, 2017/01/11 20:25:20]

I don't think we need this - can just add up the values of
ExpectedContentSize::CORRESPONDS_TO_RESPONSE_BODY and
ExpectedContentSize::LT_RESPONSE_BODY.

[maksims (do not use this acc), 2017/01/12 09:03:02]

But we'll loose data, which tells us how accurate expected content size is
comparing to total body size, then.

[mmenke, 2017/01/12 18:04:10]

Sorry, I wasn't clear.  I mean keep them as separate buckets in the histogram,
but if we ever want to know what this histogram would look like, we can just add
those two buckets in the
"Net.ResourceLoader.ExpectedContentSize.CorrespondsBodySize" histogram, so we
don't need a separate histogram for them.

+  total_read_bytes_ = 0;
 }
 
 void AsyncResourceHandler::SendUploadProgress(int64_t current_position,
                                               int64_t total_size) {
   ResourceMessageFilter* filter = GetFilter();
   if (!filter)
     return;
   filter->Send(new ResourceMsg_UploadProgress(
       GetRequestID(), current_position, total_size));
 }
 
 }  // namespace content