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

[Jon Masamitsu]

Still looks good.

Jon


On 4/1/2016 8:26 AM, sangheon wrote:
> 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/5a6369eb/attachment.htm>