RFR(xs): 8150843: [windows] os::getTimesSecs() returns negative values for kernel, user times

[Thomas Stüfe]

On Tue, Mar 1, 2016 at 8:32 AM, David Holmes <david.holmes at oracle.com>
wrote:

> On 1/03/2016 5:18 PM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>> On Mon, Feb 29, 2016 at 10:12 PM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>     Hi Thomas,
>>
>>     On 1/03/2016 3:14 AM, Thomas Stüfe wrote:
>>
>>         Hi all,
>>
>>         please review and sponsor this small fix.
>>
>>         It fixes an error in os::getTimesSecs() for windows which causes
>> the
>>         numbers for kernel- and usertime to be off. This does not cause
>>         an error,
>>         because the sole user of this function, the TraceCpuTime class,
>>         uses this
>>         function to measure twice and calculate the deltas.
>>
>>         I still think it is a good idea to fix this bug - with the fix,
>> the
>>         returned values are based on the process start and will be the
>>         same for all
>>         platforms (the *nix platforms use times()).
>>
>>         bug report: https://bugs.openjdk.java.net/browse/JDK-8150843
>>         webrev:
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8150843-windows-osGetTimesSecs/webrev.00/webrev/
>>
>>
>>     So the error is that kernel and user times are treated as-if they
>>     were absolute times - which they are not.
>>
>>     I still find this somewhat confusing though:
>>
>>       955     jlong user_millis = jlong_from(user_time.dwHighDateTime,
>>     user_time.dwLowDateTime);
>>       956     jlong system_millis =
>>     jlong_from(kernel_time.dwHighDateTime, kernel_time.dwLowDateTime);
>>
>>
>>     as neither of those calculations will give a value that represents
>>     milliseconds. I'd rather see the adjustment to millis occur above
>>     rather than here:
>>
>>     958     *process_user_time = ((double) user_millis) / ((double) (10
>>     * MICROUNITS));
>>       959     *process_system_time = ((double) system_millis) /
>>     ((double) (10 * MICROUNITS));
>>
>>
>> You are right, this is confusing. But if I do the division with a jlong,
>> I loose precision (the code before did too). So, I do now the conversion
>> in one step:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8150843-windows-osGetTimesSecs/webrev.01/webrev/
>>
>> So, the new version not only returns values with the correct offset now,
>> they are also more precise, which actually may benefit GC statistics.
>>
>
> Looks good. You're right those local temporaries are not needed. :)
>
>
Okay, thanks for the review!


>
> Thanks,
> David
>
> Kind Regards, Thomas
>>
>>
>>     Thanks,
>>     David
>>
>>
>>
>>         Kind Regards, Thomas
>>
>>
>>