Remove the hash fields from FieldTrials.

chrome/common/metrics/experiments_helper.cc

Index: chrome/common/metrics/experiments_helper.cc
diff --git a/chrome/common/metrics/experiments_helper.cc b/chrome/common/metrics/experiments_helper.cc
index 11c7c1389fca180304be1513a6498446e06d441e..ab4b06c4986021f39d84cb386fcd06794ce544e5 100644
--- a/chrome/common/metrics/experiments_helper.cc
+++ b/chrome/common/metrics/experiments_helper.cc
@@ -7,26 +7,15 @@
 #include <vector>
 
 #include "base/memory/singleton.h"
+#include "base/sha1.h"
 #include "base/string16.h"
 #include "base/stringprintf.h"
+#include "base/sys_byteorder.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/common/child_process_logging.h"
 
 namespace {
 
-// We need to pass a Compare class to the std::map template since NameGroupId
-// is a user-defined type.
-struct NameGroupIdCompare {
-  bool operator() (const base::FieldTrial::NameGroupId& lhs,
-                   const base::FieldTrial::NameGroupId& rhs) const {
-    // The group and name fields are just SHA-1 Hashes, so we just need to treat
-    // them as IDs and do a less-than comparison. We test group first, since
-    // name is more likely to collide.
-    return lhs.group == rhs.group ? lhs.name < rhs.name :
-        lhs.group < rhs.group;
-  }
-};
-
 // The internal singleton accessor for the map, used to keep it thread-safe.
 class GroupMapAccessor {
  public:
@@ -38,7 +27,7 @@ class GroupMapAccessor {
   GroupMapAccessor() {}
   ~GroupMapAccessor() {}
 
-  void AssociateID(const base::FieldTrial::NameGroupId& group_identifier,
+  void AssociateID(const experiments_helper::NameGroupId& group_identifier,
                    experiments_helper::GoogleExperimentID id) {
     base::AutoLock scoped_lock(lock_);
     DCHECK(group_to_id_map_.find(group_identifier) == group_to_id_map_.end()) <<
@@ -47,7 +36,7 @@ class GroupMapAccessor {
   }
 
   experiments_helper::GoogleExperimentID GetID(
-      const base::FieldTrial::NameGroupId& group_identifier) {
+      const experiments_helper::NameGroupId& group_identifier) {
     base::AutoLock scoped_lock(lock_);
     GroupToIDMap::const_iterator it = group_to_id_map_.find(group_identifier);
     if (it == group_to_id_map_.end())
@@ -56,33 +45,83 @@ class GroupMapAccessor {
   }
 
  private:
-  typedef std::map<base::FieldTrial::NameGroupId,
-      experiments_helper::GoogleExperimentID, NameGroupIdCompare> GroupToIDMap;
+  typedef std::map<experiments_helper::NameGroupId,
+      experiments_helper::GoogleExperimentID,
+      experiments_helper::NameGroupIdCompare> GroupToIDMap;
 
   base::Lock lock_;
   GroupToIDMap group_to_id_map_;
 };
 
+// Creates unique identifier for the trial by hashing a name string, whether
+// it's for the field trial or the group name.
+uint32 HashName(const std::string& name) {
+  // SHA-1 is designed to produce a uniformly random spread in its output space,
+  // even for nearly-identical inputs.
+  unsigned char sha1_hash[base::kSHA1Length];
+  base::SHA1HashBytes(reinterpret_cast<const unsigned char*>(name.c_str()),
+                      name.size(),
+                      sha1_hash);
+
+  COMPILE_ASSERT(sizeof(uint32) < sizeof(sha1_hash), need_more_data);
+  uint32 bits;
+  memcpy(&bits, sha1_hash, sizeof(bits));
+
+  return base::ByteSwapToLE32(bits);
+}
+
+experiments_helper::NameGroupId MakeNameGroupId(
+    const std::string& trial_name,
+    const std::string& group_name) {

[jar (doing other things), 2012/04/26 22:24:22]

nit: This would probably be cleaner taking a:
const& SelectGroup

Usually, it is nicest to keep such pairs of items (name/group) in an object as
long as possible.  I think your original code had it more along those lines.

[SteveT, 2012/04/26 23:42:47]

I'm going to have to push back on this one... This is mostly meant to facilitate
the conversion in AssociateGoogleExperimentID and GetGoogleExperimentID below.
The third place that uses this is GetFieldTrialNameGroupIdsForSelectedGroups
below. Now, I know that you're suggesting this in terms of function signature
cleanliness, but it causes some headaches below.

GetFieldTrialNameGroupIdsForSelectedGroups' impl would be slightly cleaner, but
in Associate/GetGoogleExperimentID, we'd have to create a SelectedGroup both
times and string copy trial_name and group_name into the SelectedGroup. Note
that if we change Associate/Get to also take a SelectedGroup, we'd have to have
every caller of these APIs copy strings into a SelectedGroup.

Let me know if you have any objections to my reasoning for skipping this nit.

[jar (doing other things), 2012/04/27 19:05:44]

Is your concern focused on a performance impact?  Isn't that function rarely
called?  If we were concerned about peroformance, wouldn't we cache the values
(and invalidate when we had any change, which is super rare), and (almost) never
use these calls?




On 2012/04/26 23:42:47, SteveT wrote:

[SteveT, 2012/04/27 19:15:13]

I'm currently more concerned about code cleanliness in
Assosicate/GetGoogleExperimentID rather than about performance*.

Having to create a new SelectedGroup in those two methods is a  more (and
duplicated) code than keeping the arguments of MakeNameGroupId as-is). If you
feel strongly against this, I can go ahead and make your change.

* Re: Performance: You're right that AssociateGoogleExperimentID is a
seldom-called API. It gets called once per group per FieldTrial. However,
GetGoogleExperimentID might be invoked a lot more. We'd eventually like to fetch
the ID for each registered FieldTrial to send to the server per request to
Google, so the API may be called a lot. However, we'll mitigate that by caching,
like you suggested.

[jar (doing other things), 2012/04/27 19:31:55]

The win is not that big either way (currently)... and it sounds like you're more
psyched as is....

so...

LGTM.

Thanks for responding!

On 2012/04/27 19:15:13, SteveT wrote:

+  experiments_helper::NameGroupId id;
+  id.name = HashName(trial_name);
+  id.group = HashName(group_name);
+  return id;
+}
+
+// Populates |name_group_ids| based on |selected_groups|.
+void GetFieldTrialNameGroupIdsForSelectedGroups(
+    const base::FieldTrial::SelectedGroups& selected_groups,
+    std::vector<experiments_helper::NameGroupId>* name_group_ids) {
+  DCHECK(name_group_ids->empty());
+  for (base::FieldTrial::SelectedGroups::const_iterator it =
+       selected_groups.begin(); it != selected_groups.end(); ++it) {
+    name_group_ids->push_back(MakeNameGroupId(it->trial, it->group));
+  }
+}
+
 }  // namespace
 
 namespace experiments_helper {
 
 const GoogleExperimentID kEmptyGoogleExperimentID = 0;
 
-void AssociateGoogleExperimentID(
-    const base::FieldTrial::NameGroupId& group_identifier,
-    GoogleExperimentID id) {
-  GroupMapAccessor::GetInstance()->AssociateID(group_identifier, id);
+void GetFieldTrialNameGroupIds(std::vector<NameGroupId>* name_group_ids) {
+  DCHECK(name_group_ids->empty());
+  // A note on thread safety: Since GetFieldTrialSelectedGroups is thread
+  // safe, and we operate on a separate list of that data, this function is
+  // technically thread safe as well, with respect to the FieldTriaList data.
+  base::FieldTrial::SelectedGroups selected_groups;
+  base::FieldTrialList::GetFieldTrialSelectedGroups(&selected_groups);
+  GetFieldTrialNameGroupIdsForSelectedGroups(selected_groups, name_group_ids);
+}
+
+void AssociateGoogleExperimentID(const std::string& trial_name,
+                                 const std::string& group_name,
+                                 GoogleExperimentID id) {
+  GroupMapAccessor::GetInstance()->AssociateID(
+      MakeNameGroupId(trial_name, group_name), id);
 }
 
-GoogleExperimentID GetGoogleExperimentID(
-    const base::FieldTrial::NameGroupId& group_identifier) {
-  return GroupMapAccessor::GetInstance()->GetID(group_identifier);
+GoogleExperimentID GetGoogleExperimentID(const std::string& trial_name,
+                                         const std::string& group_name) {
+  return GroupMapAccessor::GetInstance()->GetID(
+      MakeNameGroupId(trial_name, group_name));
 }
 
 void SetChildProcessLoggingExperimentList() {
-  std::vector<base::FieldTrial::NameGroupId> name_group_ids;
-  base::FieldTrialList::GetFieldTrialNameGroupIds(&name_group_ids);
+  std::vector<NameGroupId> name_group_ids;
+  GetFieldTrialNameGroupIds(&name_group_ids);
   std::vector<string16> experiment_strings(name_group_ids.size());
   for (size_t i = 0; i < name_group_ids.size(); ++i) {
     // Wish there was a StringPrintf for string16... :-(
@@ -93,3 +132,19 @@ void SetChildProcessLoggingExperimentList() {
 }
 
 }  // namespace experiments_helper
+
+// Functions below are exposed for testing explicitly behind this namespace.
+// They simply wrap existing functions in this file.
+namespace testing {
+
+void TestGetFieldTrialNameGroupIdsForSelectedGroups(
+    const base::FieldTrial::SelectedGroups& selected_groups,
+    std::vector<experiments_helper::NameGroupId>* name_group_ids) {
+  ::GetFieldTrialNameGroupIdsForSelectedGroups(selected_groups, name_group_ids);
+}
+
+uint32 TestHashName(const std::string& name) {
+  return ::HashName(name);
+}
+
+}  // namespace testing