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

Yumin Qi yumin.qi at oracle.com
Fri May 3 10:15:32 PDT 2013 

Sorry  the link is not right, the right link:
http://cr.openjdk.java.net/~minqi/8012902/webrev2/

Thanks
Yumin
On 5/3/2013 10:08 AM, Yumin Qi wrote:
> 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/1ffdc206/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/1ffdc206/attachment-0001.png

More information about the hotspot-runtime-dev mailing list