X-Chrome-Variations logic refactoring

chrome/browser/chrome_metrics_helper.h

+// 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.
+
+#ifndef CHROME_BROWSER_CHROME_METRICS_HELPER_H_
+#define CHROME_BROWSER_CHROME_METRICS_HELPER_H_
+
+#include <set>
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/metrics/field_trial.h"
+#include "base/synchronization/lock.h"
+#include "chrome/common/metrics/variations/variation_ids.h"
+
+namespace content {
+class ResourceContext;
+}
+
+namespace net {
+class HttpRequestHeaders;
+}
+
+class GURL;
+class Profile;
+class ProfileIOData;
+
+template <typename T> struct DefaultSingletonTraits;

[Peter Kasting, 2012/12/17 19:51:57]

Tiny nit: I have no idea if we document this somewhere, but it seems like we
usually put non-namespaced class declarations above the namespaced ones.  And
I'd probably eliminate the newlines between them.

+
+// A helper class for maintaining Chrome experiments and metrics state
+// transmitted in custom HTTP request headers.
+// This class is a thread-safe singleton.

[Peter Kasting, 2012/12/17 19:51:57]

So, this critical question never got answered by Steve: does this class really
need to be a threadsafe singleton?  Or, to put it differently: is the cost of
calling GetActiveFieldTrialGroups() so large that we need all the caching
behavior this class implements?  Because if we eliminate that caching, then the
only issue is whether that function can be called on multiple threads.  The
answer to that is unclear to me; the implementation uses a lock, which suggests
it's thread safe, but the header comments are unclear.

[SteveT, 2012/12/17 20:30:46]

So it's not just the call to GetActiveFieldTrialGroups that is expensive (that
method just enumerates 20-30 field trials and returns their names in a vector),
but also the rest of the work to create the cache (serializing it in a
protobuf-string format, and then base64-hashing it). Calling this once isn't
bad, but we figured calling this _every_ time a request is sent to google.com
might be a bit much.

Now, we have not measured the actual cost of this vs the cost of the locking, so
it's hard to say for sure if it's *necessary*, but I suspect it's still nice to
have this optimization. Do you think we're blowing this out of proportion here?

Bart - As for the comment, were you referring to this class being thread safe
since you've implemented proper locking in it, or were you referring to it
getting some thread-safe properties out of being used with
DefaultSingletonTraits and Singleton<...> (not familiar with whether or not that
is something you get for free from this Singleton configuration).

[Bart N., 2012/12/17 20:43:36]

No, I was referring to the fact that all public methods of this class can be
called from multiple threads, specifically AppendHeaders(...). We have to make
sure this holds, because AppendHeaders is called from IO and UI threads and we
want to avoid a race condition where headers are being updated while a call to
AppendHeaders is in progress.

If I understand Peter's comment correctly, he objects the singleton part which
is necessary because of caching. Thread-safety constraint will have to remain
regardless if we use singleton or not, because we call AppendHeaders from two
different threads. The only question remains is whether we need a lock or not.
Certainly, if the underlying GetFieldTrialList method is thread-safe, we won't
need the lock here (given that we no longer need singleton).

Lastly, regarding lock cost -- I'm almost certain the lock cost is lower than
iterating over all field trial lists and computing Base64, but both methods can
actually be cheap in absolute terms.

One more thing to keep in mind that when this change lands, we will be calling
it pretty much on each keystroke.

[SteveT, 2012/12/17 20:49:56]

GetActiveFieldTrialGroups (you were referring to this, right?) is itself guarded
by a lock. But of course that doesn't protect the X-Chrome-Variations header
string which is written/read potentially from different threads. So it sounds to
me like we still need to lock that as a separate resource. Let me know if I'm
missing something here..

[Bart N., 2012/12/17 21:09:14]

Yes, GetActiveFieldTrialGroups. So my understanding is as follows: if
GetActiveFieldTrialGroups+UpdateVariationIDsHeaderValue is cheap (and we can
decide what cheap means for us, e.g. <= 1us), we don't need the whole singleton
logic. We can basically have it stateless (e.g. let
UpdateVariationIDsHeaderValue return the header value rather than update class
state). I think this is what Peter asked for.

[SteveT, 2012/12/17 21:17:59]

Yes, that seems fair. Is it OK for us to keep the code as-is for now, and
measure/act on this after M25? Perhaps merge in something if it turns out to be
performance-critical?

+class ChromeMetricsHelper : base::FieldTrialList::Observer {
+ public:
+  static ChromeMetricsHelper* GetInstance();
+
+  // Adds Chrome experiment and metrics state as custom headers to |headers|.
+  // Some headers may not be set given the |incognito| mode or whether
+  // the user has |uma_enabled|.  Also, we never transmit headers to non-Google
+  // sites, which is checked based on the destination |url|.
+  void AppendHeaders(const GURL& url,
+                     bool incognito,
+                     bool uma_enabled,
+                     net::HttpRequestHeaders* headers);
+
+ private:
+  friend struct DefaultSingletonTraits<ChromeMetricsHelper>;
+
+  ChromeMetricsHelper();
+  virtual ~ChromeMetricsHelper();
+
+  // base::FieldTrialList::Observer implementation.
+  // This will add the variation ID associated with |trial_name| and
+  // |group_name| to the variation ID cache.
+  virtual void OnFieldTrialGroupFinalized(
+      const std::string& trial_name,
+      const std::string& group_name) OVERRIDE;
+
+  // Prepares the variation IDs cache with initial values if not already done.
+  // This method also registers the caller with the FieldTrialList to receive
+  // new variation IDs.
+  void InitVariationIDsCacheIfNeeded();
+
+  // Takes whatever is currently in |variation_ids_set_| and recreates
+  // |variation_ids_header_| with it.
+  void UpdateVariationIDsHeaderValue();
+
+  // Guards |variation_ids_cache_initialized_|, |variation_ids_set_| and
+  // |variation_ids_header_|.
+  base::Lock lock_;
+
+  // Whether or not we've initialized the cache.
+  bool variation_ids_cache_initialized_;
+
+  // Keep a cache of variation IDs that are transmitted in headers to Google.
+  // This consists of a list of valid IDs, and the actual transmitted header.
+  std::set<chrome_variations::VariationID> variation_ids_set_;

[Peter Kasting, 2012/12/17 19:51:57]

Nit: Consider a typedef (e.g. "Variations") for this type.

+  std::string variation_ids_header_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeMetricsHelper);
+};
+
+#endif  // CHROME_BROWSER_CHROME_METRICS_HELPER_H_