Optimize some startup code related to about:flags handling.

Previously, the code would create a set from all the known
flags and compare the specified flags against them.

But actually this is less efficient than just the O(n*m) search,
because n being the number of flags set by the user and m being
the number of available flags, since n will likely be small or 0
and then we don't need to do a lot of work. In particular, this
also avoids copying a bunch of strings.

Bug: 904575
Change-Id: I63e59d439c432338aea71eb6258e8f8b42f04921
Reviewed-on: https://chromium-review.googlesource.com/c/1334413
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: ssid <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608128}

components/flags_ui/feature_entry.cc

@@ -6,16 +6,48 @@
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace flags_ui {
+namespace {
+
+// WARNING: '@' is also used in the html file. If you update this constant you
+// also need to update the html file.
+const char kMultiSeparatorChar = '@';
+
+}  // namespace
 
 const char kGenericExperimentChoiceDefault[] = "Default";
 const char kGenericExperimentChoiceEnabled[] = "Enabled";
 const char kGenericExperimentChoiceDisabled[] = "Disabled";
 const char kGenericExperimentChoiceAutomatic[] = "Automatic";
 
+bool FeatureEntry::InternalNameMatches(const std::string& name) const {
+  if (!base::StartsWith(name, internal_name, base::CompareCase::SENSITIVE))
+    return false;
+
+  const size_t internal_name_length = strlen(internal_name);
+  switch (type) {
+    case FeatureEntry::SINGLE_VALUE:
+    case FeatureEntry::SINGLE_DISABLE_VALUE:
+    case FeatureEntry::ORIGIN_LIST_VALUE:
+      return name.size() == internal_name_length;
+
+    case FeatureEntry::MULTI_VALUE:
+    case FeatureEntry::ENABLE_DISABLE_VALUE:
+    case FeatureEntry::FEATURE_VALUE:
+    case FeatureEntry::FEATURE_WITH_PARAMS_VALUE:
+      // Check that the pattern matches what's produced by NameForOption().
+      int index = -1;
+      return name.size() > internal_name_length + 1 &&
+             name[internal_name_length] == kMultiSeparatorChar &&
+             base::StringToInt(name.substr(internal_name_length + 1), &index) &&
+             index >= 0 && index < num_options;
+  }
+}
+
 std::string FeatureEntry::NameForOption(int index) const {
   DCHECK(type == FeatureEntry::MULTI_VALUE ||
          type == FeatureEntry::ENABLE_DISABLE_VALUE ||
@@ -100,9 +132,7 @@ const FeatureEntry::FeatureVariation* FeatureEntry::VariationForOption(
 
 namespace testing {
 
-// WARNING: '@' is also used in the html file. If you update this constant you
-// also need to update the html file.
-const char kMultiSeparator[] = "@";
+const char kMultiSeparator[] = {kMultiSeparatorChar, '\0'};
 
 }  // namespace testing
 

components/flags_ui/feature_entry.h

@@ -179,6 +179,11 @@ struct FeatureEntry {
   // FEATURE_WITH_VARIATIONS_VALUE.
   const char* feature_trial_name;
 
+  // Check whether internal |name| matches this FeatureEntry. Depending on the
+  // type of entry, this compared it to either |internal_name| or the values
+  // produced by NameForOption().
+  bool InternalNameMatches(const std::string& name) const;
+
   // Returns the name used in prefs for the option at the specified |index|.
   // Only used for types that use |num_options|.
   std::string NameForOption(int index) const;

components/flags_ui/flags_state.cc

@@ -114,24 +114,6 @@ void AddOsStrings(unsigned bitmask, base::ListValue* list) {
   }
 }
 
-// Adds the internal names for the specified entry to |names|.
-void AddInternalName(const FeatureEntry& e, std::set<std::string>* names) {
-  switch (e.type) {
-    case FeatureEntry::SINGLE_VALUE:
-    case FeatureEntry::SINGLE_DISABLE_VALUE:
-    case FeatureEntry::ORIGIN_LIST_VALUE:
-      names->insert(e.internal_name);
-      break;
-    case FeatureEntry::MULTI_VALUE:
-    case FeatureEntry::ENABLE_DISABLE_VALUE:
-    case FeatureEntry::FEATURE_VALUE:
-    case FeatureEntry::FEATURE_WITH_PARAMS_VALUE:
-      for (int i = 0; i < e.num_options; ++i)
-        names->insert(e.NameForOption(i));
-      break;
-  }
-}
-
 // Confirms that an entry is valid, used in a DCHECK in
 // SanitizeList below.
 bool ValidateFeatureEntry(const FeatureEntry& e) {
@@ -782,54 +764,54 @@ void FlagsState::MergeFeatureCommandLineSwitch(
     command_line->AppendSwitchASCII(switch_name, switch_value);
 }
 
-void FlagsState::SanitizeList(FlagsStorage* flags_storage) const {
-  std::set<std::string> known_entries;
-  for (size_t i = 0; i < num_feature_entries_; ++i) {
-    DCHECK(ValidateFeatureEntry(feature_entries_[i]));
-    AddInternalName(feature_entries_[i], &known_entries);
+std::set<std::string> FlagsState::SanitizeList(
+    const std::set<std::string>& enabled_entries,
+    int platform_mask) const {
+  std::set<std::string> new_enabled_entries;
+
+  // For each entry in |enabled_entries|, check whether it exists in the list
+  // of supported features. Remove those that don't. Note: Even though this is
+  // an O(n^2) search, this is more efficient than creating a set from
+  // |feature_entries_| first because |feature_entries_| is large and
+  // |enabled_entries| should generally be small/empty.
+  const FeatureEntry* features_end = feature_entries_ + num_feature_entries_;
+  for (const std::string& entry_name : enabled_entries) {
+    if (features_end !=
+        std::find_if(feature_entries_, features_end,
+                     [entry_name, platform_mask](const FeatureEntry& e) {
+                       DCHECK(ValidateFeatureEntry(e));
+                       return (e.supported_platforms & platform_mask) &&
+                              e.InternalNameMatches(entry_name);
+                     })) {
+      new_enabled_entries.insert(entry_name);
+    }
   }
 
-  std::set<std::string> enabled_entries = flags_storage->GetFlags();
-
-  std::set<std::string> new_enabled_entries =
-      base::STLSetIntersection<std::set<std::string>>(known_entries,
-                                                      enabled_entries);
-
-  if (new_enabled_entries != enabled_entries)
-    flags_storage->SetFlags(new_enabled_entries);
+  return new_enabled_entries;
 }
 
 void FlagsState::GetSanitizedEnabledFlags(FlagsStorage* flags_storage,
                                           std::set<std::string>* result) const {
-  SanitizeList(flags_storage);
-  *result = flags_storage->GetFlags();
+  std::set<std::string> enabled_entries = flags_storage->GetFlags();
+  std::set<std::string> new_enabled_entries = SanitizeList(enabled_entries, -1);
+  if (new_enabled_entries.size() != enabled_entries.size())
+    flags_storage->SetFlags(new_enabled_entries);
+  result->swap(new_enabled_entries);
 }
 
 void FlagsState::GetSanitizedEnabledFlagsForCurrentPlatform(
     FlagsStorage* flags_storage,
     std::set<std::string>* result) const {
+  // TODO(asvitkine): Consider making GetSanitizedEnabledFlags() do the platform
+  // filtering by default so that we don't need two calls to SanitizeList().
   GetSanitizedEnabledFlags(flags_storage, result);
 
-  // Filter out any entries that aren't enabled on the current platform.  We
-  // don't remove these from prefs else syncing to a platform with a different
-  // set of entries would be lossy.
-  std::set<std::string> platform_entries;
-  int current_platform = GetCurrentPlatform();
-  for (size_t i = 0; i < num_feature_entries_; ++i) {
-    const FeatureEntry& entry = feature_entries_[i];
-    if (entry.supported_platforms & current_platform)
-      AddInternalName(entry, &platform_entries);
+  int platform_mask = GetCurrentPlatform();
 #if defined(OS_CHROMEOS)
-    if (feature_entries_[i].supported_platforms & kOsCrOSOwnerOnly)
-      AddInternalName(entry, &platform_entries);
+  platform_mask |= kOsCrOSOwnerOnly;
 #endif
-  }
-
-  std::set<std::string> new_enabled_entries =
-      base::STLSetIntersection<std::set<std::string>>(platform_entries,
-                                                      *result);
-
-  result->swap(new_enabled_entries);
+  std::set<std::string> platform_entries = SanitizeList(*result, platform_mask);
+  result->swap(platform_entries);
 }
 
 void FlagsState::GenerateFlagsToSwitchesMapping(

components/flags_ui/flags_state.h

@@ -176,10 +176,15 @@ class FlagsState {
       bool feature_state,
       base::CommandLine* command_line);
 
-  // Removes all entries from prefs::kEnabledLabsExperiments that are unknown,
-  // to prevent this list to become very long as entries are added and removed.
-  void SanitizeList(FlagsStorage* flags_storage) const;
+  // Sanitizes |enabled_entries| to only contain entries that are defined in the
+  // |feature_entries_| and whose |supported_platforms| matches |platform_mask|.
+  // Pass -1 to |platform_mask| to not do platform filtering.
+  std::set<std::string> SanitizeList(
+      const std::set<std::string>& enabled_entries,
+      int platform_mask) const;
 
+  // Gets sanitized entries from |flags_storage|, filtering out any entries that
+  // don't exist in |feature_entries_|, and updates |flags_storage|.
   void GetSanitizedEnabledFlags(FlagsStorage* flags_storage,
                                 std::set<std::string>* result) const;