Skip to content

Commit

Permalink
8335409: Can't allocate and retain memory from resource area in frame…
Browse files Browse the repository at this point in the history
…::oops_interpreted_do oop closure after 8329665

Reviewed-by: phh, stuefe
Backport-of: 7ab96c74e2c39f430a5c2f65a981da7314a2385b
  • Loading branch information
shipilev committed Aug 27, 2024
1 parent 345b417 commit db914b7
Showing 3 changed files with 40 additions and 45 deletions.
60 changes: 26 additions & 34 deletions src/hotspot/share/interpreter/oopMapCache.cpp
Original file line number Diff line number Diff line change
@@ -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);
}
24 changes: 14 additions & 10 deletions src/hotspot/share/interpreter/oopMapCache.hpp
Original file line number Diff line number Diff line change
@@ -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> {
1 change: 0 additions & 1 deletion src/hotspot/share/runtime/frame.cpp
Original file line number Diff line number Diff line change
@@ -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);

1 comment on commit db914b7

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.