Optimize about_flags::RecordUMAStatistics().

Combines the GetSwitchesFromFlags() and GetFeaturesFromFlags()
calls to avoid GenerateFlagsToSwitchesMapping() being called
twice, which we have evidence of being expensive.

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

chrome/browser/about_flags.cc

@@ -4683,10 +4683,10 @@ void ResetAllFlags(flags_ui::FlagsStorage* flags_storage) {
 }
 
 void RecordUMAStatistics(flags_ui::FlagsStorage* flags_storage) {
-  const std::set<std::string> switches =
-      FlagsStateSingleton::GetFlagsState()->GetSwitchesFromFlags(flags_storage);
-  const std::set<std::string> features =
-      FlagsStateSingleton::GetFlagsState()->GetFeaturesFromFlags(flags_storage);
+  std::set<std::string> switches;
+  std::set<std::string> features;
+  FlagsStateSingleton::GetFlagsState()->GetSwitchesAndFeaturesFromFlags(
+      flags_storage, &switches, &features);
   ReportAboutFlagsHistogram("Launch.FlagsAtStartup", switches, features);
 }
 

components/flags_ui/flags_state.cc

@@ -327,46 +327,30 @@ void FlagsState::ConvertFlagsToSwitches(
                            disable_features_flag_name);
 }
 
-std::set<std::string> FlagsState::GetSwitchesFromFlags(
-    FlagsStorage* flags_storage) {
+void FlagsState::GetSwitchesAndFeaturesFromFlags(
+    FlagsStorage* flags_storage,
+    std::set<std::string>* switches,
+    std::set<std::string>* features) const {
   std::set<std::string> enabled_entries;
   std::map<std::string, SwitchEntry> name_to_switch_map;
   GenerateFlagsToSwitchesMapping(flags_storage, &enabled_entries,
                                  &name_to_switch_map);
 
-  std::set<std::string> switches;
   for (const std::string& entry_name : enabled_entries) {
     const auto& entry_it = name_to_switch_map.find(entry_name);
     DCHECK(entry_it != name_to_switch_map.end());
 
     const SwitchEntry& entry = entry_it->second;
     if (!entry.switch_name.empty())
-      switches.insert("--" + entry.switch_name);
-  }
-  return switches;
-}
+      switches->insert("--" + entry.switch_name);
 
-std::set<std::string> FlagsState::GetFeaturesFromFlags(
-    FlagsStorage* flags_storage) {
-  std::set<std::string> enabled_entries;
-  std::map<std::string, SwitchEntry> name_to_switch_map;
-  GenerateFlagsToSwitchesMapping(flags_storage, &enabled_entries,
-                                 &name_to_switch_map);
-
-  std::set<std::string> features;
-  for (const std::string& entry_name : enabled_entries) {
-    const auto& entry_it = name_to_switch_map.find(entry_name);
-    DCHECK(entry_it != name_to_switch_map.end());
-
-    const SwitchEntry& entry = entry_it->second;
     if (!entry.feature_name.empty()) {
       if (entry.feature_state)
-        features.insert(entry.feature_name + ":enabled");
+        features->insert(entry.feature_name + ":enabled");
       else
-        features.insert(entry.feature_name + ":disabled");
+        features->insert(entry.feature_name + ":disabled");
     }
   }
-  return features;
 }
 
 bool FlagsState::IsRestartNeededToCommitChanges() {
@@ -699,7 +683,7 @@ void FlagsState::AddSwitchMapping(
     const std::string& key,
     const std::string& switch_name,
     const std::string& switch_value,
-    std::map<std::string, SwitchEntry>* name_to_switch_map) {
+    std::map<std::string, SwitchEntry>* name_to_switch_map) const {
   DCHECK(!base::ContainsKey(*name_to_switch_map, key));
 
   SwitchEntry* entry = &(*name_to_switch_map)[key];
@@ -711,7 +695,7 @@ void FlagsState::AddFeatureMapping(
     const std::string& key,
     const std::string& feature_name,
     bool feature_state,
-    std::map<std::string, SwitchEntry>* name_to_switch_map) {
+    std::map<std::string, SwitchEntry>* name_to_switch_map) const {
   DCHECK(!base::ContainsKey(*name_to_switch_map, key));
 
   SwitchEntry* entry = &(*name_to_switch_map)[key];
@@ -798,7 +782,7 @@ void FlagsState::MergeFeatureCommandLineSwitch(
     command_line->AppendSwitchASCII(switch_name, switch_value);
 }
 
-void FlagsState::SanitizeList(FlagsStorage* flags_storage) {
+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]));
@@ -816,14 +800,14 @@ void FlagsState::SanitizeList(FlagsStorage* flags_storage) {
 }
 
 void FlagsState::GetSanitizedEnabledFlags(FlagsStorage* flags_storage,
-                                          std::set<std::string>* result) {
+                                          std::set<std::string>* result) const {
   SanitizeList(flags_storage);
   *result = flags_storage->GetFlags();
 }
 
 void FlagsState::GetSanitizedEnabledFlagsForCurrentPlatform(
     FlagsStorage* flags_storage,
-    std::set<std::string>* result) {
+    std::set<std::string>* result) const {
   GetSanitizedEnabledFlags(flags_storage, result);
 
   // Filter out any entries that aren't enabled on the current platform.  We
@@ -851,7 +835,7 @@ void FlagsState::GetSanitizedEnabledFlagsForCurrentPlatform(
 void FlagsState::GenerateFlagsToSwitchesMapping(
     FlagsStorage* flags_storage,
     std::set<std::string>* enabled_entries,
-    std::map<std::string, SwitchEntry>* name_to_switch_map) {
+    std::map<std::string, SwitchEntry>* name_to_switch_map) const {
   GetSanitizedEnabledFlagsForCurrentPlatform(flags_storage, enabled_entries);
 
   for (size_t i = 0; i < num_feature_entries_; ++i) {

components/flags_ui/flags_state.h

@@ -70,15 +70,13 @@ class FlagsState {
                               const char* enable_features_flag_name,
                               const char* disable_features_flag_name);
 
-  // Reads the state from |flags_storage| and returns a set of strings
-  // corresponding to enabled entries. Does not populate any information about
-  // entries that enable/disable base::Feature states.
-  std::set<std::string> GetSwitchesFromFlags(FlagsStorage* flags_storage);
-
-  // Reads the state from |flags_storage| and returns a set of strings
-  // corresponding to enabled/disabled base::Feature states. Feature names are
-  // suffixed with ":enabled" or ":disabled" depending on their state.
-  std::set<std::string> GetFeaturesFromFlags(FlagsStorage* flags_storage);
+  // Reads the state from |flags_storage| and fills |switches| with the set of
+  // switches corresponding to enabled entries and |features| with the set of
+  // strings corresponding to enabled/disabled base::Feature states. Feature
+  // names are suffixed with ":enabled" or ":disabled" depending on their state.
+  void GetSwitchesAndFeaturesFromFlags(FlagsStorage* flags_storage,
+                                       std::set<std::string>* switches,
+                                       std::set<std::string>* features) const;
 
   bool IsRestartNeededToCommitChanges();
   void SetFeatureEntryEnabled(FlagsStorage* flags_storage,
@@ -142,10 +140,11 @@ class FlagsState {
   struct SwitchEntry;
 
   // Adds mapping to |name_to_switch_map| to set the given switch name/value.
-  void AddSwitchMapping(const std::string& key,
-                        const std::string& switch_name,
-                        const std::string& switch_value,
-                        std::map<std::string, SwitchEntry>* name_to_switch_map);
+  void AddSwitchMapping(
+      const std::string& key,
+      const std::string& switch_name,
+      const std::string& switch_value,
+      std::map<std::string, SwitchEntry>* name_to_switch_map) const;
 
   // Adds mapping to |name_to_switch_map| to toggle base::Feature |feature_name|
   // to state |feature_state|.
@@ -153,7 +152,7 @@ class FlagsState {
       const std::string& key,
       const std::string& feature_name,
       bool feature_state,
-      std::map<std::string, SwitchEntry>* name_to_switch_map);
+      std::map<std::string, SwitchEntry>* name_to_switch_map) const;
 
   // Updates the switches in |command_line| by applying the modifications
   // specified in |name_to_switch_map| for each entry in |enabled_entries|.
@@ -179,16 +178,16 @@ class FlagsState {
 
   // 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);
+  void SanitizeList(FlagsStorage* flags_storage) const;
 
   void GetSanitizedEnabledFlags(FlagsStorage* flags_storage,
-                                std::set<std::string>* result);
+                                std::set<std::string>* result) const;
 
   // Variant of GetSanitizedEnabledFlags that also removes any flags that aren't
   // enabled on the current platform.
   void GetSanitizedEnabledFlagsForCurrentPlatform(
       FlagsStorage* flags_storage,
-      std::set<std::string>* result);
+      std::set<std::string>* result) const;
 
   // Generates a flags to switches mapping based on the set of enabled flags
   // from |flags_storage|. On output, |enabled_entries| will contain the
@@ -197,7 +196,7 @@ class FlagsState {
   void GenerateFlagsToSwitchesMapping(
       FlagsStorage* flags_storage,
       std::set<std::string>* enabled_entries,
-      std::map<std::string, SwitchEntry>* name_to_switch_map);
+      std::map<std::string, SwitchEntry>* name_to_switch_map) const;
 
   // Called when the value of an entry with ORIGIN_LIST_VALUE is modified.
   // Modifies the corresponding command line by adding or removing the switch