Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8267752: KVHashtable doesn't deallocate entries #4501

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -242,15 +242,35 @@ class KVHashtable : public BasicHashtable<F> {
return (KVHashtableEntry*)BasicHashtable<F>::bucket(i);
}

// The following method is not MT-safe and must be done under lock.
KVHashtableEntry** bucket_addr(int i) {
return (KVHashtableEntry**)BasicHashtable<F>::bucket_addr(i);
}

KVHashtableEntry* new_entry(unsigned int hashValue, K key, V value) {
KVHashtableEntry* entry = (KVHashtableEntry*)BasicHashtable<F>::new_entry(hashValue);
entry->_key = key;
entry->_value = value;
return entry;
}

void free_entry(KVHashtableEntry* entry) {
BasicHashtable<F>::free_entry(entry);
}

public:
KVHashtable(int table_size) : BasicHashtable<F>(table_size, sizeof(KVHashtableEntry)) {}
~KVHashtable() {
KVHashtableEntry* probe = NULL;
for (int index = 0; index < table_size(); index++) {
for (KVHashtableEntry** p = bucket_addr(index); *p != NULL; ) {
probe = *p;
*p = probe->next();
This conversation was marked as resolved by coleenp

This comment has been minimized.

@iklam

iklam Jun 16, 2021
Member

Should we also call the destructor (something like probe->_value.~V() , not sure about the syntax). I.e., similar to GrowableArray<E>:

for (int i = 0; i < this->_max; i++) {
this->_data[i].~E();
}

This way, we can get rid of this code, and move delete ref() into SourceObjInfo::~SourceObjInfo()

class SrcObjTableCleaner {
public:
bool do_entry(address key, const SourceObjInfo* value) {
delete value->ref();
return true;
}
};

This comment has been minimized.

@coleenp

coleenp Jun 16, 2021
Author Contributor

I wanted to do that (and even had a version that did), but unfortunately for this code, adding a destructor to SrcObjRef destroys the ref pointer in this scope, so we'd need to have a copy constructor etc to keep the pointer alive.

SourceObjInfo src_info(ref, read_only, follow_mode);

So I thought we should wait until we replace this table to make it better. I was going to add a comment here.

This comment has been minimized.

@iklam

iklam Jun 16, 2021
Member

Sounds good. The current code is a bit clumsy but there's no memory leak anymore. We should eventually move all functionalities of KVHashTable into ResourceHashtable so we don't have 2 different partially working hashtable types!

This comment has been minimized.

@coleenp

coleenp Jun 16, 2021
Author Contributor

Yes, I agree. This src_obj_table will still need some surgery, since the replacement table should call destructors on all the elements so maybe the table shouldn't contain a copy of SourceObjectInfo.

free_entry(probe);
This conversation was marked as resolved by coleenp

This comment has been minimized.

@tstuefe

tstuefe Jun 16, 2021
Member

I tried to understand BasicHashTable::free_entry() and BasicHashTable::unlink_entry(). I may be wrong, but the latter looks wrong. I expected some linked list splicing, but all it does is set the next ptr of the unlinked entry to NULL. So it would orphan follow up entries, and leave a dangling pointer to itself at its chain predecessor. I may be missing something here.

You code still seems to be correct since you manually walk the whole entry chain.

This comment has been minimized.

@coleenp

coleenp Jun 16, 2021
Author Contributor

I hope unlink_entry isn't wrong because it's used for all the BasicHashtables. It does orphan the further entries, but this entry has already been removed from the list. So setting the next isn't really necessary here, but decrementing the number of elements is for printing and logging.

}
}
assert(BasicHashtable<F>::number_of_entries() == 0, "should have removed all entries");
}

V* add(K key, V value) {
unsigned int hash = HASH(key);