Remove ConstProp state singletons

Summary: Rewrite with context storage instead.

Reviewed By: xuhdev

Differential Revision: D91178848

fbshipit-source-id: e0f1d4941c151d4475a36c3667095ddaa7f508dc

Author: agampe

opt/constant-propagation/IPConstantPropagation.cpp

@@ -63,6 +63,8 @@ namespace {
 class AnalyzerGenerator {
   ImmutableAttributeAnalyzerState* m_immut_analyzer_state;
   ApiLevelAnalyzerState* m_api_level_analyzer_state;
+  BoxedBooleanAnalyzerState m_boxed_boolean_analyzer_state{};
+  EnumFieldAnalyzerState m_enum_field_analyzer_state;
   StringAnalyzerState* m_string_analyzer_state;
   PackageNameState* m_package_name_state;
   const State& m_cp_state;
@@ -78,13 +80,7 @@ class AnalyzerGenerator {
         m_api_level_analyzer_state(api_level_analyzer_state),
         m_string_analyzer_state(string_analyzer_state),
         m_package_name_state(package_name_state),
-        m_cp_state(cp_state) {
-    // Initialize the singletons that `operator()` needs ahead of time to
-    // avoid a data race.
-    static_cast<void>(EnumFieldAnalyzerState::get());
-    static_cast<void>(BoxedBooleanAnalyzerState::get());
-    static_cast<void>(ApiLevelAnalyzerState::get());
-  }
+        m_cp_state(cp_state) {}
 
   std::unique_ptr<IntraproceduralAnalysis> operator()(
       const DexMethod* method,
@@ -115,7 +111,7 @@ class AnalyzerGenerator {
         &m_cp_state, std::move(wps_accessor), code.cfg(),
         CombinedAnalyzer(
             class_under_init, m_immut_analyzer_state, wps_accessor_ptr,
-            EnumFieldAnalyzerState::get(), BoxedBooleanAnalyzerState::get(),
+            m_enum_field_analyzer_state, m_boxed_boolean_analyzer_state,
             m_string_analyzer_state, nullptr, *m_api_level_analyzer_state,
             m_package_name_state, m_immut_analyzer_state, nullptr),
         std::move(env));
@@ -302,10 +298,9 @@ void PassImpl::run(const DexStoresVector& stores,
   // Hold the analyzer state of ImmutableAttributeAnalyzer.
   ImmutableAttributeAnalyzerState immut_analyzer_state;
   immutable_state::analyze_constructors(scope, &immut_analyzer_state);
-  ApiLevelAnalyzerState api_level_analyzer_state =
-      ApiLevelAnalyzerState::get(min_sdk);
-  auto string_analyzer_state = StringAnalyzerState::get();
-  auto package_name_state = PackageNameState::get(package_name);
+  ApiLevelAnalyzerState api_level_analyzer_state{min_sdk};
+  auto string_analyzer_state = StringAnalyzerState::make_default();
+  auto package_name_state = PackageNameState::make(package_name);
   State cp_state;
   auto fp_iter =
       analyze(scope, &immut_analyzer_state, &api_level_analyzer_state,
@@ -319,8 +314,7 @@ void PassImpl::run(const DexStoresVector& stores,
 void PassImpl::eval_pass(DexStoresVector& /*stores*/,
                          ConfigFiles& /*conf*/,
                          PassManager&) {
-  auto string_analyzer_state = StringAnalyzerState::get();
-  string_analyzer_state.set_methods_as_root();
+  StringAnalyzerState::make_default().set_methods_as_root();
 }
 
 void PassImpl::run_pass(DexStoresVector& stores,

opt/shrinker/ShrinkerPass.cpp

@@ -47,8 +47,8 @@ void ShrinkerPass::bind_config() {
 void ShrinkerPass::eval_pass(DexStoresVector& /*stores*/,
                              ConfigFiles& /*conf*/,
                              PassManager&) {
-  auto string_analyzer_state = constant_propagation::StringAnalyzerState::get();
-  string_analyzer_state.set_methods_as_root();
+  constant_propagation::StringAnalyzerState::make_default()
+      .set_methods_as_root();
 }
 
 void ShrinkerPass::run_pass(DexStoresVector& stores,

opt/shrinker/TailDuplicationPass.cpp

@@ -228,8 +228,8 @@ void TailDuplicationPass::bind_config() {
 void TailDuplicationPass::eval_pass(DexStoresVector&,
                                     ConfigFiles&,
                                     PassManager&) {
-  auto string_analyzer_state = constant_propagation::StringAnalyzerState::get();
-  string_analyzer_state.set_methods_as_root();
+  constant_propagation::StringAnalyzerState::make_default()
+      .set_methods_as_root();
 }
 
 void TailDuplicationPass::run_pass(DexStoresVector& stores,

service/constant-propagation/ConstantPropagationAnalysis.cpp

@@ -1117,31 +1117,6 @@ bool InitFieldAnalyzer::analyze_invoke(const DexType* class_under_init,
   return false;
 }
 
-boost::optional<EnumFieldAnalyzerState> enum_field_singleton{boost::none};
-const EnumFieldAnalyzerState& EnumFieldAnalyzerState::get() {
-  if (!enum_field_singleton) {
-    // Be careful, there could be a data race here if this is called in parallel
-    enum_field_singleton = EnumFieldAnalyzerState();
-    // In tests, we create and destroy g_redex repeatedly. So we need to reset
-    // the singleton.
-    g_redex->add_destruction_task([]() { enum_field_singleton = boost::none; });
-  }
-  return *enum_field_singleton;
-}
-
-boost::optional<BoxedBooleanAnalyzerState> boxed_boolean_singleton{boost::none};
-const BoxedBooleanAnalyzerState& BoxedBooleanAnalyzerState::get() {
-  if (!boxed_boolean_singleton) {
-    // Be careful, there could be a data race here if this is called in parallel
-    boxed_boolean_singleton = BoxedBooleanAnalyzerState();
-    // In tests, we create and destroy g_redex repeatedly. So we need to reset
-    // the singleton.
-    g_redex->add_destruction_task(
-        []() { boxed_boolean_singleton = boost::none; });
-  }
-  return *boxed_boolean_singleton;
-}
-
 bool EnumFieldAnalyzer::analyze_sget(const EnumFieldAnalyzerState&,
                                      const IRInstruction* insn,
                                      ConstantEnvironment* env) {
@@ -1259,27 +1234,15 @@ bool BoxedBooleanAnalyzer::analyze_invoke(
   }
 }
 
-static boost::optional<StringAnalyzerState> s_string_analyzer_state{
-    boost::none};
-static std::mutex s_string_analyzer_state_mtx;
-StringAnalyzerState StringAnalyzerState::get() {
-  std::lock_guard<std::mutex> lock(s_string_analyzer_state_mtx);
-  if (s_string_analyzer_state == boost::none) {
-    UnorderedSet<DexMethod*> methods;
-    auto* kotlin_are_equal = DexMethod::get_method(
-        "Lkotlin/jvm/internal/Intrinsics;.areEqual:(Ljava/lang/Object;Ljava/"
-        "lang/Object;)Z");
-    if (kotlin_are_equal != nullptr && kotlin_are_equal->as_def() != nullptr) {
-      methods.emplace(kotlin_are_equal->as_def());
-    }
-    s_string_analyzer_state = StringAnalyzerState(methods);
-    // For tests.
-    g_redex->add_destruction_task([]() {
-      std::lock_guard<std::mutex> task_lock(s_string_analyzer_state_mtx);
-      s_string_analyzer_state = boost::none;
-    });
+StringAnalyzerState StringAnalyzerState::make_default() {
+  UnorderedSet<DexMethod*> methods;
+  auto* kotlin_are_equal = DexMethod::get_method(
+      "Lkotlin/jvm/internal/Intrinsics;.areEqual:(Ljava/lang/Object;Ljava/"
+      "lang/Object;)Z");
+  if (kotlin_are_equal != nullptr && kotlin_are_equal->as_def() != nullptr) {
+    methods.emplace(kotlin_are_equal->as_def());
   }
-  return *s_string_analyzer_state;
+  return StringAnalyzerState(methods);
 }
 
 void StringAnalyzerState::set_methods_as_root() {
@@ -1826,17 +1789,9 @@ bool EnumUtilsFieldAnalyzer::analyze_sget(
   return true;
 }
 
-boost::optional<DexFieldRef*> g_sdk_int_field{boost::none};
-ApiLevelAnalyzerState ApiLevelAnalyzerState::get(int32_t min_sdk) {
-  if (!g_sdk_int_field) {
-    // Be careful, there could be a data race here if this is called in parallel
-    g_sdk_int_field =
-        DexField::get_field("Landroid/os/Build$VERSION;.SDK_INT:I");
-    // In tests, we create and destroy g_redex repeatedly. So we need to reset
-    // the singleton.
-    g_redex->add_destruction_task([]() { g_sdk_int_field = boost::none; });
-  }
-  return {*g_sdk_int_field, min_sdk};
+ApiLevelAnalyzerState::ApiLevelAnalyzerState(int32_t min_sdk)
+    : min_sdk(min_sdk) {
+  sdk_int_field = DexField::get_field("Landroid/os/Build$VERSION;.SDK_INT:I");
 }
 
 bool ApiLevelAnalyzer::analyze_sget(const ApiLevelAnalyzerState& state,
@@ -1853,35 +1808,18 @@ bool ApiLevelAnalyzer::analyze_sget(const ApiLevelAnalyzerState& state,
   return false;
 }
 
-boost::optional<UnorderedSet<DexMethodRef*>> g_get_package_name_refs{
-    boost::none};
-static std::mutex s_package_name_mutex;
-PackageNameState PackageNameState::get(const std::string& package_name) {
-  return PackageNameState::get(boost::optional<std::string>(package_name));
-}
-
-PackageNameState PackageNameState::get(
+PackageNameState PackageNameState::make(
     const boost::optional<std::string>& package_name) {
-  std::lock_guard<std::mutex> lock(s_package_name_mutex);
-  if (!g_get_package_name_refs) {
-    UnorderedSet<DexMethodRef*> refs{
-        DexMethod::get_method(
-            "Landroid/content/Context;.getPackageName:()Ljava/lang/String;"),
-        DexMethod::get_method(
-            "Landroid/content/ContextWrapper;.getPackageName:()Ljava/lang/"
-            "String;")};
-    g_get_package_name_refs = refs;
-    // In tests, we create and destroy g_redex repeatedly. So we need to reset
-    // the singleton.
-    g_redex->add_destruction_task([]() {
-      std::lock_guard<std::mutex> task_lock(s_package_name_mutex);
-      g_get_package_name_refs = boost::none;
-    });
-  }
+  UnorderedSet<DexMethodRef*> refs{
+      DexMethod::get_method(
+          "Landroid/content/Context;.getPackageName:()Ljava/lang/String;"),
+      DexMethod::get_method(
+          "Landroid/content/ContextWrapper;.getPackageName:()Ljava/lang/"
+          "String;")};
   const DexString* dex_string = package_name != boost::none
                                     ? DexString::make_string(*package_name)
                                     : nullptr;
-  return {*g_get_package_name_refs, dex_string};
+  return {std::move(refs), dex_string};
 }
 
 bool PackageNameAnalyzer::analyze_invoke(const PackageNameState* state,

service/constant-propagation/ConstantPropagationAnalysis.h

@@ -279,11 +279,9 @@ class InitFieldAnalyzer final
                              ConstantEnvironment* env);
 };
 
+// Construction of this object is expensive because get_method acquires a
+// global lock. Make sure to cache it!
 struct EnumFieldAnalyzerState {
-  // Construction of this object is expensive because get_method acquires a
-  // global lock. `get` creates a singleton once and reuses it.
-  static const EnumFieldAnalyzerState& get();
-
   const DexMethod* enum_equals;
 
   EnumFieldAnalyzerState()
@@ -402,11 +400,9 @@ class ImmutableAttributeAnalyzer final
                            ConstantEnvironment* env);
 };
 
+// Construction of this object is expensive because get_type/field/method
+// acquires a global lock. Make sure to cache it.
 struct BoxedBooleanAnalyzerState {
-  // Construction of this object is expensive because get_type/field/method
-  // acquires a global lock. `get` creates a singleton once and reuses it.
-  static const BoxedBooleanAnalyzerState& get();
-
   const DexType* boolean_class{DexType::get_type("Ljava/lang/Boolean;")};
   const DexField* boolean_true{dynamic_cast<DexField*>(
       DexField::get_field("Ljava/lang/Boolean;.TRUE:Ljava/lang/Boolean;"))};
@@ -433,9 +429,11 @@ class BoxedBooleanAnalyzer final
 };
 
 struct StringAnalyzerState {
-  static StringAnalyzerState get();
   explicit StringAnalyzerState(const UnorderedSet<DexMethod*>& methods)
       : string_equality_methods(methods) {}
+
+  static StringAnalyzerState make_default();
+
   void set_methods_as_root();
   UnorderedSet<DexMethod*> string_equality_methods;
 };
@@ -467,10 +465,10 @@ class ConstantClassObjectAnalyzer
   }
 };
 
+// Construction of this object is expensive because get_method acquires a
+// global lock. Make sure to cache it!
 struct ApiLevelAnalyzerState {
-  // Construction of this object is expensive because get_method acquires a
-  // global lock. `get` caches those lookups.
-  static ApiLevelAnalyzerState get(int32_t min_sdk = 0);
+  explicit ApiLevelAnalyzerState(int32_t min_sdk);
 
   const DexFieldRef* sdk_int_field;
   int32_t min_sdk;
@@ -487,11 +485,11 @@ class ApiLevelAnalyzer final
 };
 
 struct PackageNameState {
-  static PackageNameState get(const std::string& package_name);
-  static PackageNameState get(const boost::optional<std::string>& package_name);
-
   const UnorderedSet<DexMethodRef*> getter_methods;
   const DexString* package_name;
+
+  static PackageNameState make(
+      const boost::optional<std::string>& package_name);
 };
 
 class PackageNameAnalyzer final

service/method-inliner/CallSiteSummaries.cpp

@@ -178,6 +178,13 @@ const CallSiteSummary* CallSiteSummarizer::internalize_call_site_summary(
   return m_call_site_summaries.insert(call_site_summary).first;
 }
 
+struct CallSiteSummarizer::InvokeCallSiteSummariesContext {
+  constant_propagation::ApiLevelAnalyzerState api_level_analyzer_state{0};
+  constant_propagation::BoxedBooleanAnalyzerState
+      boxed_boolean_analyzer_state{};
+  constant_propagation::EnumFieldAnalyzerState enum_field_analyzer_state;
+};
+
 void CallSiteSummarizer::summarize() {
   Timer t("compute_call_site_summaries");
 
@@ -204,6 +211,8 @@ void CallSiteSummarizer::summarize() {
     return ptr;
   };
 
+  InvokeCallSiteSummariesContext context{};
+
   summaries_scheduler.set_executor([&](DexMethod* method) {
     CallSiteArguments arguments;
     if (get_dependencies(method) == nullptr) {
@@ -243,7 +252,8 @@ void CallSiteSummarizer::summarize() {
     ConstantEnvironment initial_env =
         constant_propagation::interprocedural::env_with_params(
             is_static(method), method->get_code(), arguments);
-    auto res = get_invoke_call_site_summaries(method, callees, initial_env);
+    auto res =
+        get_invoke_call_site_summaries(method, callees, initial_env, context);
     for (auto& p : res.invoke_call_site_summaries) {
       auto* insn = p.first;
       auto* callee = m_get_callee_fn(method, insn);
@@ -287,7 +297,8 @@ InvokeCallSiteSummariesAndDeadBlocks
 CallSiteSummarizer::get_invoke_call_site_summaries(
     DexMethod* caller,
     const UnorderedMap<DexMethod*, size_t>& callees,
-    const ConstantEnvironment& initial_env) {
+    const ConstantEnvironment& initial_env,
+    const InvokeCallSiteSummariesContext& context) {
   IRCode* code = caller->get_code();
 
   InvokeCallSiteSummariesAndDeadBlocks res;
@@ -298,12 +309,11 @@ CallSiteSummarizer::get_invoke_call_site_summaries(
       constant_propagation::ConstantPrimitiveAndBoxedAnalyzer(
           m_shrinker.get_immut_analyzer_state(),
           m_shrinker.get_immut_analyzer_state(),
-          constant_propagation::EnumFieldAnalyzerState::get(),
-          constant_propagation::BoxedBooleanAnalyzerState::get(),
+          context.enum_field_analyzer_state,
+          context.boxed_boolean_analyzer_state,
           m_shrinker.get_string_analyzer_state(),
-          constant_propagation::ApiLevelAnalyzerState::get(),
-          m_shrinker.get_package_name_state(), nullptr,
-          m_shrinker.get_immut_analyzer_state(), nullptr));
+          context.api_level_analyzer_state, m_shrinker.get_package_name_state(),
+          nullptr, m_shrinker.get_immut_analyzer_state(), nullptr));
   intra_cp.run(initial_env);
   for (const auto& block : cfg.blocks()) {
     auto env = intra_cp.get_entry_state_at(block);

service/method-inliner/CallSiteSummaries.h

@@ -111,16 +111,6 @@ class CallSiteSummarizer {
   InsertOnlyConcurrentSet<CallSiteSummary, CallSiteSummaryHasher>
       m_call_site_summaries;
 
-  /**
-   * For all (reachable) invoke instructions in a given method, collect
-   * information about their arguments, i.e. whether particular arguments
-   * are constants.
-   */
-  InvokeCallSiteSummariesAndDeadBlocks get_invoke_call_site_summaries(
-      DexMethod* caller,
-      const UnorderedMap<DexMethod*, size_t>& callees,
-      const ConstantEnvironment& initial_env);
-
  public:
   CallSiteSummarizer(
       shrinker::Shrinker& shrinker,
@@ -144,6 +134,19 @@ class CallSiteSummarizer {
 
   const CallSiteSummary* get_instruction_call_site_summary(
       const IRInstruction* invoke_insn) const;
+
+  struct InvokeCallSiteSummariesContext;
+
+  /**
+   * For all (reachable) invoke instructions in a given method, collect
+   * information about their arguments, i.e. whether particular arguments
+   * are constants.
+   */
+  InvokeCallSiteSummariesAndDeadBlocks get_invoke_call_site_summaries(
+      DexMethod* caller,
+      const UnorderedMap<DexMethod*, size_t>& callees,
+      const ConstantEnvironment& initial_env,
+      const InvokeCallSiteSummariesContext& context);
 };
 
 } // namespace inliner