RFR: 8010992: Remove calls to global ::operator new[] and new
David Holmes
david.holmes at oracle.com
Fri Apr 19 05:46:59 UTC 2013
Ship it! :)
TGIF I need a break :)
Thanks,
David
On 19/04/2013 3:21 PM, Yumin Qi wrote:
>
> 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-gc-dev
mailing list