DuplicateContentResourceHandler to monitor resources and track how many times th…

content/browser/renderer_host/duplicate_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 <string>
+
+#include "content/browser/renderer_host/duplicate_resource_handler.h"

[gavinp, 2012/07/11 23:39:46]

Move this above #include <string>

[frankwang, 2012/07/12 01:50:36]

Done.

+
+#include "base/logging.h"
+#include "base/metrics/histogram.h"
+#include "content/browser/renderer_host/resource_request_info_impl.h"
+#include "net/base/io_buffer.h"
+#include "third_party/smhasher/src/MurmurHash3.h"
+
+
+namespace content{
+
+namespace{
+ 
+ std::set<uint32>* GetSetOfHashes(){

[gavinp, 2012/07/11 23:39:46]

Nit: don't indent this.

[frankwang, 2012/07/12 01:50:36]

Done.

+   static std::set<uint32> seen_resources;
+   return &seen_resources;
+ }
+
+} // namespace
+
+DuplicateResourceHandler::DuplicateResourceHandler(
+    scoped_ptr<ResourceHandler> next_handler,
+    net::URLRequest* request)
+    : LayeredResourceHandler(next_handler.Pass()),
+      bytes_hit_(0),
+      bytes_miss_(0),
+      read_buffer_size_(0), // keep track of bytes in read buffer
+      request_(request) {
+}
+
+DuplicateResourceHandler::~DuplicateResourceHandler(){
+}
+
+bool DuplicateResourceHandler::OnWillRead(int request_id, net::IOBuffer** buf, 
+                                        int* buf_size, int min_size){
+  DCHECK_EQ(-1, min_size);
+  
+  read_buffer_ = *buf;
+  read_buffer_size_ = *buf_size; 

[gavinp, 2012/07/11 23:39:46]

This is wrong. The buffer is provided by your downstream resource handler.

if (next_handler_->OnWillRead(...)) {
  read_buffer_ = *buf;  // Is it OK to keep this reference?
  read_buffer_size_ = *buf_size;
  return true;
}
return false;

[frankwang, 2012/07/12 01:50:36]

Done.

+  
+  return true;
+}
+
+bool DuplicateResourceHandler::OnReadCompleted(int request_id, int bytes_read,
+                                              bool* defer) {
+

[gavinp, 2012/07/11 23:39:46]

if (!next_handler_->OnReadCompleted(...))
  return false;

[frankwang, 2012/07/12 01:50:36]

Done.

+  // find hash of resource
+  uint32 buf_hash;
+  MurmurHash3_x86_32(read_buffer_, bytes_read, 0x0, &buf_hash);

[gavinp, 2012/07/11 23:39:46]

This is conceptually broken. The reads come to you in whatever quantity we got
from the TCP stack, possibly joined by the BufferedResourceHandler. I think you
need to take hashes of aligned 4k chunks of the file, concatenate those hashes,
and use the hash of that string of hashes to identify each file.

[gavinp, 2012/07/11 23:39:46]

Also:

MurmurHash3_x86_32(read_buffer_->data(), bytes_read, 0x0, &buf_hash);

[frankwang, 2012/07/12 01:50:36]

Done.

+
+  static base::Histogram* num_hits(NULL);

[gavinp, 2012/07/11 23:39:46]

This isn't a common pattern for creating histograms. How about:

const bool did_we_find_it = ...;

UMA_HISTOGRAM_BOOLEAN("Duplicate.HitRate", did_we_find_it);
if (did_we_find_it)
  UMA_HISTOGRAM_CUSTOM_ENUMERATION("Duplicate.HitSize", total_size_, frobnitz);
else
  UMA_HISTOGRAM_CUSTOM_ENUMERATION("Duplicate.MissSize", total_size_, frobnitz);

[frankwang, 2012/07/12 01:50:36]

I changed it just for one histogram. I am still discussing with Elie regarding
what histograms we need exactly since we are collecting data anyway.

+  if (!num_hits)
+    num_hits = base::BooleanHistogram::FactoryGet(
+      "hash.hit", base::Histogram::kUmaTargetedHistogramFlag);
+  
+  // if element is in hash
+  if (GetSetOfHashes()->find(buf_hash) != GetSetOfHashes()->end()){
+    bytes_hit_ += bytes_read;
+    num_hits->AddBoolean(true); 
+
+  } else{
+    bytes_miss_ += bytes_read;
+    num_hits->AddBoolean(false);
+    GetSetOfHashes()->insert(buf_hash);
+  }
+

[gavinp, 2012/07/11 23:41:31]

I'm not sure if it's kosher to keep read_buffer_ around the way you do (I think
it's fine, but we might want to make that an IOBuffer*?). But if you are keeping
a scoped_ptr, you should:

read_buffer_.reset();

So that you don't keep it alive any longer.

[frankwang, 2012/07/12 01:50:36]

Yes, I called release() on it because it was a scoped_refptr. I am not sure if
what I am keeping is right either. 

+  read_buffer_size_ = 0;
+
+  return true;
+}
+
+} //namespace content