Back out "run dce on methods affected by code changes"

Summary: Seems to have introduced some nondetermism

Reviewed By: wsanville

Differential Revision: D51860446

fbshipit-source-id: b27aaa380a2f078f191efd76e259e774f7f042ad

libredex/Reachability.cpp

@@ -1905,8 +1905,7 @@ void ReachableAspects::finish(const ConditionallyMarked& cond_marked,
 }
 
 std::unique_ptr<ReachableObjects> compute_reachable_objects(
-    const Scope& scope,
-    const method_override_graph::Graph& method_override_graph,
+    const DexStoresVector& stores,
     const IgnoreSets& ignore_sets,
     int* num_ignore_check_strings,
     ReachableAspects* reachable_aspects,
@@ -1917,14 +1916,17 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects(
     bool cfg_gathering_check_instance_callable,
     bool cfg_gathering_check_returning,
     bool should_mark_all_as_seed,
+    std::unique_ptr<const mog::Graph>* out_method_override_graph,
     bool remove_no_argument_constructors) {
   Timer t("Marking");
+  auto scope = build_class_scope(stores);
   std::unordered_set<const DexClass*> scope_set(scope.begin(), scope.end());
   auto reachable_objects = std::make_unique<ReachableObjects>();
   ConditionallyMarked cond_marked;
+  auto method_override_graph = mog::build_graph(scope);
 
   ConcurrentSet<ReachableObject, ReachableObjectHash> root_set;
-  RootSetMarker root_set_marker(method_override_graph, record_reachability,
+  RootSetMarker root_set_marker(*method_override_graph, record_reachability,
                                 relaxed_keep_class_members,
                                 remove_no_argument_constructors, &cond_marked,
                                 reachable_objects.get(), &root_set);
@@ -1940,7 +1942,7 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects(
   TransitiveClosureMarkerSharedState shared_state{
       std::move(scope_set),
       &ignore_sets,
-      &method_override_graph,
+      method_override_graph.get(),
       record_reachability,
       relaxed_keep_class_members,
       relaxed_keep_interfaces,
@@ -1961,13 +1963,17 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects(
       },
       root_set, num_threads,
       /*push_tasks_while_running=*/true);
-  compute_zombie_methods(method_override_graph, *reachable_objects,
+  compute_zombie_methods(*method_override_graph, *reachable_objects,
                          *reachable_aspects);
 
   if (num_ignore_check_strings != nullptr) {
     *num_ignore_check_strings = (int)stats.num_ignore_check_strings;
   }
 
+  if (out_method_override_graph) {
+    *out_method_override_graph = std::move(method_override_graph);
+  }
+
   reachable_aspects->finish(cond_marked, *reachable_objects);
 
   return reachable_objects;

libredex/Reachability.h

@@ -691,8 +691,7 @@ class TransitiveClosureMarkerWorker {
  * (e.g. proguard rules).
  */
 std::unique_ptr<ReachableObjects> compute_reachable_objects(
-    const Scope& scope,
-    const method_override_graph::Graph& method_override_graph,
+    const DexStoresVector& stores,
     const IgnoreSets& ignore_sets,
     int* num_ignore_check_strings,
     ReachableAspects* reachable_aspects,
@@ -703,6 +702,8 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects(
     bool cfg_gathering_check_instance_callable = false,
     bool cfg_gathering_check_returning = false,
     bool should_mark_all_as_seed = false,
+    std::unique_ptr<const method_override_graph::Graph>*
+        out_method_override_graph = nullptr,
     bool remove_no_argument_constructors = false);
 
 void compute_zombie_methods(

opt/remove-unreachable/RemoveUnreachable.cpp

@@ -14,8 +14,6 @@
 #include "ConfigFiles.h"
 #include "DexUtil.h"
 #include "IOUtil.h"
-#include "InitClassesWithSideEffects.h"
-#include "LocalDce.h"
 #include "MethodOverrideGraph.h"
 #include "PassManager.h"
 #include "Show.h"
@@ -178,18 +176,6 @@ void RemoveUnreachablePassBase::run_pass(DexStoresVector& stores,
   // Store names of removed classes and methods
   ConcurrentSet<std::string> removed_symbols;
 
-  auto sweep_code = m_prune_uninstantiable_insns || m_throw_propagation;
-  auto scope = build_class_scope(stores);
-  always_assert(!pm.unreliable_virtual_scopes());
-  auto method_override_graph = mog::build_graph(scope);
-  std::unique_ptr<init_classes::InitClassesWithSideEffects>
-      init_classes_with_side_effects;
-  if (sweep_code && !pm.init_class_lowering_has_run()) {
-    init_classes_with_side_effects =
-        std::make_unique<init_classes::InitClassesWithSideEffects>(
-            scope, conf.create_init_class_insns(), method_override_graph.get());
-  }
-
   root_metrics(stores, pm);
 
   bool emit_graph_this_run =
@@ -207,8 +193,8 @@ void RemoveUnreachablePassBase::run_pass(DexStoresVector& stores,
   int num_ignore_check_strings = 0;
   reachability::ReachableAspects reachable_aspects;
   auto reachables = this->compute_reachable_objects(
-      scope, *method_override_graph, pm, &num_ignore_check_strings,
-      &reachable_aspects, emit_graph_this_run, m_relaxed_keep_class_members,
+      stores, pm, &num_ignore_check_strings, &reachable_aspects,
+      emit_graph_this_run, m_relaxed_keep_class_members,
       m_prune_unreferenced_interfaces, m_prune_uninstantiable_insns,
       m_prune_uncallable_instance_method_bodies, m_throw_propagation,
       m_remove_no_argument_constructors);
@@ -233,7 +219,7 @@ void RemoveUnreachablePassBase::run_pass(DexStoresVector& stores,
   auto abstracted_classes = reachability::mark_classes_abstract(
       stores, *reachables, reachable_aspects);
   pm.incr_metric("abstracted_classes", abstracted_classes.size());
-  if (sweep_code) {
+  if (m_prune_uninstantiable_insns || m_throw_propagation) {
     remove_uninstantiables_impl::Stats remove_uninstantiables_stats;
     std::atomic<size_t> throws_inserted{0};
     InsertOnlyConcurrentSet<DexMethod*> affected_methods;
@@ -244,23 +230,6 @@ void RemoveUnreachablePassBase::run_pass(DexStoresVector& stores,
     remove_uninstantiables_stats.report(pm);
     pm.incr_metric("throws_inserted", (size_t)throws_inserted);
     pm.incr_metric("methods_with_code_changes", affected_methods.size());
-    std::unordered_set<DexMethodRef*> pure_methods;
-    LocalDce::Stats dce_stats;
-    std::mutex dce_stats_mutex;
-    workqueue_run<DexMethod*>(
-        [&](DexMethod* method) {
-          LocalDce dce(init_classes_with_side_effects.get(), pure_methods);
-          dce.dce(method->get_code()->cfg(), /* normalize_new_instances */ true,
-                  method->get_class());
-          auto local_stats = dce.get_stats();
-          std::lock_guard<std::mutex> lock(dce_stats_mutex);
-          dce_stats += local_stats;
-        },
-        affected_methods);
-    pm.incr_metric("instructions_eliminated_localdce_dead",
-                   dce_stats.dead_instruction_count);
-    pm.incr_metric("instructions_eliminated_localdce_unreachable",
-                   dce_stats.unreachable_instruction_count);
   }
   reachability::sweep(stores, *reachables,
                       output_unreachable_symbols ? &removed_symbols : nullptr,
@@ -299,7 +268,7 @@ void RemoveUnreachablePassBase::run_pass(DexStoresVector& stores,
     {
       std::ofstream os;
       open_or_die(conf.metafile("method-override-graph"), &os);
-      method_override_graph = mog::build_graph(build_class_scope(stores));
+      auto method_override_graph = mog::build_graph(build_class_scope(stores));
       method_override_graph->dump(os);
     }
   }
@@ -332,8 +301,7 @@ void RemoveUnreachablePassBase::write_out_removed_symbols(
 
 std::unique_ptr<reachability::ReachableObjects>
 RemoveUnreachablePass::compute_reachable_objects(
-    const Scope& scope,
-    const method_override_graph::Graph& method_override_graph,
+    const DexStoresVector& stores,
     PassManager& /* pm */,
     int* num_ignore_check_strings,
     reachability::ReachableAspects* reachable_aspects,
@@ -345,11 +313,11 @@ RemoveUnreachablePass::compute_reachable_objects(
     bool cfg_gathering_check_returning,
     bool remove_no_argument_constructors) {
   return reachability::compute_reachable_objects(
-      scope, method_override_graph, m_ignore_sets, num_ignore_check_strings,
-      reachable_aspects, emit_graph_this_run, relaxed_keep_class_members,
-      relaxed_keep_interfaces, cfg_gathering_check_instantiable,
-      cfg_gathering_check_instance_callable, cfg_gathering_check_returning,
-      false, remove_no_argument_constructors);
+      stores, m_ignore_sets, num_ignore_check_strings, reachable_aspects,
+      emit_graph_this_run, relaxed_keep_class_members, relaxed_keep_interfaces,
+      cfg_gathering_check_instantiable, cfg_gathering_check_instance_callable,
+      cfg_gathering_check_returning, false, nullptr,
+      remove_no_argument_constructors);
 }
 
 static RemoveUnreachablePass s_pass;

opt/remove-unreachable/RemoveUnreachable.h

@@ -64,19 +64,17 @@ class RemoveUnreachablePassBase : public Pass {
   void run_pass(DexStoresVector&, ConfigFiles&, PassManager&) override;
 
   virtual std::unique_ptr<reachability::ReachableObjects>
-  compute_reachable_objects(
-      const Scope& scope,
-      const method_override_graph::Graph& method_override_graph,
-      PassManager& pm,
-      int* num_ignore_check_strings,
-      reachability::ReachableAspects* reachable_aspects,
-      bool emit_graph_this_run,
-      bool relaxed_keep_class_members,
-      bool relaxed_keep_interfaces,
-      bool cfg_gathering_check_instantiable,
-      bool cfg_gathering_check_instance_callable,
-      bool cfg_gathering_check_returning,
-      bool remove_no_argument_constructors) = 0;
+  compute_reachable_objects(const DexStoresVector& stores,
+                            PassManager& pm,
+                            int* num_ignore_check_strings,
+                            reachability::ReachableAspects* reachable_aspects,
+                            bool emit_graph_this_run,
+                            bool relaxed_keep_class_members,
+                            bool relaxed_keep_interfaces,
+                            bool cfg_gathering_check_instantiable,
+                            bool cfg_gathering_check_instance_callable,
+                            bool cfg_gathering_check_returning,
+                            bool remove_no_argument_constructors) = 0;
 
   void write_out_removed_symbols(
       const std::string& filepath,
@@ -103,8 +101,7 @@ class RemoveUnreachablePass : public RemoveUnreachablePassBase {
       : RemoveUnreachablePassBase("RemoveUnreachablePass") {}
 
   std::unique_ptr<reachability::ReachableObjects> compute_reachable_objects(
-      const Scope& scope,
-      const method_override_graph::Graph& method_override_graph,
+      const DexStoresVector& stores,
       PassManager& pm,
       int* num_ignore_check_strings,
       reachability::ReachableAspects* reachable_aspects,

opt/remove-unreachable/TypeAnalysisAwareRemoveUnreachable.cpp

@@ -339,8 +339,7 @@ class TypeAnalysisAwareClosureMarkerWorker final
 };
 
 std::unique_ptr<ReachableObjects> compute_reachable_objects_with_type_anaysis(
-    const Scope& scope,
-    const method_override_graph::Graph& method_override_graph,
+    const DexStoresVector& stores,
     const IgnoreSets& ignore_sets,
     int* num_ignore_check_strings,
     ReachableAspects* reachable_aspects,
@@ -356,16 +355,18 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects_with_type_anaysis(
     int* num_unreachable_invokes,
     int* num_null_invokes) {
   Timer t("Marking");
+  auto scope = build_class_scope(stores);
   std::unordered_set<const DexClass*> scope_set(scope.begin(), scope.end());
   walk::parallel::code(scope, [](DexMethod*, IRCode& code) {
     code.cfg().calculate_exit_block();
   });
   auto reachable_objects = std::make_unique<ReachableObjects>();
   ConditionallyMarked cond_marked;
+  auto method_override_graph = mog::build_graph(scope);
 
   ConcurrentSet<ReachableObject, ReachableObjectHash> root_set;
   bool remove_no_argument_constructors = false;
-  RootSetMarker root_set_marker(method_override_graph, record_reachability,
+  RootSetMarker root_set_marker(*method_override_graph, record_reachability,
                                 relaxed_keep_class_members,
                                 remove_no_argument_constructors, &cond_marked,
                                 reachable_objects.get(), &root_set);
@@ -374,7 +375,7 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects_with_type_anaysis(
   size_t num_threads = redex_parallel::default_num_threads();
   Stats stats;
   TypeAnalysisAwareClosureMarkerSharedState shared_state{
-      {std::move(scope_set), &ignore_sets, &method_override_graph,
+      {std::move(scope_set), &ignore_sets, method_override_graph.get(),
        record_reachability, relaxed_keep_class_members, relaxed_keep_interfaces,
        cfg_gathering_check_instantiable, cfg_gathering_check_instance_callable,
        cfg_gathering_check_returning, &cond_marked, reachable_objects.get(),
@@ -391,7 +392,7 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects_with_type_anaysis(
       },
       root_set, num_threads,
       /* push_tasks_while_running*/ true);
-  compute_zombie_methods(method_override_graph, *reachable_objects,
+  compute_zombie_methods(*method_override_graph, *reachable_objects,
                          *reachable_aspects);
 
   if (num_ignore_check_strings != nullptr) {
@@ -416,8 +417,7 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects_with_type_anaysis(
 
 std::unique_ptr<reachability::ReachableObjects>
 TypeAnalysisAwareRemoveUnreachablePass::compute_reachable_objects(
-    const Scope& scope,
-    const method_override_graph::Graph& method_override_graph,
+    const DexStoresVector& stores,
     PassManager& pm,
     int* num_ignore_check_strings,
     reachability::ReachableAspects* reachable_aspects,
@@ -438,12 +438,11 @@ TypeAnalysisAwareRemoveUnreachablePass::compute_reachable_objects(
   int num_unreachable_invokes;
   int num_null_invokes;
   auto res = compute_reachable_objects_with_type_anaysis(
-      scope, method_override_graph, m_ignore_sets, num_ignore_check_strings,
-      reachable_aspects, emit_graph_this_run, relaxed_keep_class_members,
-      relaxed_keep_interfaces, cfg_gathering_check_instantiable,
-      cfg_gathering_check_instance_callable, cfg_gathering_check_returning,
-      gta.get(), remove_no_argument_constructors, &num_exact_resolved_callees,
-      &num_unreachable_invokes, &num_null_invokes);
+      stores, m_ignore_sets, num_ignore_check_strings, reachable_aspects,
+      emit_graph_this_run, relaxed_keep_class_members, relaxed_keep_interfaces,
+      cfg_gathering_check_instantiable, cfg_gathering_check_instance_callable,
+      cfg_gathering_check_returning, gta.get(), remove_no_argument_constructors,
+      &num_exact_resolved_callees, &num_unreachable_invokes, &num_null_invokes);
   pm.incr_metric("num_exact_resolved_callees", num_exact_resolved_callees);
   pm.incr_metric("num_unreachable_invokes", num_unreachable_invokes);
   pm.incr_metric("num_null_invokes", num_null_invokes);

opt/remove-unreachable/TypeAnalysisAwareRemoveUnreachable.h

@@ -23,8 +23,7 @@ class TypeAnalysisAwareRemoveUnreachablePass
   }
 
   std::unique_ptr<reachability::ReachableObjects> compute_reachable_objects(
-      const Scope& scope,
-      const method_override_graph::Graph& method_override_graph,
+      const DexStoresVector& stores,
       PassManager& pm,
       int* num_ignore_check_strings,
       reachability::ReachableAspects* reachable_aspects,

test/integ/FlowSensitiveReachabilityTest.cpp

@@ -31,11 +31,9 @@ TEST_F(FlowSensitiveReachabilityTest,
   reachability::ReachableAspects reachable_aspects;
   auto scope = build_class_scope(stores);
   walk::parallel::code(scope, [&](auto*, auto& code) { code.build_cfg(); });
-  auto method_override_graph = method_override_graph::build_graph(scope);
 
   auto reachable_objects = reachability::compute_reachable_objects(
-      scope, *method_override_graph, ig_sets, &num_ignore_check_strings,
-      &reachable_aspects, false,
+      stores, ig_sets, &num_ignore_check_strings, &reachable_aspects, false,
       /* relaxed_keep_class_members */ true,
       /* relaxed_keep_interfaces */ false,
       /* cfg_gathering_check_instantiable */ true);
@@ -114,11 +112,9 @@ TEST_F(FlowSensitiveReachabilityTest, cfg_gathering_check_instance_callable) {
   reachability::ReachableAspects reachable_aspects;
   auto scope = build_class_scope(stores);
   walk::parallel::code(scope, [&](auto*, auto& code) { code.build_cfg(); });
-  auto method_override_graph = method_override_graph::build_graph(scope);
 
   auto reachable_objects = reachability::compute_reachable_objects(
-      scope, *method_override_graph, ig_sets, &num_ignore_check_strings,
-      &reachable_aspects, false,
+      stores, ig_sets, &num_ignore_check_strings, &reachable_aspects, false,
       /* relaxed_keep_class_members */ true,
       /* relaxed_keep_interfaces */ false,
       /* cfg_gathering_check_instantiable */ true,
@@ -199,11 +195,9 @@ TEST_F(FlowSensitiveReachabilityTest, sweep_uncallable_virtual_methods) {
   reachability::ReachableAspects reachable_aspects;
   auto scope = build_class_scope(stores);
   walk::parallel::code(scope, [&](auto*, auto& code) { code.build_cfg(); });
-  auto method_override_graph = method_override_graph::build_graph(scope);
 
   auto reachable_objects = reachability::compute_reachable_objects(
-      scope, *method_override_graph, ig_sets, &num_ignore_check_strings,
-      &reachable_aspects, false,
+      stores, ig_sets, &num_ignore_check_strings, &reachable_aspects, false,
       /* relaxed_keep_class_members */ true,
       /* relaxed_keep_interfaces */ false,
       /* cfg_gathering_check_instantiable */ true,
@@ -294,11 +288,9 @@ TEST_F(FlowSensitiveReachabilityTest, abstract_overrides_non_abstract) {
   reachability::ReachableAspects reachable_aspects;
   auto scope = build_class_scope(stores);
   walk::parallel::code(scope, [&](auto*, auto& code) { code.build_cfg(); });
-  auto method_override_graph = method_override_graph::build_graph(scope);
 
   auto reachable_objects = reachability::compute_reachable_objects(
-      scope, *method_override_graph, ig_sets, &num_ignore_check_strings,
-      &reachable_aspects, false,
+      stores, ig_sets, &num_ignore_check_strings, &reachable_aspects, false,
       /* relaxed_keep_class_members */ true,
       /* relaxed_keep_interfaces */ false,
       /* cfg_gathering_check_instantiable */ true);
@@ -341,11 +333,9 @@ TEST_F(FlowSensitiveReachabilityTest, throw_propagation) {
   reachability::ReachableAspects reachable_aspects;
   auto scope = build_class_scope(stores);
   walk::parallel::code(scope, [&](auto*, auto& code) { code.build_cfg(); });
-  auto method_override_graph = method_override_graph::build_graph(scope);
 
   auto reachable_objects = reachability::compute_reachable_objects(
-      scope, *method_override_graph, ig_sets, &num_ignore_check_strings,
-      &reachable_aspects, false,
+      stores, ig_sets, &num_ignore_check_strings, &reachable_aspects, false,
       /* relaxed_keep_class_members */ true,
       /* relaxed_keep_interfaces */ true,
       /* cfg_gathering_check_instantiable */ true,