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 "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/MurmurHash3.h"
+
+
+namespace content {
+
+namespace{
+
+// This set keeps track of a hash of resources
+// that we have seen
+std::set<uint32>* GetSetOfHashes() {
+  static std::set<uint32> seen_resources;

[gavinp, 2012/07/14 19:05:06]

This won't build in clang, due to our static destructor rule.

Do you think any of your hashes will ever get very big? If they will, you should
consider dropping them and stop running when the system comes under memory
pressure.

+  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),
+      start_seed_(0),
+      bytes_read_(0),
+      read_buffer_(new net::IOBuffer(kReadBufSize)),
+      request_(request) {
+}
+
+DuplicateResourceHandler::~DuplicateResourceHandler() {
+}
+
+bool DuplicateResourceHandler::OnWillRead(int request_id, net::IOBuffer** buf, 
+                                          int* buf_size, int min_size) {
+  DCHECK_EQ(-1, min_size);
+
+  *buf = read_buffer_.get(); 
+  *buf_size = kReadBufSize;
+  return next_handler_->OnWillRead(request_id, buf, buf_size, min_size);

[gavinp, 2012/07/14 19:05:06]

This doesn't work; you're setting *buf and *buf_size, but so does the
AsyncResourceHandler, and it's later in the chain than the
DuplicateResourceHandler. So your buffer isn't being used... You're just hashing
initialized memory over and over again.

You shouldn't be allocating your own buffer,  either. You should be just keeping
track of the one you're expecting to see in OnReadCompleted. And you need to
grab it after OnWillRead().

[frankwang, 2012/07/16 01:44:05]

Done.

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

[gavinp, 2012/07/14 19:05:06]

Once you've fixed the above, you'll want to read the buffer here. Your earlier
crashing version was crashing because of course the AsyncResourceHandler tosses
the buffer once it's done. 

[frankwang, 2012/07/16 01:44:05]

Done.

+  if (!next_handler_->OnReadCompleted(request_id, bytes_read, defer))
+    return false;
+
+  // Find hash of buffer, using previous hash as the seed (first seed is 0)

[gavinp, 2012/07/14 19:05:06]

This does not work. MurmurHash3 is not incremental, seed doesn't work like that.
I think you want to use MurmurHash2A, since we have code for it in
third_party/smhasher (though we don't build it, that's easily fixed).

+  MurmurHash3_x86_32(read_buffer_->data(), bytes_read, start_seed_, &start_seed_);
+
+  bytes_read_ += bytes_read;
+  return true;
+}
+
+bool DuplicateResourceHandler::OnResponseCompleted(
+    int request_id,
+    const net::URLRequestStatus& status,
+    const std::string& security_info) {
+  
+  // Hash url into the resource to see whether it is from the same or different origin
+  uint32 hashed_with_url;
+  const char* url = request_->url().spec().c_str();
+  MurmurHash3_x86_32(url, strlen(url), start_seed_, &hashed_with_url);
+
+  // This boolean answers whether we found resource regardless of origin
+  const bool did_we_find_resource = GetSetOfHashes()->find(start_seed_) != GetSetOfHashes()->end();
+  // This boolean checks whether we found a resource from the same origin as one previously seen
+  const bool did_we_find_resource_same_origin = 

[gavinp, 2012/07/14 19:05:06]

Origin isn't the right term to use here, for http://foo.com/bar/quux, the web
origin is ('http', 'foo.com', 80).  Perhaps you want original_url?

[frankwang, 2012/07/16 01:44:05]

Done.

+      GetSetOfHashesWithURL()->find(hashed_with_url) != GetSetOfHashesWithURL()->end();
+  
+  // If we found the resource, classify whether it is from the same origin or different
+  if (did_we_find_resource) {
+    // If it is the same origin, it is a hit on both caches
+    if (did_we_find_resource_same_origin) {
+      UMA_HISTOGRAM_BOOLEAN("Duplicate.ProposedCache.HitRate", true);

[gavinp, 2012/07/14 19:05:06]

Each of these macro invocations has a global variable hidden in it, so it's best
to avoid having too many redundant ones.

Why not avoid the nested ifs, and instead something like:

UMA_HISTOGRAM_BOOLEAN("Duplicate.ProposedCache.Hits", did_we_find_resource);

I'm not a fan of "ProposedCache", how about "HashCache" ? Or are we even at the
level of a cache here? Maybe "Duplicate.Hash.Hits" ?

[frankwang, 2012/07/16 01:44:05]

Done.

+      UMA_HISTOGRAM_BOOLEAN("Duplicate.CurrentCache.HitRate", true);

[gavinp, 2012/07/14 19:05:06]

You're not really measuring the current cache here. You're measuring an
hypothetical URL based cache that always works when the resources are identical.

"Duplicate.HashWithUrl.Hit" ?

[frankwang, 2012/07/16 01:44:05]

Done.

+    } else {
+      // If it is a different origin (interesting case), it hits on the
+      // proposed cache not the current cache
+      UMA_HISTOGRAM_BOOLEAN("Duplicate.ProposedCache.HitRate", true);
+      UMA_HISTOGRAM_BOOLEAN("Duplicate.CurrentCache.HitRate", false);
+      // Record kilobytes (log base 10) missed because we are caching based on origin instead of resource
+      UMA_HISTOGRAM_COUNTS("Duplicate.SizeKB.Miss.CurrentCache", log10(bytes_read_));

[frankwang, 2012/07/14 02:13:59]

I changed this locally to "Duplicate.Size.Miss.CurrentCache" since it is
actually bytes base 10 because it was wrong and misleading. 

[gavinp, 2012/07/14 19:05:06]

UMA_HISTOGRAM_COUNTS is already exponentially binned, so might as well:

UMA_HISTOGRAM_CUSTOM_COUNTS("Duplicate.Hash.Size", bytes_read_, 1, 0x7FFFFFFF,
50);

[frankwang, 2012/07/16 01:44:05]

Done.

+      // Record resource type for missed resource
+      UMA_HISTOGRAM_ENUMERATION("Duplicate.ResourceType", resource_type_, ResourceType::LAST_TYPE);
+      GetSetOfHashesWithURL()->insert(hashed_with_url);
+    }
+  } else {
+    // We did not see the resource so it is a miss on both caches
+    UMA_HISTOGRAM_BOOLEAN("Duplicate.ProposedCache.HitRate", false);
+    UMA_HISTOGRAM_BOOLEAN("Duplicate.CurrentCache.HitRate", false);
+    GetSetOfHashes()->insert(start_seed_);
+    GetSetOfHashesWithURL()->insert(hashed_with_url);
+  }
+  
+  bytes_read_ = 0;
+  read_buffer_ = NULL;
+  return next_handler_->OnResponseCompleted(request_id, status, security_info);
+}
+
+}  //namespace content