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

[David Holmes]

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. :)

Thanks,
David

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