RFR (S): 8013791: G1: G1CollectorPolicy::initialize_flags() may set min_alignment > max_alignment

[Bengt Rutisson]

Hi Thomas,

Thanks for looking at this!

On 5/2/13 9:09 PM, Thomas Schatzl wrote:
> Hi again,
>
>    sorry, the email was sent accidentally too early...
>
> On Thu, 2013-05-02 at 21:04 +0200, Thomas Schatzl wrote:
>> Hi,
>>
>> On Thu, 2013-05-02 at 20:16 +0200, Bengt Rutisson wrote:
>>> Hi everyone,
>>>
>>> Could I have a couple of reviews for this small patch?
>>>
>>> http://cr.openjdk.java.net/~brutisso/8013791/webrev.00/
>>>
>>> For G1 it is important that the heap size is aligned by the region size.
>>> The ergonomics will pick CollectorPolicy::max_alignment() for aligning
>>> the heap, but this alignment is based on the card table rather than the
>>> region size.
>>>
>>> This fix makes sure that we pick the larger alignment for the
>>> max_alignment value and it adds the same assert for the min and max
>>> alignments that is available in GenCollectorPolicy::initialize_flags().
>> Is it possible to move the min/max alignment constraint checking into
>> CollectorPolicy::initialize_flags() instead of duplicating the code?
>> It seems that this is a general requirement i.e. all other collectors
>> use a descendant of GenCollectorPolicy.

Good point. I moved the asserts to CollectorPolicy::initialize_flags().

>>
>> I also think a better reproducer sets MaxRAM and MaxRAMFraction to odd
> not "odd" but "specific
>
>> values as this will hit the "right" code paths. E.g.
> java -XX:MaxRAM=155M -XX:MaxRAMFraction=1 -XX:MinRAMFraction=1
> -XX:G1HeapRegionSize=32M -XX:+UseG1GC -version

Good idea! Actually if I use MaxRAM I don't need Max/MinRAMFraction.

New webrev out soon.

Thanks,
Bengt

>
> This should hit the error always, see Arguments::set_heap_size().
>
> The fix itself looks good.
>
> Hth,
> Thomas
>