profman: Return whether the profile matched instead of updated.
This change fixes the bug that ART Services doesn't accept a profile in
a DM file. Before this change, if the reference profile doesn't exist
but a DM file exists, when compiling the app, ART services will
unexpected fall back to "verify".
When ART Services uses a profile in a DM file, it runs profman with
`--copy-and-update-profile-key` on that file and checks the return code.
The intention is to check whether the profile is usable for the APK.
Therefore, the return code should indicate whether the profile matches
the APK, rather than whether the profile has been updated or not. An
update only happens if the dex checksum matches while the dex location
doesn't match. If both the dex checksum and the dex location match,
there will be no update, but profman should still return a code
indicating that this is a happy case.
Bug: 248609414
Test: atest ArtGtestsTargetChroot:ProfileCompilationInfoTest
Test: manual -
1. Install YouTube with a DM file.
2. Delete odex file and reference profile.
3. adb shell pm art optimize-package -m speed-profile \
com.google.android.youtube
4. See the compilation successful with the profile in the DM file.
Ignore-AOSP-First: Contains internal-only change. Will cherry-pick the
AOSP part later.
Change-Id: I18e41bc962083e78e441fd18ab659037ad61badd
diff --git a/artd/artd.cc b/artd/artd.cc
index 0db2efa..8e55194 100644
--- a/artd/artd.cc
+++ b/artd/artd.cc
@@ -489,7 +489,7 @@
return NonFatal("Failed to run profman: " + result.error().message());
}
- if (result.value() == ProfmanResult::kCopyAndUpdateNoUpdate) {
+ if (result.value() == ProfmanResult::kCopyAndUpdateNoMatch) {
*_aidl_return = false;
return ScopedAStatus::ok();
}
diff --git a/artd/artd_test.cc b/artd/artd_test.cc
index 3d2fbf3..f39e2cd 100644
--- a/artd/artd_test.cc
+++ b/artd/artd_test.cc
@@ -1045,7 +1045,7 @@
CreateFile(dex_file_);
EXPECT_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _))
- .WillOnce(Return(ProfmanResult::kCopyAndUpdateNoUpdate));
+ .WillOnce(Return(ProfmanResult::kCopyAndUpdateNoMatch));
bool result;
EXPECT_TRUE(artd_->copyAndRewriteProfile(src, &dst, dex_file_, &result).isOk());
diff --git a/libprofile/profile/profile_compilation_info.cc b/libprofile/profile/profile_compilation_info.cc
index d6eb360..3891b10 100644
--- a/libprofile/profile/profile_compilation_info.cc
+++ b/libprofile/profile/profile_compilation_info.cc
@@ -2388,8 +2388,8 @@
}
bool ProfileCompilationInfo::UpdateProfileKeys(
- const std::vector<std::unique_ptr<const DexFile>>& dex_files, /*out*/ bool* updated) {
- *updated = false;
+ const std::vector<std::unique_ptr<const DexFile>>& dex_files, /*out*/ bool* matched) {
+ *matched = false;
for (const std::unique_ptr<const DexFile>& dex_file : dex_files) {
for (const std::unique_ptr<DexFileData>& dex_data : info_) {
if (dex_data->checksum == dex_file->GetLocationChecksum() &&
@@ -2409,8 +2409,8 @@
// form the old key.
dex_data->profile_key = MigrateAnnotationInfo(new_profile_key, dex_data->profile_key);
profile_key_map_.Put(dex_data->profile_key, dex_data->profile_index);
- *updated = true;
}
+ *matched = true;
}
}
}
diff --git a/libprofile/profile/profile_compilation_info.h b/libprofile/profile/profile_compilation_info.h
index 76cbf9a..27902ad 100644
--- a/libprofile/profile/profile_compilation_info.h
+++ b/libprofile/profile/profile_compilation_info.h
@@ -646,9 +646,9 @@
// If the new profile key would collide with an existing key (for a different dex)
// the method returns false. Otherwise it returns true.
//
- // `updated` is set to true if any profile key has been updated by this method.
+ // `matched` is set to true if any profile has matched any input dex file.
bool UpdateProfileKeys(const std::vector<std::unique_ptr<const DexFile>>& dex_files,
- /*out*/ bool* updated);
+ /*out*/ bool* matched);
// Checks if the profile is empty.
bool IsEmpty() const;
diff --git a/libprofile/profile/profile_compilation_info_test.cc b/libprofile/profile/profile_compilation_info_test.cc
index 2ee34f2..6d10467 100644
--- a/libprofile/profile/profile_compilation_info_test.cc
+++ b/libprofile/profile/profile_compilation_info_test.cc
@@ -956,9 +956,9 @@
AddMethod(&info, dex2, /*method_idx=*/ 0);
// Update the profile keys based on the original dex files
- bool updated = false;
- ASSERT_TRUE(info.UpdateProfileKeys(dex_files, &updated));
- ASSERT_TRUE(updated);
+ bool matched = false;
+ ASSERT_TRUE(info.UpdateProfileKeys(dex_files, &matched));
+ ASSERT_TRUE(matched);
// Verify that we find the methods when searched with the original dex files.
for (const std::unique_ptr<const DexFile>& dex : dex_files) {
@@ -984,9 +984,9 @@
AddMethod(&info, dex2, /*method_idx=*/ 0, Hotness::kFlagHot, annotation);
// Update the profile keys based on the original dex files
- bool updated = false;
- ASSERT_TRUE(info.UpdateProfileKeys(dex_files, &updated));
- ASSERT_TRUE(updated);
+ bool matched = false;
+ ASSERT_TRUE(info.UpdateProfileKeys(dex_files, &matched));
+ ASSERT_TRUE(matched);
// Verify that we find the methods when searched with the original dex files.
for (const std::unique_ptr<const DexFile>& dex : dex_files) {
@@ -1001,7 +1001,33 @@
}
}
-TEST_F(ProfileCompilationInfoTest, UpdateProfileKeyOkButNoUpdate) {
+TEST_F(ProfileCompilationInfoTest, UpdateProfileKeyOkMatchedButNoUpdate) {
+ std::vector<std::unique_ptr<const DexFile>> dex_files;
+ dex_files.push_back(std::unique_ptr<const DexFile>(dex1));
+
+ // Both the checksum and the location match the original dex file.
+ ProfileCompilationInfo info;
+ AddMethod(&info, dex1, /*method_idx=*/0);
+
+ // No update should happen, but this should be considered as a happy case.
+ bool matched = false;
+ ASSERT_TRUE(info.UpdateProfileKeys(dex_files, &matched));
+ ASSERT_TRUE(matched);
+
+ // Verify that we find the methods when searched with the original dex files.
+ for (const std::unique_ptr<const DexFile>& dex : dex_files) {
+ ProfileCompilationInfo::MethodHotness loaded_hotness =
+ GetMethod(info, dex.get(), /*method_idx=*/ 0);
+ ASSERT_TRUE(loaded_hotness.IsHot());
+ }
+
+ // Release the ownership as this is held by the test class;
+ for (std::unique_ptr<const DexFile>& dex : dex_files) {
+ UNUSED(dex.release());
+ }
+}
+
+TEST_F(ProfileCompilationInfoTest, UpdateProfileKeyOkButNoMatch) {
std::vector<std::unique_ptr<const DexFile>> dex_files;
dex_files.push_back(std::unique_ptr<const DexFile>(dex1));
@@ -1009,9 +1035,9 @@
AddMethod(&info, dex2, /*method_idx=*/ 0);
// Update the profile keys based on the original dex files.
- bool updated = false;
- ASSERT_TRUE(info.UpdateProfileKeys(dex_files, &updated));
- ASSERT_FALSE(updated);
+ bool matched = false;
+ ASSERT_TRUE(info.UpdateProfileKeys(dex_files, &matched));
+ ASSERT_FALSE(matched);
// Verify that we did not perform any update and that we cannot find anything with the new
// location.
@@ -1043,9 +1069,9 @@
// This will cause the rename to fail because an existing entry would already have that name.
AddMethod(&info, dex1_renamed, /*method_idx=*/ 0);
- bool updated = false;
- ASSERT_FALSE(info.UpdateProfileKeys(dex_files, &updated));
- ASSERT_FALSE(updated);
+ bool matched = false;
+ ASSERT_FALSE(info.UpdateProfileKeys(dex_files, &matched));
+ ASSERT_FALSE(matched);
// Release the ownership as this is held by the test class;
for (std::unique_ptr<const DexFile>& dex : dex_files) {
diff --git a/profman/include/profman/profman_result.h b/profman/include/profman/profman_result.h
index 4d2b733..9c9aca9 100644
--- a/profman/include/profman/profman_result.h
+++ b/profman/include/profman/profman_result.h
@@ -57,7 +57,7 @@
// The return codes of running profman with `--copy-and-update-profile-key`.
enum CopyAndUpdateResult {
kCopyAndUpdateSuccess = 0,
- kCopyAndUpdateNoUpdate = 21,
+ kCopyAndUpdateNoMatch = 21,
kCopyAndUpdateErrorFailedToUpdateProfile = 22,
kCopyAndUpdateErrorFailedToSaveProfile = 23,
kCopyAndUpdateErrorFailedToLoadProfile = 24,
diff --git a/profman/profile_assistant_test.cc b/profman/profile_assistant_test.cc
index 4fc8143..f7c4255 100644
--- a/profman/profile_assistant_test.cc
+++ b/profman/profile_assistant_test.cc
@@ -2068,8 +2068,8 @@
argv_str.push_back("--copy-and-update-profile-key");
std::string error;
- // Must return kCopyAndUpdateNoUpdate.
- ASSERT_EQ(ExecAndReturnCode(argv_str, &error), ProfmanResult::kCopyAndUpdateNoUpdate) << error;
+ // Must return kCopyAndUpdateNoMatch.
+ ASSERT_EQ(ExecAndReturnCode(argv_str, &error), ProfmanResult::kCopyAndUpdateNoMatch) << error;
// Verify that the content is the same.
std::string output_content;
diff --git a/profman/profman.cc b/profman/profman.cc
index 8e2f3b1..b0a3687 100644
--- a/profman/profman.cc
+++ b/profman/profman.cc
@@ -1880,8 +1880,8 @@
// Open the dex files to look up classes and methods.
std::vector<std::unique_ptr<const DexFile>> dex_files;
OpenApkFilesFromLocations(&dex_files);
- bool updated = false;
- if (!profile.UpdateProfileKeys(dex_files, &updated)) {
+ bool matched = false;
+ if (!profile.UpdateProfileKeys(dex_files, &matched)) {
return ProfmanResult::kCopyAndUpdateErrorFailedToUpdateProfile;
}
bool result = use_fds
@@ -1890,7 +1890,7 @@
if (!result) {
return ProfmanResult::kCopyAndUpdateErrorFailedToSaveProfile;
}
- return updated ? ProfmanResult::kCopyAndUpdateSuccess : ProfmanResult::kCopyAndUpdateNoUpdate;
+ return matched ? ProfmanResult::kCopyAndUpdateSuccess : ProfmanResult::kCopyAndUpdateNoMatch;
} else {
return ProfmanResult::kCopyAndUpdateErrorFailedToLoadProfile;
}