RFR (S): G1: Use ArrayAllocator for BitMaps

[David Holmes]

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
>>>
>