8335409: Can't allocate and retain memory from resource area in frame::oops_interpreted_do oop closure after 8329665

Reviewed-by: phh, stuefe
Backport-of: 7ab96c74e2c39f430a5c2f65a981da7314a2385b

Author: shipilev

src/hotspot/share/interpreter/oopMapCache.cpp

@@ -64,9 +64,6 @@ class OopMapCacheEntry: private InterpreterOopMap {
  public:
   OopMapCacheEntry() : InterpreterOopMap() {
     _next = nullptr;
-#ifdef ASSERT
-    _resource_allocate_bit_mask = false;
-#endif
   }
 };
 
@@ -175,9 +172,13 @@ class VerifyClosure : public OffsetClosure {
 
 InterpreterOopMap::InterpreterOopMap() {
   initialize();
-#ifdef ASSERT
-  _resource_allocate_bit_mask = true;
-#endif
+}
+
+InterpreterOopMap::~InterpreterOopMap() {
+  if (has_valid_mask() && mask_size() > small_mask_limit) {
+    assert(_bit_mask[0] != 0, "should have pointer to C heap");
+    FREE_C_HEAP_ARRAY(uintptr_t, _bit_mask[0]);
+  }
 }
 
 bool InterpreterOopMap::is_empty() const {
@@ -397,35 +398,24 @@ void OopMapCacheEntry::deallocate(OopMapCacheEntry* const entry) {
 
 // Implementation of OopMapCache
 
-void InterpreterOopMap::resource_copy(OopMapCacheEntry* from) {
-  assert(_resource_allocate_bit_mask,
-    "Should not resource allocate the _bit_mask");
+void InterpreterOopMap::copy_from(const OopMapCacheEntry* src) {
+  // The expectation is that this InterpreterOopMap is recently created
+  // and empty. It is used to get a copy of a cached entry.
+  assert(!has_valid_mask(), "InterpreterOopMap object can only be filled once");
+  assert(src->has_valid_mask(), "Cannot copy entry with an invalid mask");
 
-  set_method(from->method());
-  set_bci(from->bci());
-  set_mask_size(from->mask_size());
-  set_expression_stack_size(from->expression_stack_size());
-  _num_oops = from->num_oops();
+  set_method(src->method());
+  set_bci(src->bci());
+  set_mask_size(src->mask_size());
+  set_expression_stack_size(src->expression_stack_size());
+  _num_oops = src->num_oops();
 
   // Is the bit mask contained in the entry?
-  if (from->mask_size() <= small_mask_limit) {
-    memcpy((void *)_bit_mask, (void *)from->_bit_mask,
-      mask_word_size() * BytesPerWord);
+  if (src->mask_size() <= small_mask_limit) {
+    memcpy(_bit_mask, src->_bit_mask, mask_word_size() * BytesPerWord);
   } else {
-    // The expectation is that this InterpreterOopMap is a recently created
-    // and empty. It is used to get a copy of a cached entry.
-    // If the bit mask has a value, it should be in the
-    // resource area.
-    assert(_bit_mask[0] == 0 ||
-      Thread::current()->resource_area()->contains((void*)_bit_mask[0]),
-      "The bit mask should have been allocated from a resource area");
-    // Allocate the bit_mask from a Resource area for performance.  Allocating
-    // from the C heap as is done for OopMapCache has a significant
-    // performance impact.
-    _bit_mask[0] = (uintptr_t) NEW_RESOURCE_ARRAY(uintptr_t, mask_word_size());
-    assert(_bit_mask[0] != 0, "bit mask was not allocated");
-    memcpy((void*) _bit_mask[0], (void*) from->_bit_mask[0],
-      mask_word_size() * BytesPerWord);
+    _bit_mask[0] = (uintptr_t) NEW_C_HEAP_ARRAY(uintptr_t, mask_word_size(), mtClass);
+    memcpy((void*) _bit_mask[0], (void*) src->_bit_mask[0], mask_word_size() * BytesPerWord);
   }
 }
 
@@ -508,7 +498,7 @@ void OopMapCache::lookup(const methodHandle& method,
     for (int i = 0; i < probe_depth; i++) {
       OopMapCacheEntry *entry = entry_at(probe + i);
       if (entry != nullptr && !entry->is_empty() && entry->match(method, bci)) {
-        entry_for->resource_copy(entry);
+        entry_for->copy_from(entry);
         assert(!entry_for->is_empty(), "A non-empty oop map should be returned");
         log_debug(interpreter, oopmap)("- found at hash %d", probe + i);
         return;
@@ -522,7 +512,7 @@ void OopMapCache::lookup(const methodHandle& method,
   OopMapCacheEntry* tmp = NEW_C_HEAP_OBJ(OopMapCacheEntry, mtClass);
   tmp->initialize();
   tmp->fill(method, bci);
-  entry_for->resource_copy(tmp);
+  entry_for->copy_from(tmp);
 
   if (method->should_not_be_cached()) {
     // It is either not safe or not a good idea to cache this Method*
@@ -622,6 +612,8 @@ void OopMapCache::compute_one_oop_map(const methodHandle& method, int bci, Inter
   OopMapCacheEntry* tmp = NEW_C_HEAP_OBJ(OopMapCacheEntry, mtClass);
   tmp->initialize();
   tmp->fill(method, bci);
-  entry->resource_copy(tmp);
+  if (tmp->has_valid_mask()) {
+    entry->copy_from(tmp);
+  }
   OopMapCacheEntry::deallocate(tmp);
 }

src/hotspot/share/interpreter/oopMapCache.hpp

@@ -35,13 +35,14 @@
 // OopMapCache's are allocated lazily per InstanceKlass.
 
 // The oopMap (InterpreterOopMap) is stored as a bit mask. If the
-// bit_mask can fit into two words it is stored in
+// bit_mask can fit into four words it is stored in
 // the _bit_mask array, otherwise it is allocated on the heap.
 // For OopMapCacheEntry the bit_mask is allocated in the C heap
 // because these entries persist between garbage collections.
-// For InterpreterOopMap the bit_mask is allocated in
-// a resource area for better performance.  InterpreterOopMap
-// should only be created and deleted during same garbage collection.
+// For InterpreterOopMap the bit_mask is allocated in the C heap
+// to avoid issues with allocations from the resource area that have
+// to live accross the oop closure. InterpreterOopMap should only be
+// created and deleted during the same garbage collection.
 //
 // If ENABBLE_ZAP_DEAD_LOCALS is defined, two bits are used
 // per entry instead of one. In all cases,
@@ -82,7 +83,7 @@ class InterpreterOopMap: ResourceObj {
  private:
   Method*        _method;         // the method for which the mask is valid
   unsigned short _bci;            // the bci    for which the mask is valid
-  int            _mask_size;      // the mask size in bits
+  int            _mask_size;      // the mask size in bits (USHRT_MAX if invalid)
   int            _expression_stack_size; // the size of the expression stack in slots
 
  protected:
@@ -126,12 +127,13 @@ class InterpreterOopMap: ResourceObj {
 
  public:
   InterpreterOopMap();
+  ~InterpreterOopMap();
 
-  // Copy the OopMapCacheEntry in parameter "from" into this
-  // InterpreterOopMap.  If the _bit_mask[0] in "from" points to
-  // allocated space (i.e., the bit mask was to large to hold
-  // in-line), allocate the space from a Resource area.
-  void resource_copy(OopMapCacheEntry* from);
+  // Copy the OopMapCacheEntry in parameter "src" into this
+  // InterpreterOopMap.  If the _bit_mask[0] in "src" points to
+  // allocated space (i.e., the bit mask was too large to hold
+  // in-line), allocate the space from the C heap.
+  void copy_from(const OopMapCacheEntry* src);
 
   void iterate_oop(OffsetClosure* oop_closure) const;
   void print() const;
@@ -143,6 +145,8 @@ class InterpreterOopMap: ResourceObj {
 
   int expression_stack_size() const              { return _expression_stack_size; }
 
+  // Determines if a valid mask has been computed
+  bool has_valid_mask() const { return _mask_size != USHRT_MAX; }
 };
 
 class OopMapCache : public CHeapObj<mtClass> {

src/hotspot/share/runtime/frame.cpp

@@ -952,7 +952,6 @@ void frame::oops_interpreted_do(OopClosure* f, const RegisterMap* map, bool quer
   InterpreterFrameClosure blk(this, max_locals, m->max_stack(), f);
 
   // process locals & expression stack
-  ResourceMark rm(thread);
   InterpreterOopMap mask;
   if (query_oop_map_cache) {
     m->mask_for(m, bci, &mask);