RFR: 8252049: Native memory leak in ciMethodData ctor

[Vladimir Ivanov]

On Fri, 27 Nov 2020 09:11:23 GMT, David Holmes <dholmes 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
>
> I can't say this looks pretty. It is also very hard to be able to determine when the mutex was last used and whether this running of the destructor is guaranteed to be safe. What is the lifecycle of the original MethodData?

I agree that it's not pretty. 

I'd like to stress that the destruction happens on the freshly allocated `Mutex` and it is not related/copied from original `MethodData`:
MethodData::MethodData(ciMethodData& data)
  : _extra_data_lock(Mutex::leaf, "unused") {
  _extra_data_lock.~Mutex(); // release allocated resources before zeroing

So, I don't see any evident issues related to concurrent usage (since it shouldn't happen). Original `MethodData` lifecycle is tied to the `Method` it relates to. (But I don't see how it can matter here.) `ciMethodData` is allocated in compiler arena on per-compilarion task basis and is deallocated en masse when compilation finishes.

The alternative fix would be to introduce new ctors on `Mutex` and `os::PlatformMonitor` which omit allocation of native condition variable, but that is more intrusive IMO. Let me know what you prefer.

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

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