RFR: 8291969: Convert LoaderConstraintsTable to ResourceHashtable [v2]

[Coleen Phillimore]

On Mon, 22 Aug 2022 07:37:43 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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.
>
> @iklam why is this pattern good? Why can't we create a properly initialized entry and add it to the map, deleting it if there was already one present? Doesn't this expose default initialized instances?

There is a lock around all accesses to this table, so a partially constructed entry will never be exposed.

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

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