Currently GrowableArray is based on ResourceObj class but it is also used for allocation on C heap and an arena. There is very complex verification code during runtime to make sure that GrowableArray object and its array allocations match. We should rework GrowableArray implementation to make it compiling time verification. One suggestion is to use template parameter to specify base class.

Here is discussion for 6993125 fix review:

Coleen Phillimore wrote:
> On 12/10/2010 3:09 PM, Vladimir Kozlov wrote:
>> Thank you, Coleen
>>
>> I thought about introducing GrowableArray factory methods
>> for C_HEAP and Arena allocation to avoid specifying
>> allocation type in noth new() and constructor in allocation
>> site. Also we have 4 types of allocations: cheap, arena,
>> resource area and stack(embedded). But we not specifying
>> RESOURCE_AREA assuming it is default behavior. So we may
>> need to fix this also: always pass allocation type to
>> constructors.
> Not sure why we need to do this.  The 4 types of allocations are 
> represented by the base class type CHeapObj, ValueObj, ResourceObj and 
> <forgot arena obj>.  The only reason we need to pass the allocation type 
> to the constructors is because we're faking allocation from one type to 
> the other.  The constructors shouldn't care.

What I meant is the type passed to a constructor should match the type
passed to new() operator and we don't do this for resource area type:

class MethodComparator {
static GrowableArray<int> *_fwd_jmps;

  GrowableArray<int> fwd_jmps(16);
  _fwd_jmps = &fwd_jmps;

>>
>> We use ResourceObj for C heap allocation not only for
>> GrowableArray so the check works in that cases also.
>> Having multiple inheritance will be major headache,
>> I think. I would prefer to avoid it.
> Multiple inheritance is a headache for the c++ compilers to implement 

Igor Veresov suggested to use templates to specify base class.
I think it is good idea.

> but isn't really.  Where are the other cases where we use ResourceObjs 
> for things that are not GrowableArrays?  This seems wrong too.

ResourceArray in utilities/array.hpp

6994834 showed this case in C1:

IntervalList is ResourceArray:

define_array(IntervalArray, Interval*)
define_stack(IntervalList, IntervalArray)

and it is embedded into arena allocated object:

class CompilationResourceObj ALLOCATION_SUPER_CLASS_SPEC {
 public:
  void* operator new(size_t size) { return Compilation::current()->arena()->Amalloc(size); }
  void* operator new(size_t size, Arena* arena) {
    return arena->Amalloc(size);
  }
  void  operator delete(void* p) {} // nothing to do
};

class Interval : public CompilationResourceObj {
  IntervalList     _split_children;

    parent->_split_children = IntervalList(4);

>>
>> I also thought about tracking allocation places
>> (allocation region boundaries) for all types
>> of allocations. So you can ask where(adr) and
>> it will give you type (C_HEAP, Arena, resource are).
>> We need it to set correct type of an embedded object
>> to the same type as the container object.
>> Currently we do not catch situation when container
>> object is on C heap and embedded GrowableArray
>> has elements on resource area.
>>
> 
> We are planning to implement native memory tracking, which this could be 
> a subset of.   WRT the last sentence, this is true, but we do check that 
> the GrowableArray and element allocations match, or at least we still 

We only strictly check C_HEAP allocation for GrowableArray 
object and its array and when the object is embedded we have 
to do next thing (g1/collectionSetChooser.cpp):

  _markedRegions((ResourceObj::set_allocation_type((address)&_markedRegions,
                                             ResourceObj::C_HEAP),
                  100),
                 true),

For resource and arena allocation we allow GrowableArray object
to be on stack or embedded.

> check that.  How can you have an embedded growable array, or do you mean 
> an embedded growable array pointer?

I mean embedded GrowableArray object not array it points to:

src/share/vm/prims/methodHandleWalk.hpp:

  GrowableArray<SlotState> _outgoing;

  MethodHandleWalker(Handle root, bool for_invokedynamic, TRAPS)
    : _chain(root, THREAD),
      _for_invokedynamic(for_invokedynamic),
      _outgoing(THREAD, 10),

> 
>> I want to push current changes as point fix and leave
>> major rewriting of GrowableArray and ResourceObj
>> for later.
> I don't mind if you check in this bug fix. Can you file a bug for 
> cleaning this up later including some of the data from the review below, 
> and point to this bug fix?

Thanks,
Vladimir

> 
> Thanks,
> Coleen
>>
>> Thanks,
>> Vladimir
>>
>>
>> Coleen Phillimore wrote:
>>>
>>>
>>> On 12/9/2010 5:38 PM, Vladimir Kozlov wrote:
>>>> Resending since nobody responded to this request I sent week ago 
>>>> (lost?).
>>> scared...  I was thinking maybe we should refactor GrowableArray so 
>>> that it's not a ResourceObj that secretly acts as a CHeapObj, which 
>>> is sort of the root cause of these problems.  It seems to have hit a 
>>> breaking point.
>>>
>>> I don't know how to, except for using multiple inheritance.  
>>> GrowableCHeapArray : CHeapObj, etc {};
>>>
>>> Coleen
>>>
>>>>
>>>> Vladimir
>>>>
>>>> http://cr.openjdk.java.net/~kvn/6993125/webrev
>>>>
>>>> Fixed 6993125: runThese crashes with 
>>>> assert(Thread::current()->on_local_stack((address)this))
>>>>
>>>> This code is used to check that allocation space of
>>>> a GrowableArray object is matching allocation space
>>>> of its array. It is also check that operator
>>>> ResourceObj::delete() is called only for C heap
>>>> allocated objects.
>>>> To do that operator ResourceObj::new() stores an
>>>> allocation type into ResourceObj debug field.
>>>> But new() is not called for stack allocated and
>>>> embedded objects and ResourceObj() constructor
>>>> does not know if new() was called.
>>>> So the constructor is trying to guess it by
>>>> looking on _allocation value which could be
>>>> a garbage resembling a valid value.
>>>>
>>>> In this bug case the garbage was a valid value for
>>>> an embedded object and not for a stack allocated
>>>> object this is why the assert is failed.
>>>> In the 6994834 case the garbage was a valid value
>>>> for C heap allocated object and it was embedded
>>>> object (funny fact: garbage value was 0xf1f1f1f1
>>>> which is zap value for malloc memory and the
>>>> embedded object address was 0x0e0e0e0c so that
>>>> ~(0x0e0e0e0c + 0x2) == 0xf1f1f1f1).
>>>>
>>>> The only small solution for this problem I found is
>>>> to add another ResourceObj debug field and set it
>>>> in operator ResourceObj::new().
>>>> I think it should provide much less probability that
>>>> garbage in these two fields together will match
>>>> valid values. Unfortunately the probability is not 0.
>>>> An other solution is totally remove this code or put
>>>> it under a flag and test it only sometimes.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>
>