[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR: 8012902: remove use of global operator new - take 2
From:       Yumin Qi <yumin.qi () oracle ! com>
Date:       2013-04-29 17:08:40
Message-ID: 517EA918.30002 () oracle ! com
[Download RAW message or body]

Coleen and David,

On 4/29/2013 6:36 AM, Coleen Phillimore wrote:
>
> David,
>
> I think Yumin has some additional operator new[] calls that he hasn't 
> fixed yet so I'm expecting another webrev.   I agree it's a more 
> complicated change than originally thought, but it's progressing so I 
> don't think quitting now is a good idea.  The main change helps 
> resolve a fundamental problem that has recently broken the VM.  
> Yumin's change makes it harder to do this.
>
Yes, I think if we stop here, this one will not be revisited for long 
time --- more dangerous code will be added then. To prevent this from 
happening, better to stop them as soon as we can.
> On 04/29/2013 09:00 AM, David Holmes wrote:
>> Hi Yumin,
>>
>> I think we need to pull back on this until we can address the broader 
>> issues:
>>
>> a) there are a number of classes that don't obey the rules about 
>> extending one of the allocation types
>
> This will always be the case.   This shouldn't be a blocker.
Those classes (most of them) are used as stack obj, currently did not 
find any used as heap obj. For VALUE_OBJ_CLASS_SPEC, since it is empty 
on linux, every class which take it as parent will come from nothing 
that is similar to those classes not obey the rules --- This is why I 
asked if we should make it _ValueObj on linux but you think that will 
add more bytes to the objects.

>
>>
>> b) adding additional operator new/new[] for explicit C-Heap usage 
>> conflicts with the use of the existing macros/functions documented in 
>> allocation.hpp (I still think I prefer NEW_C_HEAP_OBJ + global 
>> placement new to invoke the correct constructor). If you stick with 
>> your approach then the documentation in allocation.hpp needs rewriting.
>
> I think the documentation in allocation.hpp describes how we want this 
> to work.   The exceptional cases should be documented where they 
> exist.   I don't really have an opinion whether NEW_C_HEAP_OBJ vs. 
> adding new and new[] to the exceptional classes is better.  Both have 
> their pros and cons.
>
Using macro and calling constructors need carefulness, which caused too 
much concern, so if the use case is simple, I would like to use macros, 
but if it is complex, implementing operator new is preferable I think.
>>
>> c) there seem to be other global array allocations still lurking
>>
>
> Yes, the ones we know about should be fixed.   And do due diligence to 
> find them all.  The purpose of the assert is to find any that might 
> leak in after this exercise.

I found one more case using nm on linux. Do you know what it will be on 
solaris? I tried to code a small program, but could not locate 'new' in 
the output. For shared code, linux will output all the unsettled 
operator new, what I am concerning here is some platform specific code.

>
>> d) the effect of the hotspot global operator new on the other 
>> libraries needs to be better understood and dealt with. If I 
>> understand your fix as it stands you will abort in product mode, and 
>> warn in debug - yet we know this problem exists so this will simply 
>> force an abort. I would not expect to see the ShouldNotReachHere() 
>> variants.
>>
>
> I think the logic is reversed in the new code.  #ifndef 
> CATCH_OPERATOR_NEW_USAGE should just revert to the global operators 
> new/new[]/delete/delete[], ie be empty.   The code under CATCH_* 
> should assert and return AllocateHeap() in product quietly. 
> ShouldNotReachHere() gives a fatal error in product mode too, so it 
> should be avoided.
Yes, the logic here now is on macosx (currently I did not find any other 
platform the global operator new switched to jvm 'new'), only gives 
warnings for the operator first time called, and return AllocateHeap, no 
stop here.

BTW, when I tried to test on Windows to find if they will fail on new[] 
(a lot of new[] used in awt, swing etc), the demo did not crash on new[] 
but there is a failure in awt code, Hashtable.cpp



This comes not only my fastdebug version, but all other debug versions 
on Windows. It is an awt related error.  It happens on my desktop 
whenever you type characters in input area of the testing program interface.

Thanks
Yumin

[Attachment #3 (multipart/related)]

[Attachment #5 (text/html)]

<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Coleen and David,<br>
    <br>
    <div class="moz-cite-prefix">On 4/29/2013 6:36 AM, Coleen Phillimore
      wrote:<br>
    </div>
    <blockquote cite="mid:517E7752.9070603@oracle.com" type="cite">
      <br>
      David,
      <br>
      <br>
      I think Yumin has some additional operator new[] calls that he
      hasn't fixed yet so I'm expecting another webrev.   I agree it's a
      more complicated change than originally thought, but it's
      progressing so I don't think quitting now is a good idea.  The
      main change helps resolve a fundamental problem that has recently
      broken the VM.  Yumin's change makes it harder to do this.
      <br>
      <br>
    </blockquote>
    Yes, I think if we stop here, this one will not be revisited for
    long time --- more dangerous code will be added then. To prevent
    this from happening, better to stop them as soon as we can.<br>
    <blockquote cite="mid:517E7752.9070603@oracle.com" type="cite">On
      04/29/2013 09:00 AM, David Holmes wrote:
      <br>
      <blockquote type="cite">Hi Yumin,
        <br>
        <br>
        I think we need to pull back on this until we can address the
        broader issues:
        <br>
        <br>
        a) there are a number of classes that don't obey the rules about
        extending one of the allocation types
        <br>
      </blockquote>
      <br>
      This will always be the case.   This shouldn't be a blocker.</blockquote>
    Those classes (most of them) are used as stack obj, currently did
    not find any used as heap obj. For VALUE_OBJ_CLASS_SPEC, since it is
    empty on linux, every class which take it as parent will come from
    nothing that is similar to those classes not obey the rules --- This
    is why I asked if we should make it _ValueObj on linux but you think
    that will add more bytes to the objects.<br>
    <br>
    <blockquote cite="mid:517E7752.9070603@oracle.com" type="cite">
      <br>
      <blockquote type="cite">
        <br>
        b) adding additional operator new/new[] for explicit C-Heap
        usage conflicts with the use of the existing macros/functions
        documented in allocation.hpp (I still think I prefer
        NEW_C_HEAP_OBJ + global placement new to invoke the correct
        constructor). If you stick with your approach then the
        documentation in allocation.hpp needs rewriting.
        <br>
      </blockquote>
      <br>
      I think the documentation in allocation.hpp describes how we want
      this to work.   The exceptional cases should be documented where
      they exist.   I don't really have an opinion whether
      NEW_C_HEAP_OBJ vs. adding new and new[] to the exceptional classes
      is better.  Both have their pros and cons.
      <br>
      <br>
    </blockquote>
    Using macro and calling constructors need carefulness, which caused
    too much concern, so if the use case is simple, I would like to use
    macros, but if it is complex, implementing operator new is
    preferable I think. <br>
    <blockquote cite="mid:517E7752.9070603@oracle.com" type="cite">
      <blockquote type="cite">
        <br>
        c) there seem to be other global array allocations still lurking
        <br>
        <br>
      </blockquote>
      <br>
      Yes, the ones we know about should be fixed.   And do due
      diligence to find them all.  The purpose of the assert is to find
      any that might leak in after this exercise.
      <br>
    </blockquote>
    <br>
    I found one more case using nm on linux. Do you know what it will be
    on solaris? I tried to code a small program, but could not locate
    'new' in the output. For shared code, linux will output all the
    unsettled operator new, what I am concerning here is some platform
    specific code.<br>
    <br>
    <blockquote cite="mid:517E7752.9070603@oracle.com" type="cite">
      <br>
      <blockquote type="cite">d) the effect of the hotspot global
        operator new on the other libraries needs to be better
        understood and dealt with. If I understand your fix as it stands
        you will abort in product mode, and warn in debug - yet we know
        this problem exists so this will simply force an abort. I would
        not expect to see the ShouldNotReachHere() variants.
        <br>
        <br>
      </blockquote>
      <br>
      I think the logic is reversed in the new code.  #ifndef
      CATCH_OPERATOR_NEW_USAGE should just revert to the global
      operators new/new[]/delete/delete[], ie be empty.   The code under
      CATCH_* should assert and return AllocateHeap() in product
      quietly. ShouldNotReachHere() gives a fatal error in product mode
      too, so it should be avoided.
      <br>
    </blockquote>
    Yes, the logic here now is on macosx (currently I did not find any
    other platform the global operator new switched to jvm 'new'), only
    gives warnings for the operator first time called, and return
    AllocateHeap, no stop here. <br>
    <br>
    BTW, when I tried to test on Windows to find if they will fail on
    new[] (a lot of new[] used in awt, swing etc), the demo did not
    crash on new[] but there is a failure in awt code, Hashtable.cpp<br>
    <br>
    <img src="cid:part1.09090100.07070903@oracle.com" alt=""><br>
    <br>
    This comes not only my fastdebug version, but all other debug
    versions on Windows. It is an awt related error.  It happens on my
    desktop whenever you type characters in input area of the testing
    program interface.<br>
    <br>
    Thanks<br>
    Yumin<br>
  </body>
</html>

["abaijbdd.png" (image/png)]

[prev in list] [next in list] [prev in thread] [next in thread]