RFR: 8291969: Convert LoaderConstraintsTable to ResourceHashtable [v2]

[David Holmes]

On Fri, 19 Aug 2022 05:33:57 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix include for minimal build.
>
> 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?

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

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