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-compiler-dev
mailing list