RFR (S): G1: Use ArrayAllocator for BitMaps

Tue Jun 18 07:31:55 PDT 2013

Hi Coleen,

Thanks for looking at this!

On 6/18/13 3:21 PM, Coleen Phillimore wrote:
>
> Bengt,
> I had a look at this change also.   I would pick ResourceObj over 
> ValueObj for the ArrayAllocator class.  ValueObj really is embedded 
> and is used very consistently, and ResourceObj is usually used for 
> mixed cases like this.   Is there some complication to using ResourceObj?

Now I'm confused.

I'm fine with switching to ResourceObj, but I thought that was supposed 
to be a way of tagging objects that should be allocated in resource 
areas. Here is the comment in allocation.hpp that I am using as a reference:

// For objects allocated in the resource area (see resourceArea.hpp).
// - ResourceObj
//
// For objects allocated in the C-heap (managed by: free & malloc).
// - CHeapObj
//
// For objects allocated on the stack.
// - StackObj
//
// For embedded objects.
// - ValueObj

Currently ArrayAllocator is always embedded in another object, so I 
figured that David's point was valid that it should be a ValueObj. But 
for the ResourceObj class there is this comment:

//----------------------------------------------------------------------
// Base class for objects allocated in the resource area per default.
// Optionally, objects may be allocated on the C heap with
// new(ResourceObj::C_HEAP) Foo(...) or in an Arena with new (&arena)
// ResourceObj's can be allocated within other objects, but don't use
// new or delete (allocation_type is unknown).  If new is used to allocate,
// use delete to deallocate.

Which, I guess says that a ResourceObj can be used for pretty much 
anything. If that is right, what is the point of having the ResourceObj 
class?

Thanks,
Bengt

>
> Thanks,
> Coleen
>
>
> On 06/18/2013 12:57 AM, David Holmes wrote:
>> Reviewed.
>>
>> Thanks.
>>
>> David
>>
>> On 18/06/2013 2:50 PM, Bengt Rutisson wrote:
>>> On 6/18/13 5:34 AM, David Holmes wrote:
>>>> Seems reasonable to me.
>>>
>>> Thanks, David!
>>>
>>> BTW, I am not sure if you reviewed the whole change or just 
>>> commented on
>>> the StackObj vs. ValueObj issue. Can I list you as a reviewer of the
>>> change?
>>>
>>> Thanks,
>>> Bengt
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 17/06/2013 4:17 PM, Bengt Rutisson wrote:
>>>>>
>>>>> Hi David,
>>>>>
>>>>> On 6/17/13 5:23 AM, David Holmes wrote:
>>>>>> Hi Bengt,
>>>>>>
>>>>>> I don't think it makes sense to embed a StackObj into a ValueObj! 
>>>>>> But
>>>>>> then why is ArrayAllocator a StackObj? The only existing use of it I
>>>>>> can see also embeds it! So seems to me it should be a ValueObj in 
>>>>>> the
>>>>>> first place.
>>>>>
>>>>> Good point. I changed ArrayAllocator to be a ValueObj instead. New
>>>>> webrev here:
>>>>> http://cr.openjdk.java.net/~brutisso/8016556/webrev.01/
>>>>>
>>>>> Here is the diff compared to the last version:
>>>>>
>>>>> diff --git a/src/share/vm/memory/allocation.hpp
>>>>> b/src/share/vm/memory/allocation.hpp
>>>>> --- a/src/share/vm/memory/allocation.hpp
>>>>> +++ b/src/share/vm/memory/allocation.hpp
>>>>> @@ -665,7 +665,7 @@
>>>>>   // is set so that we always use malloc except for Solaris where 
>>>>> we set
>>>>> the
>>>>>   // limit to get mapped memory.
>>>>>   template <class E, MEMFLAGS F>
>>>>> -class ArrayAllocator : public StackObj {
>>>>> +class ArrayAllocator VALUE_OBJ_CLASS_SPEC {
>>>>>     char* _addr;
>>>>>     bool _use_malloc;
>>>>>     size_t _size;
>>>>>
>>>>>
>>>>> Thanks for looking at this!
>>>>> Bengt
>>>>>
>>>>>
>>>>>>
>>>>>> David
>>>>>>
>>>>>> On 14/06/2013 12:53 AM, Bengt Rutisson wrote:
>>>>>>>
>>>>>>> Hi everyone,
>>>>>>>
>>>>>>> Could I have a couple of review for this small change?
>>>>>>> http://cr.openjdk.java.net/~brutisso/8016556/webrev.00/
>>>>>>>
>>>>>>> Sending this request to the broad hotspot dev mailing list since it
>>>>>>> touches code in vm/utilities.
>>>>>>>
>>>>>>> Background:
>>>>>>>
>>>>>>> In the constructor for the ConcurrentMark class in G1 we set up one
>>>>>>> bit
>>>>>>> map per worker thread:
>>>>>>>
>>>>>>>    for (uint i = 0; i < _max_worker_id; ++i) {
>>>>>>> ...
>>>>>>>      _count_card_bitmaps[i] = BitMap(card_bm_size, false);
>>>>>>>      ...
>>>>>>>    }
>>>>>>>
>>>>>>> Each of these bitmaps are malloced, which means that the amount of
>>>>>>> C-heap we require grows with the number of GC worker threads we
>>>>>>> have. On
>>>>>>> a machine with many CPUs we get many worker threads by default. For
>>>>>>> example, on scaaa007 I get 79 GC worker threads. The size of the
>>>>>>> bitmaps
>>>>>>> also scale with the heap size. Since this large machine has a 
>>>>>>> lot of
>>>>>>> memory we get a large default heap as well.
>>>>>>>
>>>>>>> Here is the output from just running java -version with G1 on
>>>>>>> scaaa007:
>>>>>>>
>>>>>>> $ java -d64 -XX:+UseG1GC -XX:+PrintMallocStatistics -version
>>>>>>> java version "1.8.0-ea-fastdebug"
>>>>>>> Java(TM) SE Runtime Environment (build 1.8.0-ea-fastdebug-b92)
>>>>>>> Java HotSpot(TM) 64-Bit Server VM (build 25.0-b34-fastdebug, mixed
>>>>>>> mode)
>>>>>>> allocation stats: 35199 mallocs (221MB), 13989 frees (0MB), 35MB 
>>>>>>> resrc
>>>>>>>
>>>>>>> We malloc 221MB just by starting the VM. Most of the large 
>>>>>>> allocations
>>>>>>> are due to the BitMap allocations. My patch changes the BitMap
>>>>>>> allocations to use the ArrayAllocator instead. That class uses 
>>>>>>> mmap on
>>>>>>> Solaris if the size is larger than 64K.
>>>>>>>
>>>>>>> With this patch the output looks like this:
>>>>>>>
>>>>>>> $ java -d64 -XX:+UseG1GC -XX:+PrintMallocStatistics -version
>>>>>>> java version "1.8.0-ea-fastdebug"
>>>>>>> Java(TM) SE Runtime Environment (build 1.8.0-ea-fastdebug-b93)
>>>>>>> Java HotSpot(TM) 64-Bit Server VM (build
>>>>>>> 25.0-b34-internal-201306130943.brutisso.hs-gc-g1-mmap-fastdebug, 
>>>>>>> mixed
>>>>>>> mode)
>>>>>>> allocation stats: 35217 mallocs (31MB), 14031 frees (0MB), 35MB 
>>>>>>> resrc
>>>>>>>
>>>>>>> We are down to 31MB.
>>>>>>>
>>>>>>> Note that the ArrayAllocator only has this effect on Solaris 
>>>>>>> machines.
>>>>>>> Also note that I have not reduced the total amount of memory, just
>>>>>>> moved
>>>>>>> it from the C-heap to mapped memory.
>>>>>>>
>>>>>>> One complication with the fix is that the BitMap data structures 
>>>>>>> get
>>>>>>> copied around quite a bit. The copies are shallow copies, so we 
>>>>>>> don't
>>>>>>> risk re-doing the allocation. But since I am now embedding an
>>>>>>> ArrayAllocator in the BitMap structure the destructor for copies of
>>>>>>> the
>>>>>>> ArrayAllocator gets called every now and then. The BitMap 
>>>>>>> explicitly
>>>>>>> frees up the allocated memory when it thinks it is necessary. So,
>>>>>>> rather
>>>>>>> than trying to refactor the code to avoid copying I made it
>>>>>>> optional to
>>>>>>> free the allocated memory in the ArrayAllocator desctructor.
>>>>>>>
>>>>>>> I do think it would be good to review the BitMap code. It seems 
>>>>>>> a bit
>>>>>>> fishy that we pass around shallow copies. But I think my current
>>>>>>> change
>>>>>>> keeps the same behavior as before.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Bengt
>>>>>
>>>
>