RFR: 8252049: Native memory leak in ciMethodData ctor

[Coleen Phillimore]

On Mon, 30 Nov 2020 13:39:53 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> `ciMethodData` embeds `MethodData` to snapshot a state from the original `MethodData` instance.
>> But `MethodData` embeds a `Mutex` which allocates a platform-specific implementation on C-heap.
>> The `Mutex` is overwritten with `0`s, but the resources aren't deallocated, so the leak occurs.
>> 
>> Proposed fix is to run Mutex destructor right away. 
>> 
>> Initially, I thought about switching to `Mutex*`, but then I found that Coleen already tried that and observed a performance regression [1]. So, for now I chose the conservative approach.
>> 
>> In the longer term, I would consider replacing `MethodData::_extra_data_lock` with a lock-free scheme. Having a lock-per-MDO looks kind of excessive.
>> 
>> Testing:
>> - [x] verified that no memory leak observed with the reported test
>> - [x] tier1-4
>> 
>> [1] https://mail.openjdk.java.net/pipermail/hotspot-dev/2019-October/039783.html
>
> src/hotspot/share/oops/methodData.cpp line 1208:
> 
>> 1206: MethodData::MethodData(ciMethodData& data)
>> 1207:   : _extra_data_lock(Mutex::leaf, "unused") {
>> 1208:   _extra_data_lock.~Mutex(); // release allocated resources before zeroing
> 
> So if _extra_data_lock was a pointer to Mutex, then this statement would be simply "delete _extra_data_lock;" instead, but we'd still have this copy constructor, right?

Why isn't this a regular copy constructor taking MethodData, and pass this._orig instead and leave the zeroing in the caller?  I don't like that oops/methodData knows about ciMethodData.

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

PR: https://git.openjdk.java.net/jdk/pull/1478