RFR (S): 8016556: G1: Use ArrayAllocator for BitMaps

[Bengt Rutisson]

Hi Thomas,

On 6/14/13 2:27 PM, Thomas Schatzl wrote:
> [added the CR number in the subject line]

Thanks! :)

>
> Hi,
>
> On Thu, 2013-06-13 at 16:53 +0200, 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/
>>
> Looks good.

Thanks for looking at this!

>
> A few minor comments which are free to ignore:
>
> - the code in the ArrayAllocator constructor/destructor is now more than
> a line. Some additional spacing between them may help readability.

Good point. Added the line breaks.

>
>> 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.
> - instead of the "free_in_destructor" name I would prefer something like
> "owner"; but this is really just my personal opinion.

I see your point, but I actually prefer "free_in_destructor". To me 
"owner" suggests a bit too much responsibility. With the current 
implementation it is the BitMap that will own the memory and is 
responsible for calling free() on the ArrayAllocator. I'm open for a 
better name than "free_in_destructor" but I can't really come up with one.

>
>> 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.
> I agree about both things :)
:)

Thanks again for looking at this!

Bengt


>
> Thanks,
> Thomas
>
>