DuplicateContentResourceHandler to monitor resources and track how many times th…

content/browser/renderer_host/duplicate_resource_handler.cc

Index: content/browser/renderer_host/duplicate_resource_handler.cc
===================================================================
--- content/browser/renderer_host/duplicate_resource_handler.cc	(revision 0)
+++ content/browser/renderer_host/duplicate_resource_handler.cc	(revision 0)
@@ -0,0 +1,141 @@
+// 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/renderer_host/duplicate_resource_handler.h"
+
+#include <cmath>
+#include <cstring>
+#include <set>
+
+#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 "net/url_request/url_request.h"
+#include "third_party/smhasher/src/PMurHash.h"
+
+
+namespace content {
+
+namespace{
+
+// This set keeps track of a hash of resources
+// that we have seen
+std::set<uint32>* GetSetOfHashes() {

[gavinp, 2012/07/18 12:13:32]

Naming by data type isn't ideal. Better by use: ContentHashes maybe? And
ContentWithUrlHashes below?

[frankwang, 2012/07/19 16:10:26]

Done.

+  static std::set<uint32> seen_resources;

[gavinp, 2012/07/18 12:13:32]

Probably we should bite the bullet, and use base/memory/singleton.h for this.

[frankwang, 2012/07/19 16:10:26]

Done.

+  return &seen_resources;
+}
+
+// This set keeps track of hash of resources based on origin
+// that we have seen previously
+std::set<uint32>* GetSetOfHashesWithURL(){
+  static std::set<uint32> seen_resources_with_url;
+  return &seen_resources_with_url;
+}
+
+}  // namespace
+
+DuplicateResourceHandler::DuplicateResourceHandler(
+    scoped_ptr<ResourceHandler> next_handler, 
+    ResourceType::Type resource_type,
+    net::URLRequest* request)
+    : LayeredResourceHandler(next_handler.Pass()),
+      resource_type_(resource_type),
+      ph1_(0),
+      pcarry_(0),
+      buffer_size_(0),
+      bytes_read_(0),
+      request_(request) {
+}
+
+DuplicateResourceHandler::~DuplicateResourceHandler() {
+}
+
+bool DuplicateResourceHandler::OnWillRead(int request_id, net::IOBuffer** buf, 
+                                          int* buf_size, int min_size) {
+  DCHECK_EQ(-1, min_size);
+  
+  if (!next_handler_->OnWillRead(request_id, buf, buf_size, min_size))
+    return false;
+

[gavinp, 2012/07/18 12:13:32]

Lose this blank line.

[frankwang, 2012/07/19 16:10:26]

Done.

+  read_buffer_ = *buf; 
+  buffer_size_ = *buf_size;
+  return true;
+}
+
+bool DuplicateResourceHandler::OnReadCompleted(int request_id, int bytes_read,
+                                              bool* defer) {
+
+  PMurHash32_Process(&ph1_,&pcarry_,read_buffer_->data(), bytes_read);

[gavinp, 2012/07/18 12:13:32]

spaces after commas.

[frankwang, 2012/07/19 16:10:26]

Done.

+  bytes_read_ += bytes_read;
+
+  return next_handler_->OnReadCompleted(request_id, bytes_read, defer);
+}
+
+bool DuplicateResourceHandler::OnResponseCompleted(
+    int request_id,
+    const net::URLRequestStatus& status,
+    const std::string& security_info) {

[gavinp, 2012/07/18 12:13:32]

What should you do for status != net::URLRequestStatus::SUCCESS ?

[frankwang, 2012/07/19 16:10:26]

I put in a check so that I pass through when it is not SUCCESS so the next
handler can deal with it.

+  
+  uint32 resource_hash = PMurHash32_Result(ph1_, pcarry_, bytes_read_);
+
+  // Hash url into the resource to see whether it is 
+  // from the same or different origin

[gavinp, 2012/07/18 12:13:32]

Comments should be sentences (have a period) and wrap at 80 columns. Origin
isn't the right name for what you're talking about, the origin of a resource is
a triple (scheme, hostname, portnumber) usually, as in ('http', 'foo.com', 80)
is the origin of http://foo.com/bar/quux?blatto#thisisafragment. You want to
compare the url. Of note also is that the urls you get will never have
fragments.

[frankwang, 2012/07/19 16:10:26]

Done.

+  uint32 hashed_with_url;
+  const char* url = request_->url().spec().c_str();
+  int url_length = strlen(url);

[gavinp, 2012/07/18 12:13:32]

This scares me a bit. Is it safe to trust c_str() after destruction of what may
have been a temporary std::string? And why put the null on the string anyway,
isn't std::string::data() good enough? I also don't like the O(n) strlen() when
we had a C++ string which stored explicit length earlier. Lastly, you include
<cstring>, but don't call std::strlen, and instead rely on one of your includes
having included string.h.

How about:

const std::string url_spec = request_->url().spec();

then use url_spec.data() and url_spec.size() in the call to PMurHash32_Process?

[frankwang, 2012/07/19 16:10:26]

Done.

+  PMurHash32_Process(&ph1_, &pcarry_, url, url_length);
+  hashed_with_url = PMurHash32_Result(ph1_, pcarry_, url_length + bytes_read_);
+  
+  DVLOG(4) << "url: " << url;
+  DVLOG(4) << "resource hash: " << resource_hash;
+  DVLOG(4) << "hash with url: " << hashed_with_url;
+
+  // This boolean answers whether we found resource 
+  // based just on hash
+  const bool did_we_find_resource = 

[gavinp, 2012/07/18 12:13:32]

did_match_contents_ maybe?

[frankwang, 2012/07/19 16:10:26]

Done.

+      GetSetOfHashes()->find(resource_hash) != 

[gavinp, 2012/07/18 12:13:32]

Use count(resource_hash) instead. You don't need to compare to end(), and it
makes it more clear your testing for existence. That should make the comment
above moot, too. Same just below.

Also, might as well grab a ptr to this set rather than use this method, for
easier reading:

std::set<uint32>* content_hashes = GetSetOfHashes();
...

+      GetSetOfHashes()->end();
+
+  // This boolean checks whether we found a resource from the original url 
+  // as one previously seen
+  const bool did_we_find_resource_original_url = 

[gavinp, 2012/07/18 12:13:32]

I've thought about it more, and now I don't like this name, it sounds like you
are checking if you found an URL match, when really you're checking URL and
Data. did_match_contents_and_url_ ?

[frankwang, 2012/07/19 16:10:26]

Done.

+      GetSetOfHashesWithURL()->find(hashed_with_url) != 
+      GetSetOfHashesWithURL()->end();
+  
+  // If we found the resource, classify whether it is 
+  // from the same url or different

[gavinp, 2012/07/18 12:13:32]

Best practice is to have a single instance of each histogram. It saves memory,
and I think makes more readable code. Consider:

UMA_HISTOGRAM_BOOLEAN("Duplicate.Hits", did_match_contents);
UMA_HISTOGRAM_BOOLEAN("Duplicate.HitsSameUrl", did_match_contents &&
did_match_contents_and_url);
if (did_match_contents && !did_match_contents_and_url) {
  UMA_HISTOGRAM_CUSTOM_COUNTS("Duplicate.Size.HashHitUrlMiss", ...)
  ....

[frankwang, 2012/07/19 16:10:26]

Done.

+  if (did_we_find_resource) {
+    // If it is from the original url, it will hit on both caches
+    if (did_we_find_resource_original_url) {
+      UMA_HISTOGRAM_BOOLEAN("Duplicate.Hash.Hits", true);

[gavinp, 2012/07/18 12:13:32]

I've thought about this a bit more: I think you just want Duplicate.Hits and
Duplicate.HitsSameUrl . Sorts better (the two varieties will sort right next to
each other), and shorter.

[frankwang, 2012/07/19 16:10:26]

Done.

+      UMA_HISTOGRAM_BOOLEAN("Duplicate.HashSameUrl.Hits", true);
+    } else {
+      // If it is a different url (interesting case), it hits on the
+      // proposed cache not the current cache
+      UMA_HISTOGRAM_BOOLEAN("Duplicate.Hash.Hits", true);
+      UMA_HISTOGRAM_BOOLEAN("Duplicate.HashSameUrl.Hits", false);
+      // Record bytes missed because we are caching 
+      // based on origin instead of resource
+      UMA_HISTOGRAM_CUSTOM_COUNTS("Duplicate.HashMiss.Size", bytes_read_, 

[gavinp, 2012/07/18 12:13:32]

I think you want: "Duplicate.Size.HashHitUrlMiss" this will sort better in the
histogram tool (put all varieties of sizes near each other), and better
describes what we have. Do you also want .HashHit and .HashMiss ?
.HashMissUrlHit ?

[frankwang, 2012/07/19 16:10:26]

Done.

+                                   1, 0x7FFFFFFF, 50);
+      // Record resource type for missed resource
+      UMA_HISTOGRAM_ENUMERATION("Duplicate.ResourceType", resource_type_, 

[gavinp, 2012/07/18 12:13:32]

The name should reflect that it's for missed resources.
"Duplicate.ResourceType.HashMiss".  Do you also want to talk about the MIME
type?

[frankwang, 2012/07/19 16:10:26]

Done.

+                                 ResourceType::LAST_TYPE);
+      GetSetOfHashesWithURL()->insert(hashed_with_url);
+    }
+  } else {
+    // We did not see the resource so it is a miss on both caches

[gavinp, 2012/07/18 12:13:32]

I don't like comments like this. The logic should be concise enough to make this
clear.

[frankwang, 2012/07/19 16:10:26]

Done.

+    UMA_HISTOGRAM_BOOLEAN("Duplicate.Hash.Hits", false);
+    UMA_HISTOGRAM_BOOLEAN("Duplicate.HashSameUrl.Hits", false);
+    GetSetOfHashes()->insert(resource_hash);
+    GetSetOfHashesWithURL()->insert(hashed_with_url);
+  }
+  
+  bytes_read_ = 0;
+  read_buffer_ = NULL;
+  return next_handler_->OnResponseCompleted(request_id, status, security_info);
+}
+
+}  //namespace content