RFR (S): 8073632: Make auxiliary data structures know their own translation factor

[Stefan Karlsson]

On 2015-04-23 10:55, Thomas Schatzl wrote:
> Hi Stefan,
>
> On Wed, 2015-04-22 at 13:25 +0200, Stefan Karlsson wrote:
>> On 2015-04-22 11:02, Thomas Schatzl wrote:
>>> Hi Stefan,
>>>
>>> On Wed, 2015-04-22 at 10:21 +0200, Stefan Karlsson wrote:
>>>> Hi Thomas,
>>>>
>>>> On 2015-04-22 09:56, Thomas Schatzl wrote:
>>>>>>      can I have reviews for this change that cleans up recent changes to
>>>>>> memory management in G1. In particular, for every class that represents
>>>>>> an auxiliary data structure in G1 it adds (and then uses) a specific
>>>>>> function that returns the "heap mapping factor" (the amount of bytes a
>>>>>> particular byte of that data structure corresponds in the java heap).
>>>>>> This method is then used instead of gathering it from various places
>>>>>> when allocating that data structure.
>>>>>>
>>>>>> CR:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8073632
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~tschatzl/8073632/webrev/
>>>> http://cr.openjdk.java.net/~tschatzl/8073632/webrev/src/share/vm/gc_implementation/g1/g1SATBCardTableModRefBS.hpp.frames.html
>>>>
>>>> Why is this implemented as a call into G1BlockOffsetSharedArray?
>>>>
>>>>     157   // Returns how many bytes of the heap a single byte of the Card
>>>> Table corresponds to.
>>>>     158   static size_t heap_map_factor() {
>>>>     159     return G1BlockOffsetSharedArray::heap_map_factor();
>>>>     160   }
>>> You are correct, that is unnecessary. It is better to use
>>> CardTableModRefBS::card_size.
>>>
>>> Fixed in
>>> http://cr.openjdk.java.net/~tschatzl/8073632/webrev.1
>>> Diff webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8073632/webrev.0_to_1
>> That looks better.
>>
>> It still seems weird that the card_count is set up using
>> G1BlockOffsetSharedArray:
>>
>>      G1RegionToSpaceMapper* card_counts_storage =
>>        create_aux_memory_mapper("Card counts table",
>> G1BlockOffsetSharedArray::compute_size(g1_rs.size() / HeapWordSize),
>> -                             G1BlockOffsetSharedArray::N_bytes);
>> + G1BlockOffsetSharedArray::heap_map_factor());
>>
>> I think the "Card counts table" needs its own functions as well.
> For abstraction purposes, I agree. Internally these functions will just
> forward to the card table one. The Card table(, BOT) and the card counts
> table were intentionally made to have the same granularity, so I think
> this is reasonable to reuse its methods internally.
>
> Fixed in
> http://cr.openjdk.java.net/~tschatzl/8073632/webrev.2
> Diff webrev:
> http://cr.openjdk.java.net/~tschatzl/8073632/webrev.1_to_2
>
> I put the implementations in the cpp file because I do not see a
> performance problem for them.

Looks good.

Thanks,
StefanK

>
> Thanks,
>    Thomas
>
>