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

[Coleen Phillimore]

I think I hit send too soon.

http://cr.openjdk.java.net/~minqi/8012902/webrev1/src/share/vm/runtim/unhandledOops.hpp.cdiff.html

I think this should not be mtOther, but mtThread since this belongs to 
the thread.

Other than these small things, I think it looks good.

Thanks,
Coleen


On 05/01/2013 11:20 AM, Yumin Qi wrote:
> Hi,  the link in fact directs to old webrev, you need to copy the text 
> in browser to open the new link, or follow
>
> http://cr.openjdk.java.net/~minqi/8012902/webrev1/
>
> Thanks
> Yumin
>
> On 5/1/2013 7: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/f8d86e12/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/f8d86e12/attachment-0001.png