[9] RFR(M): 8054292 : code comments leak in fastdebug builds

[Vladimir Kozlov]

On 8/28/14 6:28 AM, David Chase wrote:
>
> Revised changes: http://cr.openjdk.java.net/~drchase/8054292/webrev-for-leak-checks.12/
>
> Consistent ifdefs around _defunct
> _code_strings, not _strings
> isNull now is_null
> Added extra assert to assign.

The above changes look good.

> Added placement-new-constructor call in InterpreterCodelet (to allow that extra assert).

This one I am not sure. It could hide potential memory leak when 
constructor overwrite previously stored _strings values. But I agree 
that it is the simplest change to enable the assert. And based on what I 
see in current code it should not cause a leak because we assign default 
NULL to _strings several times until the final store in ~CodeletMark().

In short, I accept current change but, please, file RFE (starter task) 
to clean this code. As I said before strings recording for 
InterpreterCodelet could be done differently (do not do it generally for 
Stub class).

>
> Tested fastdebug and not-debug (calls itself “release”) builds w/ jtreg, fastdebug with PrintAsm enabled.
> Did 64-hour leak check with KitchenSink; any leaks seem to be below the what’s-currently-compiled noise threshold.
>
> Sanity check: is “optimized build” the default if I just run configure?

No, the default is 'product' Hotspot build. I would suggest to build 
'optimized' VM separately (it looks like 'optimized' target for whole 
forest does not work) and test it for leaks.

Thanks,
Vladimir

>
>
> On 2014-08-22, at 9:08 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> On 8/22/14 5:43 PM, David Chase wrote:
>>>
>>> On 2014-08-22, at 6:47 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>
>>>> There is some missmatch of use #ifdef ASSERT and #ifndef PRODUCT.
>>>> Please, build 'optimized' JVM in addition to 'product' and 'debug’.
>>>
>>> Optimized VM is running my matrix-multiplication experiments happily, will also do jtreg, didn’t JPRT also do that for me?
>>> I can certainly do jtreg with the optimized build.
>>
>> Okay.
>>
>>>
>>>> Seems _defunct field should be DEBUG only since it is checked only in assert. It could be NOT_PRODUCT bu in that case set_null_and_invalidate() should clean both fields under #ifndef PRODUCT.
>>>> isNull() will return 'true' in NOT_PRODUCT (optimized VM) when assign() may set _strings.
>>>
>>> John also wants it to be spelled is_null, and I just overlooked it and use JavaConventions.
>>> I was planning to have all the probe methods do as little as possible in PRODUCT.
>>> I agree, _defunct should also be DEBUG (is that the same as ASSERT?) only.
>>
>> Yes:
>>
>> #ifdef ASSERT
>> #define DEBUG_ONLY(code) code
>>
>>>
>>>> An other thing  I don't like in current code is the same name of _strings fields in both CodeStrings and CodeBuffer. It is difficult to follow which one is accessed in a code.
>>>
>>> Do you recommend that I fix that, then?  I also did not like this, but thought it was house style.
>>
>> It is not "house style".
>>
>>> I propose to call the CodeStrings-typed field of CodeBuffer “_codestrings”.
>>
>> Agree.
>>
>>>>> One client of CodeBuffers allocates them without running their constructor,
>>>>> which interfered with enabling the full set of assertions that we might like.
>>>>
>>>> It would be nice to fix it - call constructor in that place. Where it is? You can try to do what we do in growableArray:
>>>> new(&_strings) CodeStrings().
>>>
>>> I’m trying to recreate that failure; it may be Linux-specific (it failed on Linux, but did not fail for me on Mac just now, this seems peculiar and even more unsettling).
>>>
>>
>> Thanks,
>> Vladimir
>