RFR: 8146399: Refactor the BlockOffsetTable classes.

David Lindholm david.lindholm at oracle.com
Thu Jan 7 11:27:46 UTC 2016


Hi Mikael,

Thanks for looking at this! I have fixed your comments:

http://cr.openjdk.java.net/~david/JDK-8146399/webrev.01/


Thanks,
David

On 2016-01-07 11:08, Mikael Gerdin wrote:
> Hi David,
>
> Overall a nice cleanup!
> Good stuff!
> I have some small comments, see below.
>
> On 2016-01-04 15:07, David Lindholm wrote:
>> Hi,
>>
>> Please review the following patch which refactors the Block Offset Table
>> classes in G1.
>>
>> Right now there are 3 classes which are responsible for a part of the
>> Block Offset Table. These are G1BlockOffsetArrayContigSpace, which
>> inherits from G1BlockOffsetArray which inherits from G1BlockOffsetTable.
>> I have merged these 3 classes into a new one called
>> G1BlockOffsetTablePart, removing some dead code.
>>
>> The class responsible for the whole BOT has been renamed from
>> G1BlockOffsetSharedArray to G1BlockOffsetTable.
>>
>> Also, the class called G1OffsetTableContigSpace has been renamed to
>> G1ContiguousSpace.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8146399
>> Webrev: http://cr.openjdk.java.net/~david/JDK-8146399/webrev.00/
>
> g1BlockOffsetTable.hpp:
>
> In the comment before G1BlockOffsetTable it says:
> // Each G1BlockOffsetTablePart is owned by a Space.
> Should that be narrowed to say that it's owned by a G1ContiguousSpace?
>
>
> heapRegion.cpp:
>
> in HeapRegion::HeapRegion
>                        G1BlockOffsetTable* sharedOffsetArray,
> while you're changing the type of the parameter, can you also rename 
> it to match the snake_case style of the surrounding code?
>
> in G1ContiguousSpace::G1ContiguousSpace
> Can you remove the newline after ::?
>
>
> heapRegionRemSet.cpp:
>
> in HeapRegionRemSet::print
> Can you change the line you edited
> G1CollectedHeap::heap()->bot()->address_for_index(card_index);
> to use _bot instead of getting it through the heap?
>
> /Mikael
>
>>
>> Testing: JPRT and vm.gc testlist.
>>
>>
>> Thanks,
>> David
>




More information about the hotspot-gc-dev mailing list