RFR(XXS): 8029091: Bug in calculation of code cache sweeping interval

[Albert Noll]

Thanks for the review Vladimir.

Best,
Albert

Von meinem iPhone gesendet

> Am 25.11.2013 um 20:53 schrieb Vladimir Kozlov <vladimir.kozlov at oracle.com>:
> 
>> On 11/25/13 11:27 AM, Albert Noll wrote:
>> Hi Vladimir,
>> 
>> Thanks for looking at this. The current patch casts the unsigned value (ReservedCodeCacheSize) to a signed value. That should be fine, since the result will always be small enough to fit into an int.
> 
> Yes, it is true.
> 
>> 
>> This seems clearer to me than depending on ordering and implicit type casts.
> 
> Okay.
> 
> Thanks,
> Vladimir
> 
>> 
>> Best,
>> Albert
>> 
>> Von meinem iPhone gesendet
>> 
>>> Am 25.11.2013 um 18:58 schrieb Vladimir Kozlov <vladimir.kozlov at oracle.com>:
>>> 
>>> Sorry, it is Monday morning.
>>> 
>>> CodeCache::reverse_free_ratio() returns double. Then why calculation is not done in doubles in original code?
>>> Can you just swap time_since_last_sweep and CodeCache::reverse_free_ratio() if it is order dependent?
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>>> On 11/25/13 9:47 AM, Vladimir Kozlov wrote:
>>>> On other hand ReservedCodeCacheSize can't be big since there is 2Gb limit. So it is small int value.
>>>> That in mind the fix is good.
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>>> On 11/25/13 9:33 AM, Vladimir Kozlov wrote:
>>>>> Albert,
>>>>> 
>>>>> What if you use double type for calculations?:
>>>>> 
>>>>> +     const double max_wait_time = (double)ReservedCodeCacheSize / (16 * M);
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>>> On 11/25/13 5:27 AM, Albert Noll wrote:
>>>>>> Hi all,
>>>>>> 
>>>>>> could I have reviews for this small patch?
>>>>>> 
>>>>>> Bug: _
>>>>>> _https://bugs.openjdk.java.net/browse/JDK-8029091
>>>>>> 
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~anoll/8029091/webrev.00/
>>>>>> 
>>>>>> Problem:
>>>>>> The calculation of 'wait_until_next_sweep' in 'NMethodSweeper::possibly_sweep() {}' has a signed
>>>>>> to unsigned conversion bug. The calculation is currently done as follows:
>>>>>> 
>>>>>> double wait_until_next_sweep = (ReservedCodeCacheSize / (16 * M)) - time_since_last_sweep -
>>>>>> CodeCache::reverse_free_ratio();
>>>>>> 
>>>>>> Since the type of 'ReservedCodeCacheSize' (uintx) has a higher rank than 'time_since_last_sleep' (int) and
>>>>>> 'time_since_last_sweep' can be larger than ' (ReservedCodeCacheSize / (16 * M))' there is an underflow. Consequently,
>>>>>> 'wait_until_next_sweep' is assigned a wrong value, which disables the intended periodic code cache sweeps.
>>>>>> 
>>>>>> Solution:
>>>>>> Use only signed types for the calculation of 'wait_until_next_sweep'. Furthermore, an assert checks that
>>>>>> 'wait_until_next_sweep' is never larger than ' (ReservedCodeCacheSize / (16 * M))', which is the
>>>>>> maximum time between two sweeps.
>>>>>> 
>>>>>> 
>>>>>> Many thanks in advance,
>>>>>> Albert