Refactor RemoveNativeDebugInfoForJit.

This is partial revert of CL/1099280 (Remove global maps).

It somewhat resurrects the lazy method removal.

The original goal was to only remove methods from the GC,
and do all of them in bulk for simplicity and efficiency.

However, this is proving infeasible since we have several
corner cases which remove methods outside the GC code path.

The behaviour for the GC code path is preserved by this CL.
Instead of passing method array, the methods are individually
marked for removal and then repacking is immediately forced.
The only difference is that coroner cases are done lazily.

Test: ./art/test.py -b -r --host --jit --64
Change-Id: I42729545d6b51df788d92f9cf149a6e065b90c68

5 files changed, 82 insertions, 34 deletions

diff --git a/compiler/optimizing/optimizing_compiler.cc b/compiler/optimizing/optimizing_compiler.cc
index 0344fc565a..9978a6fa18 100644
--- a/compiler/optimizing/optimizing_compiler.cc
+++ b/compiler/optimizing/optimizing_compiler.cc
@@ -1295,8 +1295,6 @@ bool OptimizingCompiler::JitCompile(Thread* self,
                             /* has_should_deoptimize_flag= */ false,
                             cha_single_implementation_list)) {
       code_cache->Free(self, region, reserved_code.data(), reserved_data.data());
-      MutexLock mu(self, *Locks::jit_lock_);
-      RemoveNativeDebugInfoForJit(ArrayRef<const void*>(reinterpret_cast<const void**>(&code), 1));
       return false;
     }
 
@@ -1405,8 +1403,6 @@ bool OptimizingCompiler::JitCompile(Thread* self,
                           codegen->GetGraph()->HasShouldDeoptimizeFlag(),
                           codegen->GetGraph()->GetCHASingleImplementationList())) {
     code_cache->Free(self, region, reserved_code.data(), reserved_data.data());
-    MutexLock mu(self, *Locks::jit_lock_);
-    RemoveNativeDebugInfoForJit(ArrayRef<const void*>(reinterpret_cast<const void**>(&code), 1));
     return false;
   }
 
diff --git a/runtime/jit/debugger_interface.cc b/runtime/jit/debugger_interface.cc
index cecf533a7d..0efa4d2cdd 100644
--- a/runtime/jit/debugger_interface.cc
+++ b/runtime/jit/debugger_interface.cc
@@ -101,7 +101,14 @@ static Mutex g_dex_debug_lock("DEX native debug entries", kNativeDebugInterfaceL
 // Some writes are synchronized so libunwindstack can read the memory safely from another process.
 constexpr std::memory_order kNonRacingRelaxed = std::memory_order_relaxed;
 
+// Size of JIT code range covered by each packed JITCodeEntry.
+constexpr uint32_t kJitRepackGroupSize = 64 * KB;
+
+// Automatically call the repack method every 'n' new entries.
+constexpr uint32_t kJitRepackFrequency = 64;
+
 // Public binary interface between ART and native tools (gdb, libunwind, etc).
+// The fields below need to be exported and have special names as per the gdb api.
 extern "C" {
   enum JITAction {
     JIT_NOACTION = 0,
@@ -191,6 +198,18 @@ extern "C" {
   JITDescriptor __dex_debug_descriptor GUARDED_BY(g_dex_debug_lock) {};
 }
 
+// The fields below are internal, but we keep them here anyway for consistency.
+// Their state is related to the static state above and it must be kept in sync.
+
+// Used only in debug builds to check that we are not adding duplicate entries.
+static std::unordered_set<const void*> g_dcheck_all_jit_functions GUARDED_BY(g_jit_debug_lock);
+
+// Methods that have been marked for deletion on the next repack pass.
+static std::vector<const void*> g_removed_jit_functions GUARDED_BY(g_jit_debug_lock);
+
+// Number of small (single symbol) ELF files. Used to trigger repacking.
+static uint32_t g_jit_num_unpacked_entries = 0;
+
 struct DexNativeInfo {
   static constexpr bool kCopySymfileData = false;  // Just reference DEX files.
   static JITDescriptor& Descriptor() { return __dex_debug_descriptor; }
@@ -459,13 +478,6 @@ void NativeDebugInfoPostFork() {
   descriptor.free_entries_ = nullptr;  // Don't reuse zygote's entries.
 }
 
-// Size of JIT code range covered by each packed JITCodeEntry.
-static constexpr uint32_t kJitRepackGroupSize = 64 * KB;
-
-// Automatically call the repack method every 'n' new entries.
-static constexpr uint32_t kJitRepackFrequency = 64;
-static uint32_t g_jit_num_unpacked_entries = 0;
-
 // Split the JIT code cache into groups of fixed size and create single JITCodeEntry for each group.
 // The start address of method's code determines which group it belongs to.  The end is irrelevant.
 // New mini debug infos will be merged if possible, and entries for GCed functions will be removed.
@@ -549,11 +561,23 @@ static void RepackEntries(bool compress_entries, ArrayRef<const void*> removed)
   g_jit_num_unpacked_entries = 0;
 }
 
+void RepackNativeDebugInfoForJitLocked() REQUIRES(g_jit_debug_lock);
+
 void AddNativeDebugInfoForJit(const void* code_ptr,
                               const std::vector<uint8_t>& symfile,
                               bool allow_packing) {
   MutexLock mu(Thread::Current(), g_jit_debug_lock);
   DCHECK_NE(symfile.size(), 0u);
+  if (kIsDebugBuild && code_ptr != nullptr) {
+    DCHECK(g_dcheck_all_jit_functions.insert(code_ptr).second) << code_ptr << " already added";
+  }
+
+  // Remove all methods which have been marked for removal.  The JIT GC should
+  // force repack, so this should happen only rarely for various corner cases.
+  // Must be done before addition in case the added code_ptr is in the removed set.
+  if (!g_removed_jit_functions.empty()) {
+    RepackNativeDebugInfoForJitLocked();
+  }
 
   CreateJITCodeEntryInternal<JitNativeInfo>(ArrayRef<const uint8_t>(symfile),
                                             /*addr=*/ code_ptr,
@@ -575,9 +599,20 @@ void AddNativeDebugInfoForJit(const void* code_ptr,
   }
 }
 
-void RemoveNativeDebugInfoForJit(ArrayRef<const void*> removed) {
+void RemoveNativeDebugInfoForJit(const void* code_ptr) {
   MutexLock mu(Thread::Current(), g_jit_debug_lock);
-  RepackEntries(/*compress_entries=*/ true, removed);
+  g_dcheck_all_jit_functions.erase(code_ptr);
+
+  // Method removal is very expensive since we need to decompress and read ELF files.
+  // Collet methods to be removed and do the removal in bulk later.
+  g_removed_jit_functions.push_back(code_ptr);
+}
+
+void RepackNativeDebugInfoForJitLocked() {
+  // Remove entries which are inside packed and compressed ELF files.
+  std::vector<const void*>& removed = g_removed_jit_functions;
+  std::sort(removed.begin(), removed.end());
+  RepackEntries(/*compress_entries=*/ true, ArrayRef<const void*>(removed));
 
   // Remove entries which are not allowed to be packed (containing single method each).
   for (const JITCodeEntry* it = __jit_debug_descriptor.head_; it != nullptr;) {
@@ -587,6 +622,14 @@ void RemoveNativeDebugInfoForJit(ArrayRef<const void*> removed) {
     }
     it = next;
   }
+
+  removed.clear();
+  removed.shrink_to_fit();
+}
+
+void RepackNativeDebugInfoForJit() {
+  MutexLock mu(Thread::Current(), g_jit_debug_lock);
+  RepackNativeDebugInfoForJitLocked();
 }
 
 size_t GetJitMiniDebugInfoMemUsage() {
diff --git a/runtime/jit/debugger_interface.h b/runtime/jit/debugger_interface.h
index 477d58c24c..8433e693d7 100644
--- a/runtime/jit/debugger_interface.h
+++ b/runtime/jit/debugger_interface.h
@@ -57,7 +57,11 @@ void AddNativeDebugInfoForJit(const void* code_ptr,
     REQUIRES_SHARED(Locks::jit_lock_);  // Might need JIT code cache to allocate memory.
 
 // Notify native tools (e.g. libunwind) that JIT code has been garbage collected.
-void RemoveNativeDebugInfoForJit(ArrayRef<const void*> removed_code_ptrs)
+// The actual removal might be lazy. Removal of address that was not added is no-op.
+void RemoveNativeDebugInfoForJit(const void* code_ptr);
+
+// Merge and compress entries to save space.
+void RepackNativeDebugInfoForJit()
     REQUIRES_SHARED(Locks::jit_lock_);  // Might need JIT code cache to allocate memory.
 
 // Returns approximate memory used by debug info for JIT code.
diff --git a/runtime/jit/jit_code_cache.cc b/runtime/jit/jit_code_cache.cc
index 7ad4700c21..3b8c2a7ebd 100644
--- a/runtime/jit/jit_code_cache.cc
+++ b/runtime/jit/jit_code_cache.cc
@@ -377,6 +377,11 @@ static uintptr_t FromCodeToAllocation(const void* code) {
   return reinterpret_cast<uintptr_t>(code) - RoundUp(sizeof(OatQuickMethodHeader), alignment);
 }
 
+static const void* FromAllocationToCode(const uint8_t* alloc) {
+  size_t alignment = GetInstructionSetAlignment(kRuntimeISA);
+  return reinterpret_cast<const void*>(alloc + RoundUp(sizeof(OatQuickMethodHeader), alignment));
+}
+
 static uint32_t GetNumberOfRoots(const uint8_t* stack_map) {
   // The length of the table is stored just before the stack map (and therefore at the end of
   // the table itself), in order to be able to fetch it from a `stack_map` pointer.
@@ -459,22 +464,18 @@ void JitCodeCache::SweepRootTables(IsMarkedVisitor* visitor) {
   }
 }
 
-void JitCodeCache::FreeCodeAndData(const void* code_ptr, bool free_debug_info) {
+void JitCodeCache::FreeCodeAndData(const void* code_ptr) {
   if (IsInZygoteExecSpace(code_ptr)) {
     // No need to free, this is shared memory.
     return;
   }
   uintptr_t allocation = FromCodeToAllocation(code_ptr);
-  if (free_debug_info) {
-    // Remove compressed mini-debug info for the method.
-    // TODO: This is expensive, so we should always do it in the caller in bulk.
-    RemoveNativeDebugInfoForJit(ArrayRef<const void*>(&code_ptr, 1));
-  }
+  const uint8_t* data = nullptr;
   if (OatQuickMethodHeader::FromCodePointer(code_ptr)->IsOptimized()) {
-    private_region_.FreeData(GetRootTable(code_ptr));
+    data = GetRootTable(code_ptr);
   }  // else this is a JNI stub without any data.
 
-  private_region_.FreeCode(reinterpret_cast<uint8_t*>(allocation));
+  FreeLocked(&private_region_, reinterpret_cast<uint8_t*>(allocation), data);
 }
 
 void JitCodeCache::FreeAllMethodHeaders(
@@ -490,19 +491,13 @@ void JitCodeCache::FreeAllMethodHeaders(
         ->RemoveDependentsWithMethodHeaders(method_headers);
   }
 
-  // Remove compressed mini-debug info for the methods.
-  std::vector<const void*> removed_symbols;
-  removed_symbols.reserve(method_headers.size());
-  for (const OatQuickMethodHeader* method_header : method_headers) {
-    removed_symbols.push_back(method_header->GetCode());
-  }
-  std::sort(removed_symbols.begin(), removed_symbols.end());
-  RemoveNativeDebugInfoForJit(ArrayRef<const void*>(removed_symbols));
-
   ScopedCodeCacheWrite scc(private_region_);
   for (const OatQuickMethodHeader* method_header : method_headers) {
-    FreeCodeAndData(method_header->GetCode(), /*free_debug_info=*/ false);
+    FreeCodeAndData(method_header->GetCode());
   }
+
+  // We have potentially removed a lot of debug info. Do maintenance pass to save space.
+  RepackNativeDebugInfoForJit();
 }
 
 void JitCodeCache::RemoveMethodsIn(Thread* self, const LinearAlloc& alloc) {
@@ -993,7 +988,12 @@ void JitCodeCache::Free(Thread* self,
                         const uint8_t* data) {
   MutexLock mu(self, *Locks::jit_lock_);
   ScopedCodeCacheWrite ccw(*region);
+  FreeLocked(region, code, data);
+}
+
+void JitCodeCache::FreeLocked(JitMemoryRegion* region, const uint8_t* code, const uint8_t* data) {
   if (code != nullptr) {
+    RemoveNativeDebugInfoForJit(reinterpret_cast<const void*>(FromAllocationToCode(code)));
     region->FreeCode(code);
   }
   if (data != nullptr) {
diff --git a/runtime/jit/jit_code_cache.h b/runtime/jit/jit_code_cache.h
index d8216d2cf5..f46dc75d58 100644
--- a/runtime/jit/jit_code_cache.h
+++ b/runtime/jit/jit_code_cache.h
@@ -276,6 +276,9 @@ class JitCodeCache {
   void Free(Thread* self, JitMemoryRegion* region, const uint8_t* code, const uint8_t* data)
       REQUIRES_SHARED(Locks::mutator_lock_)
       REQUIRES(!Locks::jit_lock_);
+  void FreeLocked(JitMemoryRegion* region, const uint8_t* code, const uint8_t* data)
+      REQUIRES_SHARED(Locks::mutator_lock_)
+      REQUIRES(Locks::jit_lock_);
 
   // Perform a collection on the code cache.
   void GarbageCollectCache(Thread* self)
@@ -422,6 +425,7 @@ class JitCodeCache {
 
   // Remove CHA dependents and underlying allocations for entries in `method_headers`.
   void FreeAllMethodHeaders(const std::unordered_set<OatQuickMethodHeader*>& method_headers)
+      REQUIRES_SHARED(Locks::mutator_lock_)
       REQUIRES(!Locks::jit_lock_)
       REQUIRES(!Locks::cha_lock_);
 
@@ -432,8 +436,9 @@ class JitCodeCache {
       REQUIRES(Locks::mutator_lock_);
 
   // Free code and data allocations for `code_ptr`.
-  void FreeCodeAndData(const void* code_ptr, bool free_debug_info = true)
-      REQUIRES(Locks::jit_lock_);
+  void FreeCodeAndData(const void* code_ptr)
+      REQUIRES(Locks::jit_lock_)
+      REQUIRES_SHARED(Locks::mutator_lock_);
 
   // Number of bytes allocated in the code cache.
   size_t CodeCacheSize() REQUIRES(!Locks::jit_lock_);