RFR: 8252049: Native memory leak in ciMethodData ctor

[Kim Barrett]

On Fri, 27 Nov 2020 08:05:09 GMT, Vladimir Ivanov <vlivanov 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 agree this fixes the immediate problem of a native memory leak.

But it all seems pretty horrible from a code understanding and maintenance
POV. So while I'm approving this change, I think there is followup work to
be done, for which an RFE should be filed.

For the specific case of ciMethodData::_orig it seems like it would be
better to separate the mutex from the data and have _orig be just the data.
That might take some significant refactoring.

But more generally, I question the embedded mutex in an MDO.

>From the quoted email
(https://mail.openjdk.java.net/pipermail/hotspot-dev/2019-October/039783.html)
Coleen tried heap managing the mutex rather than having it embedded
directly, and found this caused some performace regressions.

But does there really need to be a mutex per MDO? Would a single immortal
mutex, shared by all MDOs, be adequate for this purpose? If not, maybe the
trick used by ObjectMonitor(?) -- have a set of immortal mutexes and
"randomly" choose one per MDO. This would also eliminate this part of the
walk of metadata objects to release heap resources when dropping them. And
it avoids constructing and destructing lots of mutexes (which isn't free,
even if not heap managed).

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

Marked as reviewed by kbarrett (Reviewer).

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