RFR: 8291969: Convert LoaderConstraintsTable to ResourceHashtable [v2]

[Ioi Lam]

On Wed, 17 Aug 2022 16:31:21 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This converts the LoaderConstraintTable to ResourceHashtable.  It's a bit different than the other conversions in that the LoaderConstraintEntry has to be restructured to hold all the loader constraints for that class name, since the search key is the class name.  Also, I replaced the raw arrays with GrowableArray, because it's less code.
>> Tested with tier1-3, and tier 4-7 previously with other changes.  I also verified code paths with some temporary asserts with existing tests, including jck tests.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix include for minimal build.

Overall I find this PR too difficult to review, because you have changed both the hashtable algorithm, and the data structures that are stored into the hashtable. 

Before, most of the logic was in a single class, LoaderConstraintEntry (LoaderConstraintTable was mostly a wrapper to the hashtable storage). In the new code, the logic has been split into LoaderConstraintEntry and LoaderConstraint. It's not clear to me the relationship between these two classes.

I feel that to properly review this PR, I need to repeat what you did and also need to understand how the LoaderConstraintTable works with the rest of the VM.

I think it would be much easier if you can preserve the existing logic and just change the hashtable implementation. We can have a subsequent PR for the remaining clean up.

src/hotspot/share/classfile/loaderConstraints.cpp line 56:

> 54:   }
> 55: 
> 56:   LoaderConstraint() { delete _loaders; }

A typo (should be `~LoaderConstraint` ??)

src/hotspot/share/classfile/loaderConstraints.cpp line 241:

> 239:       name->decrement_refcount();
> 240:       entry.clean_up();
> 241:       return true;

This style of resource management in the hashtable entry is dangerous:
- On line 241 `entry._constraints` is a dangling pointer. You could explicitly set `entry._constraints` to NULL inside `clean_up()`, but that's extra code you need to write, and there's a chance you'd forget about doing it.
- If the table is deleted without you explicitly deleting the entries, you will have a memory leak.

I found the following pattern to be safer and require less work:

- Define a default constructor for LoaderConstraintEntry that zeros all fields
- Never instantiate a local LoaderConstraintEntry value and store that into the table. Instead use this:


LoaderConstraintEntry* p = put_if_absent(key, &created);
if (created) {
   p->initialize(....); // allocate  p->_constraints, etc.
}


- For the rule-of-three: delete the copy constructor and copy assignment operator. You don't need them. This way you can be sure you don't need to handle the copying of _constraints between two LoaderConstraintEntry objects.
- Free _constraints inside the LoaderConstraintEntry destructor.

This way, you know that _constraints is allocated in exactly one place, never copied, and deleted in exactly one place. And you will not have a memory leak.

-------------

PR: https://git.openjdk.org/jdk/pull/9904