OSDN Git Service

Discard corrupted or out of date profiles
authorCalin Juravle <calin@google.com>
Thu, 24 Mar 2016 20:33:22 +0000 (20:33 +0000)
committerCalin Juravle <calin@google.com>
Fri, 22 Apr 2016 15:12:11 +0000 (16:12 +0100)
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

runtime/jit/offline_profiling_info.cc
runtime/jit/offline_profiling_info.h
runtime/jit/profile_compilation_info_test.cc
runtime/jit/profile_saver.cc

index fa71905..024c73a 100644 (file)
@@ -75,7 +75,9 @@ bool ProfileCompilationInfo::AddMethodsAndClasses(
   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 @@ bool ProfileCompilationInfo::MergeAndSave(const std::string& filename, uint64_t*
 
   // 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;
-    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;
+  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;
+      }
     }
-    return true;
+  } 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;
   }
 
-  // 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 @@ bool ProfileCompilationInfo::MergeAndSave(const std::string& filename, uint64_t*
   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 @@ ProfileCompilationInfo::ProfileLoadSatus ProfileCompilationInfo::LoadInternal(
 }
 
 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(),
index 492f9f8..c4055b3 100644 (file)
@@ -44,14 +44,6 @@ class ProfileCompilationInfo {
   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 @@ class ProfileCompilationInfo {
   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;
index db681f4..ad9af70 100644 (file)
@@ -67,6 +67,17 @@ class ProfileCompilationInfoTest : public CommonRuntimeTest {
     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 @@ TEST_F(ProfileCompilationInfoTest, SaveArtMethods) {
   // 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 @@ TEST_F(ProfileCompilationInfoTest, SaveArtMethods) {
 
   // 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;
index 81d81a5..df42a80 100644 (file)
@@ -211,7 +211,9 @@ bool ProfileSaver::ProcessProfilingInfo() {
       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