Use verify when speed-profile gets an empty profile
Change the compiler filter to verify if we need to compile
speed-profile but we don't get a profile, or the profile is empty.
This will improve the clarity and the precision of the telemetry
data which usually expects speed-profile to outperform verify.
Test: gtest
Bug: 188655918
Change-Id: I215552e0001d56df0e0d676721f0a741ef2573be
(cherry picked from commit 028c7efaf7321a1e253fb4d9dcc5d85e8a9e6d68)
diff --git a/dex2oat/dex2oat.cc b/dex2oat/dex2oat.cc
index fa1a10e..75a3e0d 100644
--- a/dex2oat/dex2oat.cc
+++ b/dex2oat/dex2oat.cc
@@ -544,7 +544,8 @@
force_determinism_(false),
check_linkage_conditions_(false),
crash_on_linkage_violation_(false),
- compile_individually_(false)
+ compile_individually_(false),
+ profile_load_attempted_(false)
{}
~Dex2Oat() {
@@ -1160,7 +1161,7 @@
// before reading compiler options.
static_assert(CompilerFilter::kDefaultCompilerFilter == CompilerFilter::kSpeed);
DCHECK_EQ(compiler_options_->GetCompilerFilter(), CompilerFilter::kSpeed);
- if (UseProfile()) {
+ if (HasProfileInput()) {
compiler_options_->SetCompilerFilter(CompilerFilter::kSpeedProfile);
}
@@ -1169,9 +1170,6 @@
}
ProcessOptions(parser_options.get());
-
- // Insert some compiler things.
- InsertCompileOptions(argc, argv);
}
// Check whether the oat output files are writable, and open them for later. Also open a swap
@@ -1395,7 +1393,7 @@
if (!IsImage()) {
return;
}
- if (profile_compilation_info_ != nullptr) {
+ if (DoProfileGuidedOptimizations()) {
// TODO: The following comment looks outdated or misplaced.
// Filter out class path classes since we don't want to include these in the image.
HashSet<std::string> image_classes = profile_compilation_info_->GetClassDescriptors(
@@ -2327,12 +2325,16 @@
return is_host_;
}
- bool UseProfile() const {
+ bool HasProfileInput() const {
return profile_file_fd_ != -1 || !profile_file_.empty();
}
+ // Must be called after the profile is loaded.
bool DoProfileGuidedOptimizations() const {
- return UseProfile();
+ DCHECK(!HasProfileInput() || profile_load_attempted_)
+ << "The profile has to be loaded before we can decided "
+ << "if we do profile guided optimizations";
+ return profile_compilation_info_ != nullptr && !profile_compilation_info_->IsEmpty();
}
bool DoGenerateCompactDex() const {
@@ -2358,7 +2360,8 @@
}
bool LoadProfile() {
- DCHECK(UseProfile());
+ DCHECK(HasProfileInput());
+ profile_load_attempted_ = true;
// TODO(calin): We should be using the runtime arena pool (instead of the
// default profile arena). However the setup logic is messy and needs
// cleaning up before that (e.g. the oat writers are created before the
@@ -2391,6 +2394,29 @@
return true;
}
+ // If we're asked to speed-profile the app but we have no profile, or the profile
+ // is empty, change the filter to verify, and the image_type to none.
+ // A speed-profile compilation without profile data is equivalent to verify and
+ // this change will increase the precision of the telemetry data.
+ void UpdateCompilerOptionsBasedOnProfile() {
+ if (!DoProfileGuidedOptimizations() &&
+ compiler_options_->GetCompilerFilter() == CompilerFilter::kSpeedProfile) {
+ VLOG(compiler) << "Changing compiler filter to verify from speed-profile "
+ << "because of empty or non existing profile";
+
+ compiler_options_->SetCompilerFilter(CompilerFilter::kVerify);
+
+ // Note that we could reset the image_type to CompilerOptions::ImageType::kNone
+ // to prevent an app image generation.
+ // However, if we were pass an image file we would essentially leave the image
+ // file empty (possibly triggering some harmless errors when we try to load it).
+ //
+ // Letting the image_type_ be determined by whether or not we passed an image
+ // file will at least write the appropriate header making it an empty but valid
+ // image.
+ }
+ }
+
class ScopedDex2oatReporting {
public:
explicit ScopedDex2oatReporting(const Dex2Oat& dex2oat) {
@@ -2601,9 +2627,6 @@
elf_writers_.emplace_back(linker::CreateElfWriterQuick(*compiler_options_, oat_file.get()));
elf_writers_.back()->Start();
bool do_oat_writer_layout = DoDexLayoutOptimizations() || DoOatLayoutOptimizations();
- if (profile_compilation_info_ != nullptr && profile_compilation_info_->IsEmpty()) {
- do_oat_writer_layout = false;
- }
oat_writers_.emplace_back(new linker::OatWriter(
*compiler_options_,
timings_,
@@ -2971,6 +2994,9 @@
// argument.
std::string apex_versions_argument_;
+ // Whether or we attempted to load the profile (if given).
+ bool profile_load_attempted_;
+
DISALLOW_IMPLICIT_CONSTRUCTORS(Dex2Oat);
};
@@ -3073,13 +3099,22 @@
// If needed, process profile information for profile guided compilation.
// This operation involves I/O.
- if (dex2oat->UseProfile()) {
+ if (dex2oat->HasProfileInput()) {
if (!dex2oat->LoadProfile()) {
LOG(ERROR) << "Failed to process profile file";
return dex2oat::ReturnCode::kOther;
}
}
+ // Check if we need to update any of the compiler options (such as the filter)
+ // and do it before anything else (so that the other operations have a true
+ // view of the state).
+ dex2oat->UpdateCompilerOptionsBasedOnProfile();
+
+ // Insert the compiler options in the key value store.
+ // We have to do this after we altered any incoming arguments
+ // (such as the compiler filter).
+ dex2oat->InsertCompileOptions(argc, argv);
// Check early that the result of compilation can be written
if (!dex2oat->OpenFile()) {
@@ -3125,7 +3160,7 @@
// Note: If dex2oat fails, installd will remove the oat files causing the app
// to fallback to apk with possible in-memory extraction. We want to avoid
// that, and thus we're lenient towards profile corruptions.
- if (dex2oat->UseProfile()) {
+ if (dex2oat->DoProfileGuidedOptimizations()) {
dex2oat->VerifyProfileData();
}
diff --git a/dex2oat/dex2oat_test.cc b/dex2oat/dex2oat_test.cc
index c4c39c2..cc29b0f 100644
--- a/dex2oat/dex2oat_test.cc
+++ b/dex2oat/dex2oat_test.cc
@@ -208,6 +208,20 @@
std::string error_msg_ = "";
};
+// This test class provides an easy way to validate an expected filter which is different
+// then the one pass to generate the odex file (compared to adding yet another argument
+// to what's already huge test methods).
+class Dex2oatWithExpectedFilterTest : public Dex2oatTest {
+ protected:
+ void CheckFilter(
+ CompilerFilter::Filter expected ATTRIBUTE_UNUSED,
+ CompilerFilter::Filter actual) override {
+ EXPECT_EQ(expected_filter_, actual);
+ }
+
+ CompilerFilter::Filter expected_filter_;
+};
+
class Dex2oatSwapTest : public Dex2oatTest {
protected:
void RunTest(bool use_fd, bool expect_use, const std::vector<std::string>& extra_args = {}) {
@@ -443,6 +457,14 @@
bool expect_large,
bool expect_downgrade,
const std::vector<std::string>& extra_args = {}) {
+ RunTest(filter, filter, expect_large, expect_downgrade, extra_args);
+ }
+
+ void RunTest(CompilerFilter::Filter filter,
+ CompilerFilter::Filter expected_filter,
+ bool expect_large,
+ bool expect_downgrade,
+ const std::vector<std::string>& extra_args = {}) {
std::string dex_location = GetScratchDir() + "/DexNoOat.jar";
std::string odex_location = GetOdexDir() + "/DexOdexNoOat.odex";
std::string app_image_file = GetScratchDir() + "/Test.art";
@@ -458,6 +480,7 @@
odex_location,
app_image_file,
filter,
+ expected_filter,
expect_large,
expect_downgrade);
}
@@ -466,6 +489,7 @@
const std::string& odex_location,
const std::string& app_image_file,
CompilerFilter::Filter filter,
+ CompilerFilter::Filter expected_filter,
bool expect_large,
bool expect_downgrade) {
if (expect_downgrade) {
@@ -503,7 +527,7 @@
// If the input filter was "below," it should have been used.
if (!CompilerFilter::IsAsGoodAs(CompilerFilter::kExtract, filter)) {
- EXPECT_EQ(odex_file->GetCompilerFilter(), filter);
+ EXPECT_EQ(odex_file->GetCompilerFilter(), expected_filter);
}
// If expect large, make sure the app image isn't generated or is empty.
@@ -511,7 +535,7 @@
EXPECT_EQ(file->GetLength(), 0u);
}
} else {
- EXPECT_EQ(odex_file->GetCompilerFilter(), filter);
+ EXPECT_EQ(odex_file->GetCompilerFilter(), expected_filter);
ASSERT_TRUE(file != nullptr) << app_image_file;
EXPECT_GT(file->GetLength(), 0u);
}
@@ -576,7 +600,7 @@
// Regressin test for b/35665292.
TEST_F(Dex2oatVeryLargeTest, SpeedProfileNoProfile) {
// Test that dex2oat doesn't crash with speed-profile but no input profile.
- RunTest(CompilerFilter::kSpeedProfile, false, false);
+ RunTest(CompilerFilter::kSpeedProfile, CompilerFilter::kVerify, false, false);
}
class Dex2oatLayoutTest : public Dex2oatTest {
@@ -675,6 +699,7 @@
// Don't check the result since CheckResult relies on the class being in the profile.
image_file_empty_profile = GetImageObjectSectionSize(app_image_file);
EXPECT_GT(image_file_empty_profile, 0u);
+ CheckCompilerFilter(dex_location, odex_location, CompilerFilter::Filter::kVerify);
}
// Small profile.
@@ -685,6 +710,7 @@
/*num_profile_classes=*/ 1);
CheckValidity();
CheckResult(dex_location, odex_location, app_image_file);
+ CheckCompilerFilter(dex_location, odex_location, CompilerFilter::Filter::kSpeedProfile);
if (app_image) {
// Test that the profile made a difference by adding more classes.
@@ -693,6 +719,21 @@
}
}
+ void CheckCompilerFilter(
+ const std::string& dex_location,
+ const std::string& odex_location,
+ CompilerFilter::Filter expected_filter) {
+ std::string error_msg;
+ std::unique_ptr<OatFile> odex_file(OatFile::Open(/*zip_fd=*/ -1,
+ odex_location.c_str(),
+ odex_location.c_str(),
+ /*executable=*/ false,
+ /*low_4gb=*/ false,
+ dex_location,
+ &error_msg));
+ EXPECT_EQ(odex_file->GetCompilerFilter(), expected_filter);
+ }
+
void RunTestVDex() {
std::string dex_location = GetScratchDir() + "/DexNoOat.jar";
std::string odex_location = GetOdexDir() + "/DexOdexNoOat.odex";
@@ -1841,7 +1882,10 @@
ASSERT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) != 0) << status << " " << output_;
}
-TEST_F(Dex2oatTest, AppImageNoProfile) {
+TEST_F(Dex2oatWithExpectedFilterTest, AppImageNoProfile) {
+ // Set the expected filter.
+ expected_filter_ = CompilerFilter::Filter::kVerify;
+
ScratchFile app_image_file;
const std::string out_dir = GetScratchDir();
const std::string odex_location = out_dir + "/base.odex";
@@ -1890,7 +1934,10 @@
/*use_zip_fd=*/ true));
}
-TEST_F(Dex2oatTest, AppImageEmptyDex) {
+TEST_F(Dex2oatWithExpectedFilterTest, AppImageEmptyDex) {
+ // Set the expected filter.
+ expected_filter_ = CompilerFilter::Filter::kVerify;
+
// Create a profile with the startup method marked.
ScratchFile profile_file;
ScratchFile temp_dex;
diff --git a/libprofile/profile/profile_compilation_info.cc b/libprofile/profile/profile_compilation_info.cc
index f32c122..5cbdced 100644
--- a/libprofile/profile/profile_compilation_info.cc
+++ b/libprofile/profile/profile_compilation_info.cc
@@ -2214,7 +2214,10 @@
bool ProfileCompilationInfo::IsEmpty() const {
DCHECK_EQ(info_.size(), profile_key_map_.size());
- return info_.empty();
+ // Note that this doesn't look at the bitmap region, so we will return true
+ // when the profile contains only non-hot methods. This is generally ok
+ // as for speed-profile to be useful we do need hot methods and resolved classes.
+ return GetNumberOfMethods() == 0 && GetNumberOfResolvedClasses() == 0;
}
ProfileCompilationInfo::InlineCacheMap*
diff --git a/runtime/dexopt_test.cc b/runtime/dexopt_test.cc
index cfd7f91..fc5a0086 100644
--- a/runtime/dexopt_test.cc
+++ b/runtime/dexopt_test.cc
@@ -26,11 +26,14 @@
#include "base/mem_map.h"
#include "common_runtime_test.h"
#include "compiler_callbacks.h"
+#include "dex/art_dex_file_loader.h"
+#include "dex/dex_file_loader.h"
#include "dex2oat_environment_test.h"
#include "dexopt_test.h"
#include "gc/space/image_space.h"
#include "hidden_api.h"
#include "oat.h"
+#include "profile/profile_compilation_info.h"
namespace art {
void DexoptTest::SetUp() {
@@ -110,6 +113,24 @@
ScratchFile profile_file;
if (CompilerFilter::DependsOnProfile(filter)) {
+ // Create a profile with some basic content so that dex2oat
+ // doesn't get an empty profile and changes the filter to verify.
+ std::string error_msg;
+ std::vector<std::unique_ptr<const DexFile>> dex_files;
+ const ArtDexFileLoader dex_file_loader;
+ ASSERT_TRUE(dex_file_loader.Open(
+ dex_location.c_str(), dex_location.c_str(), /*verify=*/ false, /*verify_checksum=*/ false,
+ &error_msg, &dex_files));
+ EXPECT_GE(dex_files.size(), 1U);
+ std::unique_ptr<const DexFile>& dex_file = dex_files[0];
+ ProfileCompilationInfo info;
+
+ info.AddClass(*dex_file, dex::TypeIndex(0));
+
+ ASSERT_TRUE(info.Save(profile_file.GetFd()));
+ ASSERT_EQ(0, profile_file.GetFile()->Flush());
+
+ // Set the argument
args.push_back("--profile-file=" + profile_file.GetFilename());
}