RFR: 8010992: Remove calls to global ::operator new[] and new
Yumin Qi
yumin.qi at oracle.com
Tue Apr 16 15:12:06 PDT 2013
Let me check how many places NEW_C_HEAP_OBJ used in hotspot.
Thanks
Yumin
On 4/16/2013 3:05 PM, Coleen Phillimore wrote:
>
> On 04/16/2013 05:59 PM, Zhengyu Gu wrote:
>> Agree with Coleen. NEW_C_HEAP_OBJ and NEW_C_HEAP_OBJECT are really
>> confusing, maybe NEW_C_HEAP_CLASS_OBJ (?)
>
> Or change the other 2 occurrences of NEW_C_HEAP_OBJ's to
> NEW_C_HEAP_OBJECT (better name) and completely remove NEW_C_HEAP_OBJ.
> Most code uses NEW_C_HEAP_ARRAY() with 1 element to get the
> NEW_C_HEAP_OBJ behavior anyway.
>
> The other uses are structs, they won't change behavior.
>
> Coleen
>
>>
>> Otherwise, it is good.
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>> On 4/16/2013 5:45 PM, Coleen Phillimore wrote:
>>>
>>> Hi Yumin,
>>>
>>> It's a bit confusing that there's both a NEW_C_HEAP_OBJ and a
>>> NEW_C_HEAP_OBJECT. I assume the latter was supposed to be symmetric
>>> with NEW_C_HEAP_OBJECT_ARRAY, but it is never used and sort of
>>> unclear when you'd want to use it. The NEW_C_HEAP_OBJ call in
>>> thread.cpp could use the latter and not need the call to
>>> HandleMark::initialize(). And the change in handles.hpp wouldn't
>>> be needed then either.
>>>
>>> The rest looks good. I see why you didn't use placement delete
>>> because it isn't really supported by C++ (checking some web
>>> searching engine).
>>>
>>> Coleen
>>>
>>>
>>> On 04/16/2013 04:52 PM, Yumin Qi wrote:
>>>> Hi, all
>>>>
>>>> new webrev at
>>>> http://cr.openjdk.java.net/~minqi/8010992/webrev3
>>>>
>>>> Changes: NEW_C_HEAP_ARRAY to create HandleMark in
>>>> Thread::Thread, use FREE_C_HEAP_OBJECT to free it in ~Thread.
>>>> quickSort.cpp, remove conditional includes.
>>>>
>>>> Thanks
>>>> Yumin
>>>>
>>>>
>>>> On 4/16/2013 5:56 AM, David Holmes wrote:
>>>>> On 16/04/2013 10:39 PM, Zhengyu Gu wrote:
>>>>>> On 4/15/2013 7:20 PM, David Holmes wrote:
>>>>>>> Adding back compiler and gc teams
>>>>>>>
>>>>>>> On 16/04/2013 7:48 AM, Yumin Qi wrote:
>>>>>>>> Zhenyu,
>>>>>>>>
>>>>>>>> On 4/15/2013 2:02 PM, Zhengyu Gu wrote:
>>>>>>>>> Maybe you need a replacement delete operator, please check
>>>>>>>>> http://www.cplusplus.com/reference/new/operator%20delete/
>>>>>>>>>
>>>>>>>> i can just do
>>>>>>>>
>>>>>>>> delete ((type *)&array_name[index]);
>>>>>>>>
>>>>>>>> so the destructor will be called, is this right?
>>>>>>>
>>>>>>> No, it will also deallocate the memory unless you use a variant of
>>>>>>> delete (which is what I think Zhengyu was referring to) that
>>>>>>> doesn't
>>>>>>> actually deallocate the memory.
>>>>>>>
>>>>>> Yes, I meant placement delete operator, which takes two pointers.
>>>>>
>>>>> But you can only call that explicitly as a function. The delete
>>>>> expression will never use it, so I don't see how it applies. We
>>>>> need to call the destructor without de-allocating and the only way
>>>>> I can see to do that is to call the destructor explicitly.
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>
>>>>>> -Zhengyu
>>>>>>
>>>>>>
>>>>>>> I think this is getting out of hand - if we go this path then we
>>>>>>> should simply have a custom operator new[] and operator delete[]
>>>>>>> and
>>>>>>> not need NEW_C_HEAP_ARRAY etc. But I don't think our usage is
>>>>>>> simple
>>>>>>> enough to actually do that. So if we need NEW_C_HEAP_OBJ_ARRAY that
>>>>>>> allocates and constructs then we need a FREE_C_HEAP_OBJ_ARRAY that
>>>>>>> destructs and then de-allocates.
>>>>>>>
>>>>>>> I have no issue with calling destructors explicitly.
>>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Yumin
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> -Zhengyu
>>>>>>>>>
>>>>>>>>> On 4/15/2013 2:42 PM, Zhengyu Gu wrote:
>>>>>>>>>> 623 #define FREE_C_HEAP_OBJECT_ARRAY(type, array_name, size,
>>>>>>>>>> memflags) \
>>>>>>>>>> 624 { \
>>>>>>>>>> 625 if (array_name != NULL)
>>>>>>>>>> { \
>>>>>>>>>> 626 for (int index = 0; index < size; index ++)
>>>>>>>>>> { \
>>>>>>>>>> 627 /* placement to call dtor on type
>>>>>>>>>> */ \
>>>>>>>>>> 628 ((type*)&array_name[index])->~type(); \
>>>>>>>>>> 629 } \
>>>>>>>>>> 630 FREE_C_HEAP_ARRAY(type, array_name,
>>>>>>>>>> memflags); \
>>>>>>>>>> 631 } \
>>>>>>>>>> 632 }
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I really don't like this, calling dtor ...
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> -Zhengyu
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 4/15/2013 1:41 PM, Yumin Qi wrote:
>>>>>>>>>>> new webrev at:
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~minqi/8010992/webrev2
>>>>>>>>>>>
>>>>>>>>>>> Can I have a review from GC team for this change?
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Yumin
>>>>>>>>>>>
>>>>>>>>>>> On 4/14/2013 11:07 PM, David Holmes wrote:
>>>>>>>>>>>> Hi Yumin,
>>>>>>>>>>>>
>>>>>>>>>>>> On 13/04/2013 7:07 AM, Yumin Qi wrote:
>>>>>>>>>>>>> After take feedback and modify, new webrev
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~minqi/8010992/webrev1
>>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eminqi/8010992/webrev1>
>>>>>>>>>>>>
>>>>>>>>>>>> I still find the HandleMark changes unsatisfactory. The CHeap
>>>>>>>>>>>> allocated HandleMark in the Thread() constructor is a bit of a
>>>>>>>>>>>> hack. HandleMarks should be stack-allocated. Plus we seem
>>>>>>>>>>>> to leak
>>>>>>>>>>>> that CHeap allocated HandleMark as we don't keep any
>>>>>>>>>>>> pointer to it!
>>>>>>>>>>>> I think this needs to be re-visited, but as a separate CR.
>>>>>>>>>>>>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> allocation.hpp:
>>>>>>>>>>>>
>>>>>>>>>>>> If NEW_C_HEAP_OBJECT_ARRAY invokes the ctor for each array
>>>>>>>>>>>> element
>>>>>>>>>>>> don't we need a new FREE_C_HEAP_OBJECT_ARRAY to invoke the
>>>>>>>>>>>> dtor's
>>>>>>>>>>>> before deallocating ?
>>>>>>>>>>>>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> src/share/vm/utilities/quickSort.cpp
>>>>>>>>>>>>
>>>>>>>>>>>> Why does only Solaris need the allocation headers included
>>>>>>>>>>>> ?? I
>>>>>>>>>>>> would expect all platforms to need to the same headers here.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> David
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>> Yumin
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 4/12/2013 10:45 AM, Zhengyu Gu wrote:
>>>>>>>>>>>>>> .
>>>>>>>>>>>>>>>> cardTableModRefBS.cpp
>>>>>>>>>>>>>>>> #87 and #88, why set_start(NULL) are needed?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This is default constructor does, here just copy that code.
>>>>>>>>>>>>>>> Since we
>>>>>>>>>>>>>>> did not call constructor by using this MACRO. It is a
>>>>>>>>>>>>>>> _ValueObj,
>>>>>>>>>>>>>>> should not call new, but I think we can use
>>>>>>>>>>>>>>> NEW_C_HEAP_OBJ3,
>>>>>>>>>>>>>>> which
>>>>>>>>>>>>>>> will call ctors.
>>>>>>>>>>>>>> NEW_C_HEAP_OBJECT_ARRAY macro does invoke ctor ... that
>>>>>>>>>>>>>> is what
>>>>>>>>>>>>>> allocation.hpp #618 does.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -Zhengyu
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> carTableRS.cpp
>>>>>>>>>>>>>>>> #70, why it is commented out? If so, you don't need
>>>>>>>>>>>>>>>> the dstor
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> See reply to David H.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>> Yumin
>>>>>>>>>>>>>>>> -Zhengyu
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 4/12/2013 2:12 AM, Yumin Qi wrote:
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Can I have your inputs for the fix?
>>>>>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~minqi/8010992/webrev/
>>>>>>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eminqi/8010992/webrev/>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Bug: 8010992: Remove calls to global ::operator
>>>>>>>>>>>>>>>>> new[] and
>>>>>>>>>>>>>>>>> new
>>>>>>>>>>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8010992
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Problem description: Remove the usage of global operator
>>>>>>>>>>>>>>>>> ::new[]
>>>>>>>>>>>>>>>>> and ::new. In hotspot debug build, disable the usage
>>>>>>>>>>>>>>>>> of global
>>>>>>>>>>>>>>>>> new[] and new. Hotspot does not throw c++ exceptions,
>>>>>>>>>>>>>>>>> but it
>>>>>>>>>>>>>>>>> cannot prevent third party code to catch such
>>>>>>>>>>>>>>>>> exceptions. By
>>>>>>>>>>>>>>>>> disabling use of global operator new[] and new, we
>>>>>>>>>>>>>>>>> constrain
>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>> exception disposal within hotspot. C++ classes (as
>>>>>>>>>>>>>>>>> same for
>>>>>>>>>>>>>>>>> structs) in hotspot have to either extends from
>>>>>>>>>>>>>>>>> CHeapObj or
>>>>>>>>>>>>>>>>> ResourceObj unless they are stack objects or values which
>>>>>>>>>>>>>>>>> have to
>>>>>>>>>>>>>>>>> be from StackObj or _ValueObj respectively. Or they
>>>>>>>>>>>>>>>>> have to
>>>>>>>>>>>>>>>>> implement their own operator new[] or new.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>> Yumin
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>>
>
More information about the hotspot-compiler-dev
mailing list