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

[Yumin Qi]

Coleen,

   Thanks, new webrev:
http://cr.openjdk.java.net/~minqi/8012902/webrev2

Yumin

On 5/2/2013 2:41 PM, Coleen Phillimore wrote:
>
> 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/20130503/64ea25f4/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/20130503/64ea25f4/attachment-0001.png