RFR: 8252049: Native memory leak in ciMethodData ctor [v3]

[Kim Barrett]

On Fri, 4 Dec 2020 22:32:25 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
>
> Vladimir Ivanov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   MDHeader shouldn't subclass StackObj

Mostly good; a few small suggestions or questions.

src/hotspot/share/ci/ciMethodData.cpp line 227:

> 225:   // total_size = _data_size + _extra_data_size
> 226:   // args_data_limit = data_base + total_size - parameter_data_size
> 227:   assert(sizeof(_orig) % HeapWordSize == 0, "align");

Use static_assert.

src/hotspot/share/ci/ciMethodData.cpp line 40:

> 38: // ciMethodData::ciMethodData
> 39: //
> 40: ciMethodData::ciMethodData(MethodData* md) : ciMetadata(md), _orig() {

For a future cleanup, a third, private, constructor could be added that does all the initialization, and the two public constructors could delegate to it, thereby eliminating a lot of code duplication.

src/hotspot/share/oops/methodData.hpp line 1987:

> 1985:   public:
> 1986:     MDHeader() : _nof_decompiles(0), _nof_overflow_recompiles(0), _nof_overflow_traps(0) {
> 1987:       assert(sizeof(_trap_hist) % sizeof(HeapWord) == 0, "align");

static_assert.  And should be consistent between here and next line as to `sizeof(HeapWord)` vs `HeapWordSize`.

src/hotspot/share/oops/methodData.hpp line 1972:

> 1970:   }; // Public flag values
> 1971: 
> 1972:   class MDHeader {

"header" seems like an odd naming choice for this.  Should the name here be tied to the names in the comment in load_data?

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

Changes requested by kbarrett (Reviewer).

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