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

[Albert Noll]

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.

This seems clearer to me than depending on ordering and implicit type casts.

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