do not re-order methods from perf-sensitive classes

Summary: Keep the original order of methods belonging to perf sensitive classes. Reviewed By: amnn Differential Revision: D27823070 fbshipit-source-id: 8f3e8c45b21bf6dca1b95217fc26518dbb0ca6e4
wsanville · May 28, 2021 · d020da4 · d020da4
1 parent 75cbbda
commit d020da4
Show file tree

Hide file tree

Showing 5 changed files with 252 additions and 3 deletions.
diff --git a/libredex/DexOutput.h b/libredex/DexOutput.h
@@ -296,6 +296,8 @@ struct CodeItemEmit {
 struct DexOutputTestHelper;
 
 class DexOutput {
+  friend class DexOutputTest;
+
  public:
   dex_stats_t m_stats;
 

diff --git a/libredex/MethodSimilarityOrderer.cpp b/libredex/MethodSimilarityOrderer.cpp
@@ -83,6 +83,11 @@ void MethodSimilarityOrderer::insert(DexMethod* method) {
   m_method_indices.emplace(method, index);
 
   auto& code_hash_ids = m_method_code_hash_ids[method];
+
+  if (type_class(method->get_class())->is_perf_sensitive()) {
+    return;
+  }
+
   auto* code = method->get_dex_code();
   if (code) {
     gather_code_hash_ids(code, code_hash_ids);
@@ -98,7 +103,14 @@ DexMethod* MethodSimilarityOrderer::get_next() {
     return nullptr;
   }
   boost::optional<size_t> best_candidate_index;
-  if (!m_last_code_hash_ids.empty()) {
+
+  // If the next method is part of a perf sensitive class,
+  // then do not look for a candidate, just preserve the
+  // original order.
+  auto* next_method = m_methods.begin()->second;
+  bool is_next_perf_sensitive = m_method_code_hash_ids[next_method].empty();
+
+  if (!is_next_perf_sensitive && !m_last_code_hash_ids.empty()) {
     // Similarity score for each candidate method, based on the number of
     // shared, missing and additional hash ids when compared to the previously
     // chosen method.
@@ -126,11 +138,12 @@ DexMethod* MethodSimilarityOrderer::get_next() {
       p.second.additional = other_code_hash_ids_size - p.second.shared;
       p.second.missing = last_code_hash_ids_size - p.second.shared;
     }
-    // Then we'll find the best matching candidate with a non-negative score.
+    // Then we'll find the best matching candidate with a non-negative score
+    // that is not perf sensitive.
     boost::optional<Score> best_candidate_score;
     for (auto& p : candidate_scores) {
       auto& score = p.second;
-      if (score.value() < 0) {
+      if (score.value() < 0 || m_method_code_hash_ids[p.first].empty()) {
         continue;
       }
       if (!best_candidate_score ||

diff --git a/test/integ/DexOutputTest.cpp b/test/integ/DexOutputTest.cpp
@@ -0,0 +1,139 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <gtest/gtest.h>
+#include <json/json.h>
+
+#include "ControlFlow.h"
+#include "DexInstruction.h"
+#include "DexPosition.h"
+#include "DexUtil.h"
+#include "IODIMetadata.h"
+#include "IRCode.h"
+#include "InstructionLowering.h"
+#include "RedexTest.h"
+#include "Show.h"
+#include "Walkers.h"
+
+#include "DexOutput.h"
+
+class DexOutputTest : public RedexIntegrationTest {
+ public:
+  DexOutputTest() {}
+
+  std::vector<DexMethod*> get_ordered_methods(DexOutput& dout) {
+    std::vector<DexMethod*> code_items;
+    for (auto& ci : dout.m_code_item_emits) {
+      code_items.push_back(ci.method);
+    }
+    return code_items;
+  }
+};
+
+TEST_F(DexOutputTest, testSimilarityOrderer) {
+  std::vector<SortMode> sort_modes{SortMode::METHOD_PROFILED_ORDER,
+                                   SortMode::METHOD_SIMILARITY};
+
+  Json::Value cfg;
+  ConfigFiles config_files(cfg);
+  std::unique_ptr<PositionMapper> pos_mapper(PositionMapper::make(""));
+  auto scope = build_class_scope(stores);
+
+  // Lower the code
+  walk::parallel::methods<>(
+      scope, [](DexMethod* m) { instruction_lowering::lower(m, true); });
+
+  DexOutput dout("", &*classes, nullptr, true, 0, 0,
+                 DebugInfoKind::NoCustomSymbolication, nullptr, config_files,
+                 pos_mapper.get(), nullptr, nullptr, nullptr, 25);
+
+  dout.prepare(SortMode::DEFAULT, sort_modes, config_files, "dex\n039");
+  auto code_items = get_ordered_methods(dout);
+
+  uint32_t i = 0;
+  std::vector<std::string> method_names(code_items.size());
+  for (auto* m : code_items) {
+    method_names[i++] = show(m);
+  }
+
+  std::vector<std::string> expected_order = {
+      "LDexOutputTest$NonPerfSensitiveClass;.<init>:(LDexOutputTest;)V",
+      "LDexOutputTest$PerfSensitiveClass;.<init>:(LDexOutputTest;)V",
+      "LDexOutputTest$SecondPerfSensitiveClass;.<init>:(LDexOutputTest;)V",
+      "LDexOutputTest$NonPerfSensitiveClass;.EjustReturnFive:()I",
+      "LDexOutputTest$PerfSensitiveClass;.EjustReturnFive:()I",
+      "LDexOutputTest$SecondPerfSensitiveClass;.EjustReturnFive:()I",
+      "LDexOutputTest;.AjustReturnFive:()I",
+      "LDexOutputTest;.EjustReturnFive:()I",
+      "LDexOutputTest$NonPerfSensitiveClass;.FsomeLogic:(I)I",
+      "LDexOutputTest$PerfSensitiveClass;.FsomeLogic:(I)I",
+      "LDexOutputTest$SecondPerfSensitiveClass;.FsomeLogic:(I)I",
+      "LDexOutputTest;.CsomeLogic:(I)I",
+      "LDexOutputTest;.FsomeLogic:(I)I",
+      "LDexOutputTest;.HsomeLogic:(I)I",
+      "LDexOutputTest;.<init>:()V",
+      "LDexOutputTest;.BjustCallSixpublic:()I",
+      "LDexOutputTest;.GjustCallSixpublic:()I",
+      "LDexOutputTest;.DgetSixpublic:()I"};
+
+  EXPECT_TRUE(std::equal(method_names.begin(), method_names.end(),
+                         expected_order.begin()));
+}
+
+TEST_F(DexOutputTest, testPerfSensitive) {
+  std::vector<SortMode> sort_modes{SortMode::METHOD_PROFILED_ORDER,
+                                   SortMode::METHOD_SIMILARITY};
+
+  Json::Value cfg;
+  ConfigFiles config_files(cfg);
+  std::unique_ptr<PositionMapper> pos_mapper(PositionMapper::make(""));
+
+  (*classes)[1]->set_perf_sensitive(true);
+  (*classes)[2]->set_perf_sensitive(true);
+
+  auto scope = build_class_scope(stores);
+
+  // Lower the code
+  walk::parallel::methods<>(
+      scope, [](DexMethod* m) { instruction_lowering::lower(m, true); });
+
+  DexOutput dout("", &*classes, nullptr, true, 0, 0,
+                 DebugInfoKind::NoCustomSymbolication, nullptr, config_files,
+                 pos_mapper.get(), nullptr, nullptr, nullptr, 25);
+
+  dout.prepare(SortMode::DEFAULT, sort_modes, config_files, "dex\n039");
+  auto code_items = get_ordered_methods(dout);
+
+  uint32_t i = 0;
+  std::vector<std::string> method_names(code_items.size());
+  for (auto* m : code_items) {
+    method_names[i++] = show(m);
+  }
+
+  std::vector<std::string> expected_order = {
+      "LDexOutputTest$NonPerfSensitiveClass;.<init>:(LDexOutputTest;)V",
+      "LDexOutputTest$NonPerfSensitiveClass;.EjustReturnFive:()I",
+      "LDexOutputTest;.AjustReturnFive:()I",
+      "LDexOutputTest;.EjustReturnFive:()I",
+      "LDexOutputTest$NonPerfSensitiveClass;.FsomeLogic:(I)I",
+      "LDexOutputTest$PerfSensitiveClass;.<init>:(LDexOutputTest;)V",
+      "LDexOutputTest$PerfSensitiveClass;.EjustReturnFive:()I",
+      "LDexOutputTest$PerfSensitiveClass;.FsomeLogic:(I)I",
+      "LDexOutputTest$SecondPerfSensitiveClass;.<init>:(LDexOutputTest;)V",
+      "LDexOutputTest$SecondPerfSensitiveClass;.EjustReturnFive:()I",
+      "LDexOutputTest$SecondPerfSensitiveClass;.FsomeLogic:(I)I",
+      "LDexOutputTest;.<init>:()V",
+      "LDexOutputTest;.BjustCallSixpublic:()I",
+      "LDexOutputTest;.GjustCallSixpublic:()I",
+      "LDexOutputTest;.CsomeLogic:(I)I",
+      "LDexOutputTest;.FsomeLogic:(I)I",
+      "LDexOutputTest;.HsomeLogic:(I)I",
+      "LDexOutputTest;.DgetSixpublic:()I"};
+
+  EXPECT_TRUE(std::equal(method_names.begin(), method_names.end(),
+                         expected_order.begin()));
+}
diff --git a/test/integ/DexOutputTest.java b/test/integ/DexOutputTest.java
@@ -0,0 +1,88 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+public class DexOutputTest {
+  public static int AjustReturnFive() {
+    return 5;
+  }
+
+  public int BjustCallSixpublic() {
+    return DgetSixpublic();
+  }
+
+  public int CsomeLogic(int y) {
+    int x = 0;
+    x = x / y;
+    x++;
+    return x;
+  }
+
+  public int DgetSixpublic() {
+    return 6;
+  }
+
+  public static int EjustReturnFive() {
+    return 5;
+  }
+
+  public int FsomeLogic(int y) {
+    int x = 0;
+    x = x / y;
+    x++;
+    return x;
+  }
+
+  public int GjustCallSixpublic() {
+    return DgetSixpublic();
+  }
+
+  public int HsomeLogic(int y) {
+    int x = 0;
+    x = x / y;
+    x++;
+    return x;
+  }
+
+  class PerfSensitiveClass {
+    public int FsomeLogic(int y) {
+      int x = 0;
+      x = x / y;
+      x++;
+      return x;
+    }
+
+    public int EjustReturnFive() {
+      return 5;
+    }
+  }
+
+  class NonPerfSensitiveClass {
+    public int FsomeLogic(int y) {
+      int x = 0;
+      x = x / y;
+      x++;
+      return x;
+    }
+
+    public int EjustReturnFive() {
+      return 5;
+    }
+  }
+
+  class SecondPerfSensitiveClass {
+    public int FsomeLogic(int y) {
+      int x = 0;
+      x = x / y;
+      x++;
+      return x;
+    }
+
+    public int EjustReturnFive() {
+      return 5;
+    }
+  }
+}
diff --git a/test/integ/Makefile.am b/test/integ/Makefile.am
@@ -21,6 +21,7 @@ check_PROGRAMS = \
     dedup_blocks_test \
     dedup_vmethods_test \
     default_annotation_test \
+    dex_output_test \
     final_inline_analysis_test \
     global_type_analysis_test \
     instruction_sequence_outliner_test \
@@ -71,6 +72,9 @@ EXTRA_dedup_vmethods_test_DEPENDENCIES = dedup_vmethods_test-class.dex
 default_annotation_test_SOURCES = DefaultAnnotation.cpp
 EXTRA_default_annotation_test_DEPENDENCIES = default_annotation_test-class.dex
 
+dex_output_test_SOURCES = DexOutputTest.cpp
+EXTRA_dex_output_test_DEPENDENCIES = dex_output_test-class.dex
+
 final_inline_analysis_test_SOURCES = FinalInlineAnalysisTest.cpp
 EXTRA_final_inline_analysis_test_DEPENDENCIES = final_inline_analysis_test-class.dex
 
@@ -175,6 +179,9 @@ dedup_vmethods_test-class.jar: DedupVirtualMethodsTest.java
 default_annotation_test-class.jar: DefaultAnnotationTest.java
 	$(create_jar)
 
+dex_output_test-class.jar: DexOutputTest.java
+	$(create_jar)
+
 final_inline_analysis_test-class.jar: FinalInlineAnalysisTest.java
 	$(create_jar)