RFR: 8010992: Remove calls to global ::operator new[] and new

Yumin Qi yumin.qi at oracle.com
Thu Apr 18 22:21:28 PDT 2013


On 4/18/2013 7:10 PM, David Holmes wrote:
> On 18/04/2013 5:53 PM, Yumin Qi wrote:
>> Thanks Coleen and David, I update the webrev (same URL) for
>> HandleMark(handles.hpp) and also several files' Copyright year.
>
> Thanks for that.
>
> One thing that just occurred to me is that NEW_C_HEAP_OBJ can only be 
> used for classes that have no virtual methods - as there is nothing to 
> initialize the vtable pointer. I think we got lucky with HandleMark in 
> that regard, but there should be a warning attached to it as well.
>
Thanks! I tried, it will segment fault!

+// allocate type in heap without calling ctor
+// WARNING: type must not have virtual functions!!! There is no way to 
initialize vtable.
  #define NEW_C_HEAP_OBJ(type, memflags)\
    NEW_C_HEAP_ARRAY(type, 1, memflags)

Same link update.

Thanks
Yumin
> Thanks,
> David
>
>> Thanks
>> Yumin
>>
>> On 4/17/2013 7:52 PM, David Holmes wrote:
>>> Hi Yumin,
>>>
>>> Only thing missing - as discussed - is that HandleMark should now
>>> extend StackObj, with a comment about the special CHeap HandleMark in
>>> thread.
>>>
>>> Thanks for your perseverance on this.
>>>
>>> David
>>>
>>> On 18/04/2013 3:54 AM, Yumin Qi wrote:
>>>> Hi,  This version Change back the HandleMark creating in Thread 
>>>> creation
>>>> back to previous version and add more comments to allocation.hpp for
>>>> documenting the usage of newly introduced macros.
>>>>
>>>> http://cr.openjdk.java.net/~minqi/8010992/webrev3
>>>>
>>>> Thanks
>>>> Yumin
>>>>
>>>> On 4/16/2013 1: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