RFR: 8010992: Remove calls to global ::operator new[] and new
David Holmes
david.holmes at oracle.com
Tue Apr 16 19:16:26 PDT 2013
Bah! Now hotspot-runtime got left off the cc list. :(
David
On 17/04/2013 11:12 AM, David Holmes wrote:
> On 17/04/2013 7:45 AM, 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 critical difference is the invocation of the default constructor.
>
> I think we have gone done the wrong path here because we really want the
> language to handle correct allocation and initialization (and
> de-allocation, destruction) of arrays. The allocation side seems correct
> but for deallocation I see a couple of problems:
>
> a) you now need to know how big the array is so you can iterate through
> and invoke the destructor for each element. This might not be an issue
> for current usage but may be in general.
>
> b) The destruction logic is incorrect I think:
>
> ((type*)&array_name[index])->~type();
>
> This will only work correctly if the object is exactly of type "type".
> If a subclass has been stored into the array then we will invoke the
> destructor of the baseclass, due to the explicit cast to the base type.
> I don't even think a virtual destructor will be of any help there.
>
> Aside - this comment is not relevant (allocation.hpp):
>
> 637 /* placement to call dtor on type */ \
>
>
>> 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.
>
> I'm not clear on the proposal here. But HandleMark is a PITA!
>
>> 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).
>
> Placement delete doesn't help because it doesn't involve invoking
> destructors.
>
> David
>
>> 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-runtime-dev
mailing list