Discard corrupted or out of date profiles
Until now we bailed out if the previous profile contained invalid data.
This CLs forces the save and clears any data in a profile that has the
wrong version or contains bad data.
Bug: 27081617
(cherry picked from commit fe297a96bc6d3da11579709add9b4568730d2b4f)
Change-Id: I9184e0483ea0a869d7aa92630acd6fa04a9d2e03
diff --git a/runtime/jit/offline_profiling_info.cc b/runtime/jit/offline_profiling_info.cc
index fa71905..024c73a 100644
--- a/runtime/jit/offline_profiling_info.cc
+++ b/runtime/jit/offline_profiling_info.cc
@@ -75,7 +75,9 @@
return true;
}
-bool ProfileCompilationInfo::MergeAndSave(const std::string& filename, uint64_t* bytes_written) {
+bool ProfileCompilationInfo::MergeAndSave(const std::string& filename,
+ uint64_t* bytes_written,
+ bool force) {
ScopedTrace trace(__PRETTY_FUNCTION__);
ScopedFlock flock;
std::string error;
@@ -88,26 +90,35 @@
// Load the file but keep a copy around to be able to infer if the content has changed.
ProfileCompilationInfo fileInfo;
- if (!fileInfo.Load(fd)) {
- LOG(WARNING) << "Could not load previous profile data from file " << filename;
+ ProfileLoadSatus status = fileInfo.LoadInternal(fd, &error);
+ if (status == kProfileLoadSuccess) {
+ // Merge the content of file into the current object.
+ if (MergeWith(fileInfo)) {
+ // If after the merge we have the same data as what is the file there's no point
+ // in actually doing the write. The file will be exactly the same as before.
+ if (Equals(fileInfo)) {
+ if (bytes_written != nullptr) {
+ *bytes_written = 0;
+ }
+ return true;
+ }
+ } else {
+ LOG(WARNING) << "Could not merge previous profile data from file " << filename;
+ if (!force) {
+ return false;
+ }
+ }
+ } else if (force &&
+ ((status == kProfileLoadVersionMismatch) || (status == kProfileLoadBadData))) {
+ // Log a warning but don't return false. We will clear the profile anyway.
+ LOG(WARNING) << "Clearing bad or obsolete profile data from file "
+ << filename << ": " << error;
+ } else {
+ LOG(WARNING) << "Could not load profile data from file " << filename << ": " << error;
return false;
}
- // Merge the content of file into the current object.
- if (!MergeWith(fileInfo)) {
- LOG(WARNING) << "Could not merge previous profile data from file " << filename;
- }
-
- // If after the merge we have the same data as what is the file there's no point
- // in actually doing the write. The file will be exactly the same as before.
- if (Equals(fileInfo)) {
- if (bytes_written != nullptr) {
- *bytes_written = 0;
- }
- return true;
- }
-
- // We need to clear the data because we don't support append to the profiles yet.
+ // We need to clear the data because we don't support appending to the profiles yet.
if (!flock.GetFile()->ClearContent()) {
PLOG(WARNING) << "Could not clear profile file: " << filename;
return false;
@@ -128,31 +139,6 @@
return result;
}
-bool ProfileCompilationInfo::SaveProfilingInfo(
- const std::string& filename,
- const std::vector<ArtMethod*>& methods,
- const std::set<DexCacheResolvedClasses>& resolved_classes,
- uint64_t* bytes_written) {
- if (methods.empty() && resolved_classes.empty()) {
- VLOG(profiler) << "No info to save to " << filename;
- if (bytes_written != nullptr) {
- *bytes_written = 0;
- }
- return true;
- }
-
- ProfileCompilationInfo info;
- if (!info.AddMethodsAndClasses(methods, resolved_classes)) {
- LOG(WARNING) << "Checksum mismatch when processing methods and resolved classes for "
- << filename;
- if (bytes_written != nullptr) {
- *bytes_written = 0;
- }
- return false;
- }
- return info.MergeAndSave(filename, bytes_written);
-}
-
// Returns true if all the bytes were successfully written to the file descriptor.
static bool WriteBuffer(int fd, const uint8_t* buffer, size_t byte_count) {
while (byte_count > 0) {
@@ -535,18 +521,25 @@
}
bool ProfileCompilationInfo::MergeWith(const ProfileCompilationInfo& other) {
+ // First verify that all checksums match. This will avoid adding garbage to
+ // the current profile info.
+ // Note that the number of elements should be very small, so this should not
+ // be a performance issue.
+ for (const auto& other_it : other.info_) {
+ auto info_it = info_.find(other_it.first);
+ if ((info_it != info_.end()) && (info_it->second.checksum != other_it.second.checksum)) {
+ LOG(WARNING) << "Checksum mismatch for dex " << other_it.first;
+ return false;
+ }
+ }
+ // All checksums match. Import the data.
for (const auto& other_it : other.info_) {
const std::string& other_dex_location = other_it.first;
const DexFileData& other_dex_data = other_it.second;
-
auto info_it = info_.find(other_dex_location);
if (info_it == info_.end()) {
info_it = info_.Put(other_dex_location, DexFileData(other_dex_data.checksum));
}
- if (info_it->second.checksum != other_dex_data.checksum) {
- LOG(WARNING) << "Checksum mismatch for dex " << other_dex_location;
- return false;
- }
info_it->second.method_set.insert(other_dex_data.method_set.begin(),
other_dex_data.method_set.end());
info_it->second.class_set.insert(other_dex_data.class_set.begin(),
diff --git a/runtime/jit/offline_profiling_info.h b/runtime/jit/offline_profiling_info.h
index 492f9f8..c4055b3 100644
--- a/runtime/jit/offline_profiling_info.h
+++ b/runtime/jit/offline_profiling_info.h
@@ -44,14 +44,6 @@
static const uint8_t kProfileMagic[];
static const uint8_t kProfileVersion[];
- // Saves profile information about the given methods in the given file.
- // Note that the saving proceeds only if the file can be locked for exclusive access.
- // If not (the locking is not blocking), the function does not save and returns false.
- static bool SaveProfilingInfo(const std::string& filename,
- const std::vector<ArtMethod*>& methods,
- const std::set<DexCacheResolvedClasses>& resolved_classes,
- uint64_t* bytes_written = nullptr);
-
// Add the given methods and classes to the current profile object.
bool AddMethodsAndClasses(const std::vector<ArtMethod*>& methods,
const std::set<DexCacheResolvedClasses>& resolved_classes);
@@ -63,7 +55,10 @@
bool Save(int fd);
// Loads and merges profile information from the given file into the current
// object and tries to save it back to disk.
- bool MergeAndSave(const std::string& filename, uint64_t* bytes_written);
+ // If `force` is true then the save will go through even if the given file
+ // has bad data or its version does not match. In this cases the profile content
+ // is ignored.
+ bool MergeAndSave(const std::string& filename, uint64_t* bytes_written, bool force);
// Returns the number of methods that were profiled.
uint32_t GetNumberOfMethods() const;
diff --git a/runtime/jit/profile_compilation_info_test.cc b/runtime/jit/profile_compilation_info_test.cc
index db681f4..ad9af70 100644
--- a/runtime/jit/profile_compilation_info_test.cc
+++ b/runtime/jit/profile_compilation_info_test.cc
@@ -67,6 +67,17 @@
return static_cast<uint32_t>(file.GetFd());
}
+ bool SaveProfilingInfo(
+ const std::string& filename,
+ const std::vector<ArtMethod*>& methods,
+ const std::set<DexCacheResolvedClasses>& resolved_classes) {
+ ProfileCompilationInfo info;
+ if (!info.AddMethodsAndClasses(methods, resolved_classes)) {
+ return false;
+ }
+ return info.MergeAndSave(filename, nullptr, false);
+ }
+
// Cannot sizeof the actual arrays so hardcode the values here.
// They should not change anyway.
static constexpr int kProfileMagicSize = 4;
@@ -87,9 +98,7 @@
// Save virtual methods from Main.
std::set<DexCacheResolvedClasses> resolved_classes;
std::vector<ArtMethod*> main_methods = GetVirtualMethods(class_loader, "LMain;");
- ASSERT_TRUE(ProfileCompilationInfo::SaveProfilingInfo(profile.GetFilename(),
- main_methods,
- resolved_classes));
+ ASSERT_TRUE(SaveProfilingInfo(profile.GetFilename(), main_methods, resolved_classes));
// Check that what we saved is in the profile.
ProfileCompilationInfo info1;
@@ -104,9 +113,7 @@
// Save virtual methods from Second.
std::vector<ArtMethod*> second_methods = GetVirtualMethods(class_loader, "LSecond;");
- ASSERT_TRUE(ProfileCompilationInfo::SaveProfilingInfo(profile.GetFilename(),
- second_methods,
- resolved_classes));
+ ASSERT_TRUE(SaveProfilingInfo(profile.GetFilename(), second_methods, resolved_classes));
// Check that what we saved is in the profile (methods form Main and Second).
ProfileCompilationInfo info2;
diff --git a/runtime/jit/profile_saver.cc b/runtime/jit/profile_saver.cc
index 81d81a5..df42a80 100644
--- a/runtime/jit/profile_saver.cc
+++ b/runtime/jit/profile_saver.cc
@@ -211,7 +211,9 @@
continue;
}
uint64_t bytes_written;
- if (cached_info->MergeAndSave(filename, &bytes_written)) {
+ // Force the save. In case the profile data is corrupted or the the profile
+ // has the wrong version this will "fix" the file to the correct format.
+ if (cached_info->MergeAndSave(filename, &bytes_written, /*force*/ true)) {
last_save_number_of_methods_ = cached_info->GetNumberOfMethods();
last_save_number_of_classes__ = cached_info->GetNumberOfResolvedClasses();
// Clear resolved classes. No need to store them around as