Request for reviews (S): 7014874: Incorrect COOPs modes on solaris-{sparcv9, amd64} with ParallelGC

[Y. Srinivas Ramakrishna]

Hi Vladimir, thanks for checking!

On 2/7/2011 12:08 PM, Vladimir Kozlov wrote:
> Ramki,
>
> I looked more on this. Gen collectors align Young and
> Old gens sizes down. Look in collectorPolicy.cpp on methods
> initialize_flags(). They align up MaxHeapSize but if it
> is already specified with good rounding (as in my case)
> then it stays the same:
>
> set_max_heap_byte_size(align_size_up(MaxHeapSize, max_alignment()));

It would seem to me that this should be align_size_down() above for the
case when it's not aligned and we want to stay within the band specified
by [Min, Max]. But I understand that in yr case with an already aligned heap size
spec this would not cause an issue. [The concern i was getting at
comes up from time to time, but we have ended up patching over it in piecemeal
fashion in the past; we should probably just consolidate this into one place and
fix it, but that's a bigger, separate issue deserving its own CR, so should
not block your fix here.]

>
> G1 uses this size without any adjustment (G1CollectedHeap::initialize()):
>
> size_t max_byte_size = collector_policy()->max_heap_byte_size();

That's good.

>
> It seems, only Parallel GC modifies it.

Thanks for checking, and your change looks fine to me for addressing the
immediate problem. Thumbs up from my side, and sorry for my nit-picking! :-)

-- ramki

>
> Thanks,
> Vladimir
>
> Y. Srinivas Ramakrishna wrote:
>> Hi Vladimir --
>>
>> On 1/27/2011 3:10 PM, Vladimir Kozlov wrote:
>>> As Igor explained me only Parallel GC requires boundary
>>> page size alignment due to UseAdaptiveGCBoundary feature.
>>
>> Hmm, I do see a lot of "align_size_up()" done on max_size specs
>> in CollectedHeap::initialize_size_info() and friends,
>> as well as in GenerationSpec::align(). I didn't follow the
>> logic through to see if that will or will not affect the
>> whole heap sizing behaviour wrt the issue
>> reported in the CR. The bug report states the
>> anomalous behaviour with ParallelScavenge heaps,
>> Have you tested G1CollectedHeap and GenCollectedHeap
>> to see if they do the right thing?
>>
>> thanks.
>> -- ramki
>>
>>>
>>> Vladimir
>>>
>>> Y. S. Ramakrishna wrote:
>>>> May be you need a "partition_aligned()" method that
>>>> takes an aligned whole and produces aligned partitions
>>>> thereof given a desired_partitioning request. It would
>>>> seem as though similar logic would need to apply to
>>>> other kinds of heaps... or do they do the right thing
>>>> already? OK, i suppose i should go look, since i am
>>>> supposed to be reviewing, not interviewing ;-)
>>>>
>>>> -- ramki
>>>>
>>>> On 01/27/11 14:49, Vladimir Kozlov wrote:
>>>>> Before rounding YG+OG == total heap size. To keep the same
>>>>> total size I need to round one size up and the other down.
>>>>> I still may not preserver the same total size if it is not
>>>>> rounded to the same or large alignment. But it is fine.
>>>>>
>>>>> I already fixed PG rounding before in 6984368 changes:
>>>>>
>>>>> src/share/vm/memory/collectorPolicy.cpp Tue Sep 14 17:19:35 2010 -0700
>>>>> @@ -32,7 +32,11 @@
>>>>> MaxPermSize = PermSize;
>>>>> }
>>>>> PermSize = MAX2(min_alignment(), align_size_down_(PermSize, min_alignment()));
>>>>> - MaxPermSize = align_size_up(MaxPermSize, max_alignment());
>>>>> + // Don't increase Perm size limit above specified.
>>>>> + MaxPermSize = align_size_down(MaxPermSize, max_alignment());
>>>>> + if (PermSize > MaxPermSize) {
>>>>> + PermSize = MaxPermSize;
>>>>> + }
>>>>>
>>>>> Vladimir
>>>>>
>>>>> Y. S. Ramakrishna wrote:
>>>>>> I don't understand the logic of the patch. Or rather, i do,
>>>>>> but i question if it is complete: why didn't you also align
>>>>>> the max size of YG and PG down in like manner? I think you
>>>>>> should or you would be open to the same issues, no?
>>>>>>
>>>>>> In fact, it seems as though, as a matter of uniform policy, all min's
>>>>>> should align upwards and all max's align downwards wherever (page-)alignment
>>>>>> is sought.
>>>>>>
>>>>>> -- ramki
>>>>>>
>>>>>>
>>>>>> On 01/27/11 14:18, Vladimir Kozlov wrote:
>>>>>>> http://cr.openjdk.java.net/~kvn/7014874/webrev
>>>>>>>
>>>>>>> Fixed 7014874: Incorrect COOPs modes on solaris-{sparcv9,amd64} with ParallelGC
>>>>>>>
>>>>>>> scale_by_NewRatio_aligned() aligns boundary between old
>>>>>>> and young gens to min_alignment() (64K). But code in
>>>>>>> ParallelScavengeHeap::initialize() aligns up both old and
>>>>>>> young gen sizes to large page size.
>>>>>>> As result total heap size could be increased by one large
>>>>>>> page and cross 4gb boundary preventing usage of 32-bit COOPs.
>>>>>>>
>>>>>>> Align old gen size down to keep specified heap size but
>>>>>>> not less than its old gen min size.
>>