RFR: 8010992: Remove calls to global ::operator new[] and new
David Holmes
david.holmes at oracle.com
Wed Apr 17 01:12:43 UTC 2013
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-gc-dev
mailing list