Lazy evaluate enterprise status for finch study filtering.

The previous CL crrev.com/c/1789926 introduced filtering finch
studies by enterprise status. However, computing enterprise status
negatively affects performance. This CL only computes enterprise
status if at least 1 study is filtering by enterprise.

Bug: 1001649
Change-Id: Ifd533a6549eac6ba83bcd6b2e00a40653fe5bc97
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1822822
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703072}

components/variations/BUILD.gn

@@ -130,6 +130,7 @@ source_set("unit_tests") {
   sources = [
     "active_field_trials_unittest.cc",
     "child_process_field_trial_syncer_unittest.cc",
+    "client_filterable_state_unittest.cc",
     "entropy_provider_unittest.cc",
     "hashing_unittest.cc",
     "net/variations_command_line_unittest.cc",

components/variations/client_filterable_state.cc

@@ -31,7 +31,15 @@ Study::Platform ClientFilterableState::GetCurrentPlatform() {
 #endif
 }
 
-ClientFilterableState::ClientFilterableState() {}
+ClientFilterableState::ClientFilterableState(
+    IsEnterpriseFunction is_enterprise_function)
+    : is_enterprise_function_(std::move(is_enterprise_function)) {}
 ClientFilterableState::~ClientFilterableState() {}
 
+bool ClientFilterableState::IsEnterprise() const {
+  if (!is_enterprise_.has_value())
+    is_enterprise_ = std::move(is_enterprise_function_).Run();
+  return is_enterprise_.value();
+}
+
 }  // namespace variations

components/variations/client_filterable_state.h

@@ -7,20 +7,29 @@
 
 #include <string>
 
+#include "base/callback.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/time/time.h"
 #include "base/version.h"
 #include "components/variations/proto/study.pb.h"
 
 namespace variations {
 
+using IsEnterpriseFunction = base::OnceCallback<bool()>;
+
 // A container for all of the client state which is used for filtering studies.
 struct ClientFilterableState {
   static Study::Platform GetCurrentPlatform();
 
-  ClientFilterableState();
+  ClientFilterableState(IsEnterpriseFunction is_enterprise_function);
   ~ClientFilterableState();
 
+  // Whether this is an enterprise client. Always false on android, iOS, and
+  // linux. Determined by VariationsServiceClient::IsEnterprise for windows,
+  // chromeOs, and mac.
+  bool IsEnterprise() const;
+
   // The system locale.
   std::string locale;
 
@@ -51,11 +60,6 @@ struct ClientFilterableState {
   // Based on base::SysInfo::IsLowEndDevice().
   bool is_low_end_device = false;
 
-  // Whether this is an enterprise client. Always false on android, iOS, and
-  // linux. Determined by VariationsServiceClient::IsEnterprise for windows,
-  // chromeOs, and mac.
-  bool is_enterprise = false;
-
   // The country code to use for studies configured with session consistency.
   std::string session_consistency_country;
 
@@ -63,6 +67,12 @@ struct ClientFilterableState {
   std::string permanent_consistency_country;
 
  private:
+  // Evaluating enterprise status negatively affects performance, so we only
+  // evaluate it if needed (i.e. if a study is filtering by enterprise) and at
+  // most once.
+  mutable IsEnterpriseFunction is_enterprise_function_;
+  mutable base::Optional<bool> is_enterprise_;
+
   DISALLOW_COPY_AND_ASSIGN(ClientFilterableState);
 };
 

components/variations/client_filterable_state_unittest.cc

+// Copyright 2019 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.
+
+#include "components/variations/client_filterable_state.h"
+
+#include "base/bind.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace variations {
+
+TEST(VariationsClientFilterableStateTest, IsEnterprise) {
+  // Test, for non enterprise clients, is_enterprise_function_ is called once.
+  ClientFilterableState client_non_enterprise(
+      base::BindOnce([] { return false; }));
+  EXPECT_FALSE(client_non_enterprise.IsEnterprise());
+  EXPECT_FALSE(client_non_enterprise.IsEnterprise());
+
+  // Test, for enterprise clients, is_enterprise_function_ is called once.
+  ClientFilterableState client_enterprise(base::BindOnce([] { return true; }));
+  EXPECT_TRUE(client_enterprise.IsEnterprise());
+  EXPECT_TRUE(client_enterprise.IsEnterprise());
+}
+
+}  // namespace variations

components/variations/service/safe_seed_manager_unittest.cc

@@ -81,7 +81,7 @@ class FakeSeedStore : public VariationsSeedStore {
 // |safe_seed_manager|.
 void SetDefaultActiveState(SafeSeedManager* safe_seed_manager) {
   std::unique_ptr<ClientFilterableState> client_state =
-      std::make_unique<ClientFilterableState>();
+      std::make_unique<ClientFilterableState>(base::OnceCallback<bool()>());
   client_state->locale = kTestLocale;
   client_state->permanent_consistency_country =
       kTestPermanentConsistencyCountry;

components/variations/service/variations_field_trial_creator.cc

@@ -234,8 +234,12 @@ bool VariationsFieldTrialCreator::CreateTrialsFromSeed(
 std::unique_ptr<ClientFilterableState>
 VariationsFieldTrialCreator::GetClientFilterableStateForVersion(
     const base::Version& version) {
+  // Note that passing base::Unretained(client_) is safe here because |client_|
+  // lives until Chrome exits.
+  auto IsEnterpriseCallback = base::Bind(&VariationsServiceClient::IsEnterprise,
+                                         base::Unretained(client_));
   std::unique_ptr<ClientFilterableState> state =
-      std::make_unique<ClientFilterableState>();
+      std::make_unique<ClientFilterableState>(IsEnterpriseCallback);
   state->locale = application_locale_;
   state->reference_date = GetReferenceDateForExpiryChecks(local_state());
   state->version = version;
@@ -252,7 +256,6 @@ VariationsFieldTrialCreator::GetClientFilterableStateForVersion(
   // evaluated, that field trial would not be able to apply for this case.
   state->is_low_end_device = base::SysInfo::IsLowEndDevice();
 #endif
-  state->is_enterprise = client_->IsEnterprise();
   state->session_consistency_country = GetLatestCountry();
   state->permanent_consistency_country = LoadPermanentConsistencyCountry(
       version, state->session_consistency_country);

components/variations/study_filtering.cc

@@ -119,8 +119,10 @@ bool CheckStudyLowEndDevice(const Study::Filter& filter,
          filter.is_low_end_device() == is_low_end_device;
 }
 
-bool CheckStudyEnterprise(const Study::Filter& filter, bool is_enterprise) {
-  return !filter.has_is_enterprise() || filter.is_enterprise() == is_enterprise;
+bool CheckStudyEnterprise(const Study::Filter& filter,
+                          const ClientFilterableState& client_state) {
+  return !filter.has_is_enterprise() ||
+         filter.is_enterprise() == client_state.IsEnterprise();
 }
 
 bool CheckStudyStartDate(const Study::Filter& filter,
@@ -278,7 +280,7 @@ bool ShouldAddStudy(const Study& study,
       return false;
     }
 
-    if (!CheckStudyEnterprise(study.filter(), client_state.is_enterprise)) {
+    if (!CheckStudyEnterprise(study.filter(), client_state)) {
       DVLOG(1) << "Filtered out study " << study.name()
                << " due to enterprise state.";
       return false;

components/variations/study_filtering.h

@@ -46,7 +46,8 @@ bool CheckStudyLowEndDevice(const Study::Filter& filter,
                             bool is_low_end_device);
 
 // Checks whether a study is applicable given |is_enterprise| per |filter|.
-bool CheckStudyEnterprise(const Study::Filter& filter, bool is_enterprise);
+bool CheckStudyEnterprise(const Study::Filter& filter,
+                          const ClientFilterableState& client_state);
 
 // Checks whether a study is applicable for the given date/time per |filter|.
 bool CheckStudyStartDate(const Study::Filter& filter,

components/variations/study_filtering_unittest.cc

@@ -10,6 +10,8 @@
 
 #include <vector>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_split.h"
@@ -272,19 +274,22 @@ TEST(VariationsStudyFilteringTest, CheckStudyLowEndDevice) {
 
 TEST(VariationsStudyFilteringTest, CheckStudyEnterprise) {
   Study::Filter filter;
+  ClientFilterableState client_non_enterprise(
+      base::BindOnce([] { return false; }));
+  ClientFilterableState client_enterprise(base::BindOnce([] { return true; }));
 
   // Check that if the filter is not set, study applies to both enterprise and
   // non-enterprise clients.
-  EXPECT_TRUE(internal::CheckStudyEnterprise(filter, true));
-  EXPECT_TRUE(internal::CheckStudyEnterprise(filter, false));
+  EXPECT_TRUE(internal::CheckStudyEnterprise(filter, client_enterprise));
+  EXPECT_TRUE(internal::CheckStudyEnterprise(filter, client_non_enterprise));
 
   filter.set_is_enterprise(true);
-  EXPECT_TRUE(internal::CheckStudyEnterprise(filter, true));
-  EXPECT_FALSE(internal::CheckStudyEnterprise(filter, false));
+  EXPECT_TRUE(internal::CheckStudyEnterprise(filter, client_enterprise));
+  EXPECT_FALSE(internal::CheckStudyEnterprise(filter, client_non_enterprise));
 
   filter.set_is_enterprise(false);
-  EXPECT_FALSE(internal::CheckStudyEnterprise(filter, true));
-  EXPECT_TRUE(internal::CheckStudyEnterprise(filter, false));
+  EXPECT_FALSE(internal::CheckStudyEnterprise(filter, client_enterprise));
+  EXPECT_TRUE(internal::CheckStudyEnterprise(filter, client_non_enterprise));
 }
 
 TEST(VariationsStudyFilteringTest, CheckStudyStartDate) {
@@ -637,7 +642,7 @@ TEST(VariationsStudyFilteringTest, FilterAndValidateStudies) {
   AddExperiment("A", 10, study3);
   AddExperiment("Default", 25, study3);
 
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.locale = "en-CA";
   client_state.reference_date = base::Time::Now();
   client_state.version = base::Version("20.0.0.0");
@@ -696,7 +701,7 @@ TEST(VariationsStudyFilteringTest, FilterAndValidateStudiesWithCountry) {
     if (test.filter_exclude_country)
       study->mutable_filter()->add_exclude_country(test.filter_exclude_country);
 
-    ClientFilterableState client_state;
+    ClientFilterableState client_state({});
     client_state.locale = "en-CA";
     client_state.reference_date = base::Time::Now();
     client_state.version = base::Version("20.0.0.0");
@@ -714,7 +719,7 @@ TEST(VariationsStudyFilteringTest, FilterAndValidateStudiesWithCountry) {
 }
 
 TEST(VariationsStudyFilteringTest, GetClientCountryForStudy_Session) {
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.session_consistency_country = "session_country";
   client_state.permanent_consistency_country = "permanent_country";
 
@@ -725,7 +730,7 @@ TEST(VariationsStudyFilteringTest, GetClientCountryForStudy_Session) {
 }
 
 TEST(VariationsStudyFilteringTest, GetClientCountryForStudy_Permanent) {
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.session_consistency_country = "session_country";
   client_state.permanent_consistency_country = "permanent_country";
 

components/variations/variations_seed_processor_unittest.cc

@@ -13,6 +13,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/format_macros.h"
@@ -266,7 +267,7 @@ TEST_F(VariationsSeedProcessorTest,
   const base::Time year_ago =
       base::Time::Now() - base::TimeDelta::FromDays(365);
 
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.locale = "en-CA";
   client_state.reference_date = base::Time::Now();
   client_state.version = base::Version("20.0.0.0");
@@ -548,7 +549,7 @@ TEST_F(VariationsSeedProcessorTest, StartsActive) {
   AddExperiment("Default", 0, study3);
   study3->set_activation_type(Study_ActivationType_ACTIVATE_ON_QUERY);
 
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.locale = "en-CA";
   client_state.reference_date = base::Time::Now();
   client_state.version = base::Version("20.0.0.0");

components/variations/variations_seed_store_unittest.cc

@@ -108,7 +108,7 @@ VariationsSeed CreateTestSeed() {
 // for testing.
 std::unique_ptr<ClientFilterableState> CreateTestClientFilterableState() {
   std::unique_ptr<ClientFilterableState> client_state =
-      std::make_unique<ClientFilterableState>();
+      std::make_unique<ClientFilterableState>(base::OnceCallback<bool()>());
   client_state->locale = "es-MX";
   client_state->reference_date = WrapTime(1234554321);
   client_state->version = base::Version("1.2.3.4");
@@ -568,7 +568,7 @@ TEST(VariationsSeedStoreTest, LoadSafeSeed_EmptySeed) {
   // Loading an empty seed should return false.
   TestVariationsSeedStore seed_store(&prefs);
   VariationsSeed loaded_seed;
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   base::Time fetch_time;
   EXPECT_FALSE(
       seed_store.LoadSafeSeed(&loaded_seed, &client_state, &fetch_time));
@@ -578,7 +578,7 @@ TEST(VariationsSeedStoreTest, StoreSafeSeed_ValidSeed) {
   const VariationsSeed seed = CreateTestSeed();
   const std::string serialized_seed = SerializeSeed(seed);
   const std::string signature = "a completely ignored signature";
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.locale = "en-US";
   client_state.reference_date = WrapTime(12345);
   client_state.session_consistency_country = "US";
@@ -618,7 +618,7 @@ TEST(VariationsSeedStoreTest, StoreSafeSeed_ValidSeed) {
 TEST(VariationsSeedStoreTest, StoreSafeSeed_EmptySeed) {
   const std::string serialized_seed;
   const std::string signature = "a completely ignored signature";
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.locale = "en-US";
   client_state.reference_date = WrapTime(54321);
   client_state.session_consistency_country = "US";
@@ -663,7 +663,7 @@ TEST(VariationsSeedStoreTest, StoreSafeSeed_EmptySeed) {
 TEST(VariationsSeedStoreTest, StoreSafeSeed_InvalidSeed) {
   const std::string serialized_seed = "a nonsense seed";
   const std::string signature = "a completely ignored signature";
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.locale = "en-US";
   client_state.reference_date = WrapTime(12345);
   client_state.session_consistency_country = "US";
@@ -711,7 +711,7 @@ TEST(VariationsSeedStoreTest, StoreSafeSeed_InvalidSignature) {
   const std::string serialized_seed = SerializeSeed(seed);
   // A valid signature, but for a different seed.
   const std::string signature = kBase64SeedSignature;
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.locale = "en-US";
   client_state.reference_date = WrapTime(12345);
   client_state.session_consistency_country = "US";
@@ -762,7 +762,7 @@ TEST(VariationsSeedStoreTest, StoreSafeSeed_ValidSignature) {
   ASSERT_TRUE(
       base::Base64Decode(kUncompressedBase64SeedData, &serialized_seed));
   const std::string signature = kBase64SeedSignature;
-  ClientFilterableState client_state;
+  ClientFilterableState client_state({});
   client_state.locale = "en-US";
   client_state.reference_date = WrapTime(12345);
   client_state.session_consistency_country = "US";
@@ -807,7 +807,7 @@ TEST(VariationsSeedStoreTest, StoreSafeSeed_IdenticalToLatestSeed) {
   const std::string serialized_seed = SerializeSeed(seed);
   const std::string base64_seed = SerializeSeedBase64(seed);
   const std::string signature = "a completely ignored signature";
-  ClientFilterableState unused_client_state;
+  ClientFilterableState unused_client_state({});
   const base::Time fetch_time = WrapTime(12345);
 
   TestingPrefServiceSimple prefs;
@@ -865,7 +865,7 @@ TEST(VariationsSeedStoreTest, StoreSafeSeed_PreviouslyIdenticalToLatestSeed) {
   const std::string base64_new_seed = SerializeSeedBase64(new_seed);
   const std::string signature = "a completely ignored signature";
   const base::Time fetch_time = WrapTime(12345);
-  ClientFilterableState unused_client_state;
+  ClientFilterableState unused_client_state({});
 
   TestingPrefServiceSimple prefs;
   VariationsSeedStore::RegisterPrefs(prefs.registry());