When an entry is removed from a ResourceHashtable, sometimes it's necessary to perform special cleanup. Currently this is only possible via the unlink() API:
https://github.com/openjdk/jdk/blob/5dfc4ec7d94af9fe39fdee9d83b06101b827a3c6/src/hotspot/share/utilities/resourceHash.hpp#L258-L278
template<class ITER>
void unlink(ITER* iter, bool dummy) {
const unsigned sz = table_size();
for (unsigned index = 0; index < sz; index++) {
Node** ptr = bucket_at(index);
while (*ptr != nullptr) {
Node* node = *ptr;
// do_entry must clean up the key and value in Node.
bool clean = iter->do_entry(node->_key, node->_value);
if (clean) {
*ptr = node->_next;
if (ALLOC_TYPE == AnyObj::C_HEAP) {
delete node;
}
If the node->_value needs special clean up, it must be done inside iter->do_entry(). Here's an example:
https://github.com/openjdk/jdk/blob/5dfc4ec7d94af9fe39fdee9d83b06101b827a3c6/src/hotspot/share/oops/instanceKlass.cpp#L1005-L1020
void InstanceKlass::clean_initialization_error_table() {
struct InitErrorTableCleaner {
bool do_entry(const InstanceKlass* ik, OopHandle h) {
if (!ik->is_loader_alive()) {
h.release(Universe::vm_global());
return true;
} else {
return false;
}
}
};
assert_locked_or_safepoint(ClassInitError_lock);
InitErrorTableCleaner cleaner;
_initialization_error_table.unlink(&cleaner);
}
However, entries can also be removed via the remove() API, or in the ResourceHashtable's destructor. In these cases, the caller must perform the special clean up in a different way than with the unlink() API.
With the current implementation, if the entries require special clean up, we really should assert in the ResourceHashtable's destructor that the table must be empty, or else there would be memory leak. However, with the current API, there is no way to declare that the entries need special clean up.
Also, the destructors for the _key and _value members of the ResourceHashtableNode are NOT called if the table is allocated using AnyObj::RESOURCE_AREA. This will lead to memory leaks if the table contains SymbolHandle, for example.
======================================
Proposal:
[1] The ResourceHashtableNode destructor should be called even if the table is allocated in RESOURCE_AREA. This ensures that the ~K() and ~V() destructors are called.
[2] Look at all the calls to the unlink() APIs -- for most of them, we should be able to perform the clean up inside the ~K() and ~V() destructors. Example: SourceObjInfo. SeeJDK-8301304.
[3] unlink(Function F) should be changed so that F cannot modify the key or the value. I.e., F should be used only to check whether an entry should be removed or not. It should not be used for deallocation.
[4] See if we still need a special variant of unlink(Function F) that allows F to modify the value. E.g., can the InstanceKlass::clean_initialization_error_table() example above be implemented using destructors?
https://github.com/openjdk/jdk/blob/5dfc4ec7d94af9fe39fdee9d83b06101b827a3c6/src/hotspot/share/utilities/resourceHash.hpp#L258-L278
template<class ITER>
void unlink(ITER* iter, bool dummy) {
const unsigned sz = table_size();
for (unsigned index = 0; index < sz; index++) {
Node** ptr = bucket_at(index);
while (*ptr != nullptr) {
Node* node = *ptr;
// do_entry must clean up the key and value in Node.
bool clean = iter->do_entry(node->_key, node->_value);
if (clean) {
*ptr = node->_next;
if (ALLOC_TYPE == AnyObj::C_HEAP) {
delete node;
}
If the node->_value needs special clean up, it must be done inside iter->do_entry(). Here's an example:
https://github.com/openjdk/jdk/blob/5dfc4ec7d94af9fe39fdee9d83b06101b827a3c6/src/hotspot/share/oops/instanceKlass.cpp#L1005-L1020
void InstanceKlass::clean_initialization_error_table() {
struct InitErrorTableCleaner {
bool do_entry(const InstanceKlass* ik, OopHandle h) {
if (!ik->is_loader_alive()) {
h.release(Universe::vm_global());
return true;
} else {
return false;
}
}
};
assert_locked_or_safepoint(ClassInitError_lock);
InitErrorTableCleaner cleaner;
_initialization_error_table.unlink(&cleaner);
}
However, entries can also be removed via the remove() API, or in the ResourceHashtable's destructor. In these cases, the caller must perform the special clean up in a different way than with the unlink() API.
With the current implementation, if the entries require special clean up, we really should assert in the ResourceHashtable's destructor that the table must be empty, or else there would be memory leak. However, with the current API, there is no way to declare that the entries need special clean up.
Also, the destructors for the _key and _value members of the ResourceHashtableNode are NOT called if the table is allocated using AnyObj::RESOURCE_AREA. This will lead to memory leaks if the table contains SymbolHandle, for example.
======================================
Proposal:
[1] The ResourceHashtableNode destructor should be called even if the table is allocated in RESOURCE_AREA. This ensures that the ~K() and ~V() destructors are called.
[2] Look at all the calls to the unlink() APIs -- for most of them, we should be able to perform the clean up inside the ~K() and ~V() destructors. Example: SourceObjInfo. See
[3] unlink(Function F) should be changed so that F cannot modify the key or the value. I.e., F should be used only to check whether an entry should be removed or not. It should not be used for deallocation.
[4] See if we still need a special variant of unlink(Function F) that allows F to modify the value. E.g., can the InstanceKlass::clean_initialization_error_table() example above be implemented using destructors?
- relates to
-
JDK-8301136 Improve unlink() and unlink_all() of ResourceHashtableBase
- Closed