precache: Allow experiment bitsets of any size.

components/precache/core/precache_fetcher.cc

Index: components/precache/core/precache_fetcher.cc
diff --git a/components/precache/core/precache_fetcher.cc b/components/precache/core/precache_fetcher.cc
index 0bc2e1b2b40cb1e2c17a5bfdf84000c00aec1a08..f48f1915dcaa0843ddb4165e028087b74d778cbc 100644
--- a/components/precache/core/precache_fetcher.cc
+++ b/components/precache/core/precache_fetcher.cc
@@ -22,6 +22,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/optional.h"
 #include "base/sha1.h"
 #include "base/strings/string_piece.h"
 #include "base/task_runner_util.h"
@@ -125,18 +126,38 @@ bool ParseProtoFromFetchResponse(const net::URLFetcher& source,
 }
 
 // Returns the resource selection bitset from the |manifest| for the given
-// |experiment_id|. By default all resource will be selected if the experiment
-// group is not found.
-uint64_t GetResourceBitset(const PrecacheManifest& manifest,
-                           uint32_t experiment_id) {
+// |experiment_id|. If the experiment group is not found, then this returns
+// nullopt, in which case all resources should be selected.
+base::Optional<std::vector<bool>> GetResourceBitset(
+    const PrecacheManifest& manifest,
+    uint32_t experiment_id) {

[pasko, 2017/02/24 13:42:33]

Do I understand correctly that each server manifest maps a finch group to a
bitset, and then you also experiment with different bitsets in the manifest
generation pipeline?

Wow. Advanced.

Did you find 64 to be limiting in some specific cases or you just want to
experiment with larger bitsets and see what happens?

[twifkak, 2017/02/24 21:20:12]

Yup.
Thanks. Or sorry. :P
Don't know for certain, but we suspect it's an issue. 64 is the number of
_unfiltered_ resources -- the more filters we experiment with, the fewer
_filtered_ resources per experiment we get. So, e.g. if we experiment with
filtering out short-lived or uncached resources, and 90% of resources are
filtered out, then we only get 6.4 resources per manifest.

In addition, with global ranking, it may be useful for some sites to have more
resources while others have less.

[pasko, 2017/02/27 18:12:16]

Ah, thank you for background. Naive question: why not applying bitsets after
filtering? Is it something historical?

[twifkak, 2017/02/27 21:46:28]

Perhaps I misunderstand your question. The bitsets are the mechanism for
client-side filtering. The idea is we want to play with different manifest
variants; it would be possible to generate and host multiple copies of the
manifests, but this is much cheaper. So let's say we want to experiment with two
versions of the manifest:

Variant 1: [a.js, b.js]
Variant 2: [a.js, c.js]

Then we'd encode that as:

{resources: [a.js, b.js, c.js],
 variants: {1: 0x3, 2: 0x5}}

Because the bitset was a fixed64, that put a hard limit on the number of
elements in 'resources' at 64, regardless of how many bits are set.

PS--The history of this feature is we added it to test the question "Should we
precache images?" We've more recently used it to test several other what-ifs,
though.

[pasko, 2017/02/28 12:21:31]

Oh, ah. Having bitsets _for_ filtering makes a lot of sense to me. Thank you.

+  base::Optional<std::vector<bool>> ret;
   if (manifest.has_experiments()) {
     const auto& resource_bitset_map =
         manifest.experiments().resources_by_experiment_group();
-    const auto& resource_bitset_it = resource_bitset_map.find(experiment_id);
-    if (resource_bitset_it != resource_bitset_map.end())
-      return resource_bitset_it->second.bitset();
+    const auto& it = resource_bitset_map.find(experiment_id);
+    if (it != resource_bitset_map.end()) {
+      if (it->second.has_bitset()) {
+        const std::string& bitset = it->second.bitset();
+        ret.emplace(bitset.size() * 8);
+        for (size_t i = 0; i < bitset.size(); ++i) {
+          for (size_t j = 0; j < 8; ++j) {
+            if ((1 << j) & bitset[i])
+              ret.value()[i * 8 + j] = true;
+          }
+        }
+      } else if (it->second.has_deprecated_bitset()) {
+        uint64_t bitset = it->second.deprecated_bitset();
+        ret.emplace(64);
+        for (int i = 0; i < 64; ++i) {
+          if ((0x1ULL << i) & bitset)
+            ret.value()[i] = true;
+        }
+      }
+    }
   }
-  return ~0ULL;
+  // Only return one variable to ensure RVO triggers.

[pasko, 2017/02/24 13:42:33]

what is RVO? Render View Observer?

[twifkak, 2017/02/24 21:20:12]

Return Value Optimization. Constructs the object on the stack frame of the
calling function, rather than constructing it here and then copying or moving it
back.

[pasko, 2017/02/27 18:12:16]

Acknowledged.

+  return ret;
 }
 
 // URLFetcherResponseWriter that ignores the response body, in order to avoid
@@ -746,10 +767,10 @@ void PrecacheFetcher::OnManifestFetchComplete(int64_t host_visits,
       const int32_t len =
           std::min(manifest.resource_size(),
                    unfinished_work_->config_settings().top_resources_count());
-      const uint64_t resource_bitset =
+      const base::Optional<std::vector<bool>> resource_bitset =
           GetResourceBitset(manifest, experiment_id_);
       for (int i = 0; i < len; ++i) {
-        if (((0x1ULL << i) & resource_bitset) &&
+        if ((!resource_bitset.has_value() || resource_bitset.value()[i]) &&
             manifest.resource(i).has_url()) {
           GURL url(manifest.resource(i).url());
           if (url.is_valid()) {