RFR (L): JDK-6843347 Boundary values in some public GC options cause crashes

[Bengt Rutisson]

Hi Thomas,

Looks good!

Bengt

On 5/15/13 9:32 AM, Thomas Schatzl wrote:
> Hi all,
>
>    please review another version of the changes for this CR after Bengt's
> comments.
>
> The difference to the previous is
> - do not try to fix up a MarkSweepAlwaysCompactCount=0 setting on the
> command line (but fail on argument verification later)
> - changed various "unsigned int" to "uint" that are related to
> MarkSweepAlwaysCompactCount handling (psMarkSweep.cpp, space.hpp,
> markSweep.?pp,
> - remove redundant braces/fix spacing in psMarkSweep.cpp
>
> v1 to v2:
> - change the type of MarkSweepAlwaysCompactCount from intx to uintx,
> with related changes.
>
> initial to v1:
> - minimum value for HeapSizePerGCThread is os::vm_page_size()
>
> Bugs.sun
> http://bugs.sun.com/view_bug.do?bug_id=6843347
>
> JBS:
> https://jbs.oracle.com/bugs/browse/JDK-6843347
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/6843347/webrev.3/
>
> Testing:
> jprt
>
> Thanks,
>    Thomas
>
> On Tue, 2013-05-14 at 09:48 +0200, Thomas Schatzl wrote:
>> Hi,
>>
>> On Tue, 2013-05-14 at 08:47 +0200, Bengt Rutisson wrote:
>>> Hi Thomas,
>>>
>>> In arguments.cpp you check for MarkSweepAlwaysCompactCount == 0 and in
>>> that case silently set MarkSweepAlwaysCompactCount = 1. I think I
>>> would prefer not to reset it and let there be an error message when we
>>> hit "verify_min_value(MarkSweepAlwaysCompactCount, 1,
>>> "MarkSweepAlwaysCompactCount");" further down. Otherwise people won't
>>> know that they have made a mistake on the command line if they run
>>> with -XX:MarkSweepAlwaysCompactCount=0.
>> That's fine with me - I added that for backwards compatibility as the
>> use e.g. in psMarkSweepDecorator.cpp also silently set it to this value.
>>
>>> One minor nit:
>>>
>>> In psMarkSweep.cpp you updated one line to be:
>>>
>>> unsigned int count = (maximum_heap_compaction)? 1 :
>>> MarkSweepAlwaysCompactCount;
>>>
>>> I don't think the parenthesis increase readability here, but it would
>>> help with a space before the ?. Also, I am more used to seeing uint
>>> instead of unsigned int, but HotSpot has a lot of occurrences of both
>>> variants, so I leave it up to you to choose. So, to summarize I would
>>> prefer:
>>>
>>> uint count = maximum_heap_compaction ? 1 :
>>> MarkSweepAlwaysCompactCount;
>>>
>> Will fix both issues. I changed the type to uint as I also prefer that.
>> Thanks for your comments.
>>
>> Thomas
>>
>>
>>
>