improve perf check for unfinalize init inline

Summary:
Be more conservative that avoid inlining constructor that need write barrier into method that are perf sensitive.
Also expand this check to not only method that need to add write barrier, but also method that were added write barrier in previous or current inliner run.

Reviewed By: wsanville

Differential Revision: D71438750

fbshipit-source-id: 9b7b50ff998c93285a6ac9586818f4788a77464e

Author: ssj933

libredex/SourceBlocks.h

@@ -341,6 +341,24 @@ inline bool is_hot(cfg::Block* b) {
   return sb->foreach_val_early([](const auto& v) { return v && v->val > 0; });
 }
 
+// If a method's entry block is hot, consider this method is hot.
+inline bool method_is_hot(DexMethod* method) {
+  auto& cfg = method->get_code()->cfg();
+  return is_hot(cfg.entry_block());
+}
+
+// If a method's entry block may be hot, consider this method may be hot.
+inline bool method_maybe_hot(DexMethod* method) {
+  auto& cfg = method->get_code()->cfg();
+  return maybe_hot(cfg.entry_block());
+}
+
+// If a method's entry block is not cold, consider this method is not cold.
+inline bool method_is_not_cold(DexMethod* method) {
+  auto& cfg = method->get_code()->cfg();
+  return is_not_cold(cfg.entry_block());
+}
+
 template <typename Iterator>
 inline SourceBlock* find_between(const Iterator& start, const Iterator& end) {
   auto it = std::find_if(

opt/write_barrier/WriteBarrierLoweringPass.cpp

@@ -10,10 +10,10 @@
 #include "CFGMutation.h"
 #include "ControlFlow.h"
 #include "DexAsm.h"
+#include "DexStructure.h"
 #include "Show.h"
 #include "SourceBlocks.h"
 #include "Walkers.h"
-#include <DexStructure.h>
 
 namespace {
 using namespace dex_asm;

service/method-inliner/Inliner.cpp

@@ -134,7 +134,8 @@ MultiMethodInliner::MultiMethodInliner(
     bool local_only,
     bool consider_hot_cold,
     InlinerCostConfig inliner_cost_config,
-    const std::unordered_set<const DexMethod*>* unfinalized_init_methods)
+    const std::unordered_set<const DexMethod*>* unfinalized_init_methods,
+    InsertOnlyConcurrentSet<DexMethod*>* methods_with_write_barrier)
     : m_concurrent_resolver(std::move(concurrent_resolve_fn)),
       m_scheduler(
           [this](DexMethod* method) {
@@ -173,7 +174,8 @@ MultiMethodInliner::MultiMethodInliner(
       m_local_only(local_only),
       m_consider_hot_cold(consider_hot_cold),
       m_inliner_cost_config(inliner_cost_config),
-      m_unfinalized_init_methods(unfinalized_init_methods) {
+      m_unfinalized_init_methods(unfinalized_init_methods),
+      m_methods_with_write_barrier(methods_with_write_barrier) {
   Timer t("MultiMethodInliner construction");
   for (const auto& callee_callers : true_virtual_callers) {
     auto callee = callee_callers.first;
@@ -699,7 +701,8 @@ size_t MultiMethodInliner::inline_inlinables(
     cfg_next_caller_reg = caller->cfg().get_registers_size();
   }
   size_t calls_not_inlinable{0}, calls_not_inlined{0}, no_returns{0},
-      unreachable_insns{0}, caller_too_large{0};
+      unreachable_insns{0}, caller_too_large{0},
+      calls_not_inlined_with_perf_sensitive_constructors{0};
 
   size_t intermediate_shrinkings{0};
   size_t intermediate_remove_unreachable_blocks{0};
@@ -869,6 +872,23 @@ size_t MultiMethodInliner::inline_inlinables(
     auto needs_init_class = get_needs_init_class(callee_method);
     auto needs_constructor_fence =
         get_needs_constructor_fence(caller_method, callee_method);
+    auto callee_has_constructor_fence =
+        m_config.unfinalize_relaxed_init_inline &&
+        m_methods_with_write_barrier &&
+        m_methods_with_write_barrier->count(callee_method) > 0;
+    if (needs_constructor_fence || callee_has_constructor_fence) {
+      if ((m_config.unfinalize_perf_mode ==
+               inliner::UnfinalizePerfMode::NOT_COLD &&
+           source_blocks::method_is_not_cold(caller_method)) ||
+          (m_config.unfinalize_perf_mode ==
+               inliner::UnfinalizePerfMode::MAYBE_HOT &&
+           source_blocks::method_maybe_hot(caller_method)) ||
+          (m_config.unfinalize_perf_mode == inliner::UnfinalizePerfMode::HOT &&
+           source_blocks::method_is_hot(caller_method))) {
+        calls_not_inlined_with_perf_sensitive_constructors++;
+        continue;
+      }
+    }
     bool success = inliner::inline_with_cfg(
         caller_method, callee_method, callsite_insn,
         inlinable.needs_receiver_cast, needs_init_class, *cfg_next_caller_reg,
@@ -899,16 +919,23 @@ size_t MultiMethodInliner::inline_inlinables(
         m_inlined_with_fence.insert(m);
       }
       constructor_fences++;
-    } else if (get_needs_constructor_fence(/* caller */ nullptr,
-                                           callee_method)) {
-      // Inlining callee in any other context would need a constructor fence;
-      // that means that final fields are involved, and we just didn't need a
-      // constructor fence here because we inlined into an init overload. Record
-      // this.
-      always_assert(method::is_init(caller_method));
-      always_assert(method::is_init(callee_method));
-      always_assert(caller_method->get_class() == callee_method->get_class());
-      m_unfinalized_overloads.emplace(caller_method, callee_method);
+      if (m_methods_with_write_barrier) {
+        m_methods_with_write_barrier->insert(caller_method);
+      }
+    } else {
+      if (callee_has_constructor_fence) {
+        m_methods_with_write_barrier->insert(caller_method);
+      }
+      if (get_needs_constructor_fence(/* caller */ nullptr, callee_method)) {
+        // Inlining callee in any other context would need a constructor fence;
+        // that means that final fields are involved, and we just didn't need a
+        // constructor fence here because we inlined into an init overload.
+        // Record this.
+        always_assert(method::is_init(caller_method));
+        always_assert(method::is_init(callee_method));
+        always_assert(caller_method->get_class() == callee_method->get_class());
+        m_unfinalized_overloads.emplace(caller_method, callee_method);
+      }
     }
 
     inlined_callees.push_back(callee_method);
@@ -944,6 +971,10 @@ size_t MultiMethodInliner::inline_inlinables(
   if (calls_not_inlined) {
     info.calls_not_inlined += calls_not_inlined;
   }
+  if (calls_not_inlined_with_perf_sensitive_constructors) {
+    info.calls_not_inlined_with_perf_sensitive_constructors +=
+        calls_not_inlined_with_perf_sensitive_constructors;
+  }
   if (no_returns) {
     info.no_returns += no_returns;
   }

service/method-inliner/Inliner.h

@@ -274,6 +274,8 @@ class MultiMethodInliner {
       bool consider_hot_cold = false,
       InlinerCostConfig inliner_cost_config = DEFAULT_COST_CONFIG,
       const std::unordered_set<const DexMethod*>* unfinalized_init_methods =
+          nullptr,
+      InsertOnlyConcurrentSet<DexMethod*>* methods_with_write_barrier =
           nullptr);
 
   /*
@@ -781,6 +783,7 @@ class MultiMethodInliner {
     std::atomic<size_t> constructor_fences{0};
     std::atomic<size_t> calls_not_inlinable{0};
     std::atomic<size_t> calls_not_inlined{0};
+    std::atomic<size_t> calls_not_inlined_with_perf_sensitive_constructors{0};
     std::atomic<size_t> no_returns{0};
     std::atomic<size_t> unreachable_insns{0};
     std::atomic<size_t> intermediate_shrinkings{0};
@@ -848,6 +851,8 @@ class MultiMethodInliner {
   InsertOnlyConcurrentMap<const DexMethod*, const DexMethod*>
       m_unfinalized_overloads;
 
+  InsertOnlyConcurrentSet<DexMethod*>* m_methods_with_write_barrier;
+
  public:
   const InliningInfo& get_info() { return info; }
 

service/method-inliner/MethodInliner.cpp

@@ -720,24 +720,6 @@ void gather_true_virtual_methods(
   }
 }
 
-// If a method's entry block is hot, consider this method is hot.
-bool is_hot(DexMethod* method) {
-  auto& cfg = method->get_code()->cfg();
-  return source_blocks::is_hot(cfg.entry_block());
-}
-
-// If a method's entry block may be hot, consider this method may be hot.
-bool maybe_hot(DexMethod* method) {
-  auto& cfg = method->get_code()->cfg();
-  return source_blocks::maybe_hot(cfg.entry_block());
-}
-
-// If a method's entry block is not cold, consider this method is not cold.
-bool is_not_cold(DexMethod* method) {
-  auto& cfg = method->get_code()->cfg();
-  return source_blocks::is_not_cold(cfg.entry_block());
-}
-
 // If a constructor cannot be inlined because it writes to final fields, then we
 // can "simply" remove the final modifier. We typically run the
 // AccessMarkingMarking pass multiple times, including after the final inliner,
@@ -752,8 +734,25 @@ void unfinalize_fields_if_beneficial_for_relaxed_init_inlining(
     const Scope& scope,
     const inliner::UnfinalizePerfMode& unfinalize_perf_mode,
     InsertOnlyConcurrentMap<DexField*, std::vector<DexMethod*>>*
-        unfinalized_fields) {
+        unfinalized_fields,
+    InsertOnlyConcurrentSet<DexMethod*>* methods_with_write_barrier) {
   walk::parallel::classes(scope, [&](DexClass* cls) {
+    // Since we run inliner several times, in previous inliner run we might
+    // unfinalized some fields and added write barrier in method already.
+    // We need to take these method into consideration when we check
+    // inlining eligibility.
+    for (auto method : cls->get_all_methods()) {
+      if (method->get_code() == nullptr) {
+        continue;
+      }
+      for (const auto& mie : InstructionIterable(method->get_code()->cfg())) {
+        if (opcode::is_write_barrier(mie.insn->opcode())) {
+          methods_with_write_barrier->insert(method);
+          break;
+        }
+      }
+    }
+
     // We also exclude possibly anonymous classes as those may
     // regress the effectiveness of the class-merging passes.
     // TODO T184662680: While this is not a correctness issue,
@@ -777,11 +776,11 @@ void unfinalize_fields_if_beneficial_for_relaxed_init_inlining(
     bool perf_sensitive = false;
     for (auto* init_method : cls->get_ctors()) {
       if ((unfinalize_perf_mode == inliner::UnfinalizePerfMode::NOT_COLD &&
-           is_not_cold(init_method)) ||
+           source_blocks::method_is_not_cold(init_method)) ||
           (unfinalize_perf_mode == inliner::UnfinalizePerfMode::MAYBE_HOT &&
-           maybe_hot(init_method)) ||
+           source_blocks::method_maybe_hot(init_method)) ||
           (unfinalize_perf_mode == inliner::UnfinalizePerfMode::HOT &&
-           is_hot(init_method))) {
+           source_blocks::method_is_hot(init_method))) {
         perf_sensitive = true;
         break;
       }
@@ -906,11 +905,13 @@ void run_inliner(
 
   InsertOnlyConcurrentMap<DexField*, std::vector<DexMethod*>>
       unfinalized_fields;
+  InsertOnlyConcurrentSet<DexMethod*> methods_with_write_barrier;
   std::unordered_set<const DexMethod*> unfinalized_init_methods;
   if (inliner_config.relaxed_init_inline &&
       inliner_config.unfinalize_relaxed_init_inline && min_sdk >= 21) {
     unfinalize_fields_if_beneficial_for_relaxed_init_inlining(
-        scope, inliner_config.unfinalize_perf_mode, &unfinalized_fields);
+        scope, inliner_config.unfinalize_perf_mode, &unfinalized_fields,
+        &methods_with_write_barrier);
     mgr.set_metric("unfinalized_fields", unfinalized_fields.size());
     TRACE(INLINE, 3, "unfinalized %zu fields", unfinalized_fields.size());
     for (auto&& [_, init_methods] : unfinalized_fields) {
@@ -982,7 +983,8 @@ void run_inliner(
       analyze_and_prune_inits, conf.get_pure_methods(), min_sdk_api,
       cross_dex_penalty,
       /* configured_finalish_field_names */ {}, local_only, consider_hot_cold,
-      inliner_cost_config, &unfinalized_init_methods);
+      inliner_cost_config, &unfinalized_init_methods,
+      &methods_with_write_barrier);
   inliner.inline_methods();
 
   // refinalize where possible
@@ -1140,6 +1142,9 @@ void run_inliner(
   mgr.incr_metric("intermediate_remove_unreachable_blocks",
                   inliner.get_info().intermediate_remove_unreachable_blocks);
   mgr.incr_metric("calls_not_inlined", inliner.get_info().calls_not_inlined);
+  mgr.incr_metric(
+      "calls_not_inlined_with_perf_sensitive_constructors",
+      inliner.get_info().calls_not_inlined_with_perf_sensitive_constructors);
   mgr.incr_metric("methods_removed", deleted.size());
   mgr.incr_metric("escaped_virtual", inliner.get_info().escaped_virtual);
   mgr.incr_metric("unresolved_methods", inliner.get_info().unresolved_methods);

test/unit/MethodInlineTest.cpp

@@ -2675,6 +2675,7 @@ TEST_F(MethodInlineTest, inline_init_unfinalized_relaxed) {
   inliner_config.shrinker.run_local_dce = true;
   inliner_config.shrinker.compute_pure_methods = false;
   inliner_config.relaxed_init_inline = true;
+  inliner_config.unfinalize_perf_mode = inliner::UnfinalizePerfMode::NONE;
 
   caller->get_code()->build_cfg();
   init->get_code()->build_cfg();