RFR: 8012902: remove use of global operator new - take 2

[Coleen Phillimore]

Yumin,

http://cr.openjdk.java.net/~minqi/8012902/webrev1/src/share/vm/memory/memRegion.hpp.cdiff.html
http://cr.openjdk.java.net/~minqi/8012902/webrev1/src/share/vm/memory/memRegion.cpp.cdiff.html
Can you put a comment why you are defining operator new(s) and delete(s) 
in this class, ie. it's an exceptional case.

http://cr.openjdk.java.net/~minqi/8012902/webrev1/src/share/vm/runtime/handles.hpp.cdiff.html
Same for Handles.   Explain that there is a special case where it needs 
to be C heap allocated so it needs these operators.

Thanks,
Coleen

On 05/01/2013 10:56 AM, Yumin Qi wrote:
> Hi,
>
>   This revised version replace  CATCH_OPERATOR_NEW_USAGE with 
> ALLOW_OPERATOR_NEW_USAGE.  For platforms on which operator new called 
> from other source other than jvm, define this macro to enable global 
> operator new instead.
>
>   Also fixed KlassHandle creation in ciReplay.cpp, that still use 
> global operator new.
>
> http://cr.openjdk.java.net/~minqi/8012902/webrev1/ 
> <http://cr.openjdk.java.net/%7Eminqi/8012902/webrev/>
>
>   Tested: vm.quick.testlist, jtreg, runThese, JPRT
>
> Thanks
> Yumin
>
> *
>
>
> *
> On 4/29/2013 10:08 AM, Yumin Qi wrote:
>> Coleen and David,
>>
>> On 4/29/2013 6:36 AM, Coleen Phillimore wrote:
>>>
>>> David,
>>>
>>> I think Yumin has some additional operator new[] calls that he 
>>> hasn't fixed yet so I'm expecting another webrev.   I agree it's a 
>>> more complicated change than originally thought, but it's 
>>> progressing so I don't think quitting now is a good idea.  The main 
>>> change helps resolve a fundamental problem that has recently broken 
>>> the VM.  Yumin's change makes it harder to do this.
>>>
>> Yes, I think if we stop here, this one will not be revisited for long 
>> time --- more dangerous code will be added then. To prevent this from 
>> happening, better to stop them as soon as we can.
>>> On 04/29/2013 09:00 AM, David Holmes wrote:
>>>> Hi Yumin,
>>>>
>>>> I think we need to pull back on this until we can address the 
>>>> broader issues:
>>>>
>>>> a) there are a number of classes that don't obey the rules about 
>>>> extending one of the allocation types
>>>
>>> This will always be the case.   This shouldn't be a blocker.
>> Those classes (most of them) are used as stack obj, currently did not 
>> find any used as heap obj. For VALUE_OBJ_CLASS_SPEC, since it is 
>> empty on linux, every class which take it as parent will come from 
>> nothing that is similar to those classes not obey the rules --- This 
>> is why I asked if we should make it _ValueObj on linux but you think 
>> that will add more bytes to the objects.
>>
>>>
>>>>
>>>> b) adding additional operator new/new[] for explicit C-Heap usage 
>>>> conflicts with the use of the existing macros/functions documented 
>>>> in allocation.hpp (I still think I prefer NEW_C_HEAP_OBJ + global 
>>>> placement new to invoke the correct constructor). If you stick with 
>>>> your approach then the documentation in allocation.hpp needs 
>>>> rewriting.
>>>
>>> I think the documentation in allocation.hpp describes how we want 
>>> this to work.   The exceptional cases should be documented where 
>>> they exist.   I don't really have an opinion whether NEW_C_HEAP_OBJ 
>>> vs. adding new and new[] to the exceptional classes is better.  Both 
>>> have their pros and cons.
>>>
>> Using macro and calling constructors need carefulness, which caused 
>> too much concern, so if the use case is simple, I would like to use 
>> macros, but if it is complex, implementing operator new is preferable 
>> I think.
>>>>
>>>> c) there seem to be other global array allocations still lurking
>>>>
>>>
>>> Yes, the ones we know about should be fixed.   And do due diligence 
>>> to find them all.  The purpose of the assert is to find any that 
>>> might leak in after this exercise.
>>
>> I found one more case using nm on linux. Do you know what it will be 
>> on solaris? I tried to code a small program, but could not locate 
>> 'new' in the output. For shared code, linux will output all the 
>> unsettled operator new, what I am concerning here is some platform 
>> specific code.
>>
>>>
>>>> d) the effect of the hotspot global operator new on the other 
>>>> libraries needs to be better understood and dealt with. If I 
>>>> understand your fix as it stands you will abort in product mode, 
>>>> and warn in debug - yet we know this problem exists so this will 
>>>> simply force an abort. I would not expect to see the 
>>>> ShouldNotReachHere() variants.
>>>>
>>>
>>> I think the logic is reversed in the new code.  #ifndef 
>>> CATCH_OPERATOR_NEW_USAGE should just revert to the global operators 
>>> new/new[]/delete/delete[], ie be empty.   The code under CATCH_* 
>>> should assert and return AllocateHeap() in product quietly. 
>>> ShouldNotReachHere() gives a fatal error in product mode too, so it 
>>> should be avoided.
>> Yes, the logic here now is on macosx (currently I did not find any 
>> other platform the global operator new switched to jvm 'new'), only 
>> gives warnings for the operator first time called, and return 
>> AllocateHeap, no stop here.
>>
>> BTW, when I tried to test on Windows to find if they will fail on 
>> new[] (a lot of new[] used in awt, swing etc), the demo did not crash 
>> on new[] but there is a failure in awt code, Hashtable.cpp
>>
>>
>>
>> This comes not only my fastdebug version, but all other debug 
>> versions on Windows. It is an awt related error.  It happens on my 
>> desktop whenever you type characters in input area of the testing 
>> program interface.
>>
>> Thanks
>> Yumin
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130502/f365d437/attachment-0001.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/png
Size: 27148 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130502/f365d437/attachment-0001.png