RFR(s): 8152176: Big ParGCCardsPerStrideChunk values can cause overflow for CMS GC

[sangheon]

Hi Tom,

Thanks for reviewing this.

On 04/01/2016 08:03 AM, Tom Benson wrote:
> Hi Sangheon,
> This looks fine to me except for a couple of trivial points in 
> commandLineFlagConstraintsGC.cpp comments.
>
> Typo in "should" :
> + // ParGCCardsPerStrideChunk shoule be
Fixed.

> I think the "However," in this line is not needed, and confusing:
>
> + // from CardTableModRefBSForCTRS::process_stride(). However, 
> ParGCStridesPerThread is already checked
>
> I'd probably say "Note that ParGCStridesPerThread ..." instead.
I like your suggestion and fixed.

webrev:
http://cr.openjdk.java.net/~sangheki/8152176/webrev.02
http://cr.openjdk.java.net/~sangheki/8152176/webrev.02_to_01

Thanks,
Sangheon


>
> Tom
>
>
> On 3/29/2016 6:42 PM, sangheon wrote:
>> Thanks, Jon.
>>
>> Sangheon
>>
>>
>> On 03/29/2016 02:13 PM, Jon Masamitsu wrote:
>>>
>>>
>>> On 3/29/2016 12:07 PM, sangheon wrote:
>>>> Hi Jon,
>>>>
>>>> Thank you for reviewing this.
>>>>
>>>> On 03/29/2016 10:49 AM, Jon Masamitsu wrote:
>>>>> Sangheon,
>>>>>
>>>>> I agree that we don't want to expose 
>>>>> CardTableModRefBS::_last_valid_index
>>>>> but I'd consider making CardTableModRefBS::cards_required() public.
>>>>> That would remove the need to duplicate the calculation and it seems
>>>>> a reasonable question to ask the card table implementation how many
>>>>> cards it is going to require.  I'll let other disagree if they like.
>>>> Good idea and changed to 'public'.
>>>>
>>>>>
>>>>> Add in the message a little more about why the value is too large.
>>>>>
>>>>>
>>>>> 396 CommandLineError::print(verbose,
>>>>> 397 "ParGCCardsPerStrideChunk (" INTX_FORMAT ") "is too large for 
>>>>> the heap size and " "must be "
>>>>> 398 "less than or equal to card table size (" SIZE_FORMAT ")\n",
>>>>> 399 value, card_table_size);
>>>> Fixed.
>>>>
>>>>> I think a little more information here would help future 
>>>>> maintainers so
>>>>> instead of
>>>>> 403 // If n_strides which is used with ParGCCardsPerStrideChunk is 
>>>>> really large, we would face an overflow.
>>>>>
>>>> I updated the comment.
>>>>
>>>>> This statement can overflow?
>>>>>
>>>>> 404 uintx n_strides = ParallelGCThreads * ParGCStridesPerThread;
>>>> No.
>>>> Because above statement is checked not to overflow from 
>>>> ParGCStridesPerThreadConstraintFunc() which is called before this 
>>>> constraint function. (added this explanation as well)
>>>
>>> Ok.
>>>
>>>>
>>>> Updated webrev:
>>>> http://cr.openjdk.java.net/~sangheki/8152176/webrev.01/
>>>> http://cr.openjdk.java.net/~sangheki/8152176/webrev.01_to_00/
>>>
>>> Looks good.
>>>
>>> Jon
>>>
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>>
>>>>> Jon
>>>>>
>>>>> On 3/28/2016 3:50 PM, sangheon wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Could I have some reviews for this change?
>>>>>>
>>>>>> As very large value of ParGCCardsPerStrideChunk flag would result 
>>>>>> in an overflow, we need a constraint function after memory 
>>>>>> initialization. And the function will check the flag to be less 
>>>>>> than of equal to the size of card table and not to make an 
>>>>>> overflow with other stride factors(ParallelGCThreads and 
>>>>>> ParGCStridesPerThread).
>>>>>>
>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8152176
>>>>>> Webrev: http://cr.openjdk.java.net/~sangheki/8152176/webrev.00
>>>>>> Testing: JPRT, all runtime/commandline JTREG tests on all platforms
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>>
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160401/9713b7aa/attachment.htm>