RFR: 8010992: Remove calls to global ::operator new[] and new
David Holmes
david.holmes at oracle.com
Fri Apr 19 02:10:02 UTC 2013
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,
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-gc-dev
mailing list