On 2/04/2020 6:48 pm, linzang(臧琳) wrote:
Hi David,
Thanks to point it out, I have uploaded a new patch at
http://cr.openjdk.java.net/~lzang/8241638/webrev03/
As Alan pointed out this definition is not just for Linux. I would
suggest this comment block:
! * Add CounterGet() implementation for launch time debug on Linux.
! * Use gettimeofday() here since the gethrtime() or clock_gettime()
! * may not be available for all Linux platforms.
! * The potential risk of using gettimeofday() is it can be affected
! * by settimeofday() or adjtime().
! * Choose gettimeofday() here because it is more common on linux and
! * it is better than just return magic numbers for launch time debug.
becomes simply:
* Provide a CounterGet() implementation based on gettimeofday() which
* is universally available, even though it may not be 'high resolution'
* compared to platforms that provide gethrtime() (like Solaris). It is
* also subject to time-of-day changes, but alternatives may not be
* known to be available at either build time or run time.
No need for updated webrev.
Thanks,
David
BRs,
Lin
On 2020/4/2, 2:54 PM, "David Holmes" <david.hol...@oracle.com> wrote:
Hi Lin,
On 31/03/2020 12:39 pm, linzang(臧琳) wrote:
> Hi David, Henry, Alan,
> Thanks a lot for your review.
> I have considered use clock_gettime() first, but I seached the code
and found there is a marco SUPPORTS_CLOCK_MONOTONIC guard the usage of it. Which
makes me think it may not be available under specific situation. So I choosed
gettimeofday.
> Do you think the patch need to be refined to remove HAVE_GETHRTIME
as Alan suggested? Thanks.
Leaving it as-is seems okay. Though the likelihood of anyone taking
advantage of an existing gethrtime() function anywhere other than
Solaris seems near zero. But if the Solaris port deprecation goes ahead
the point will be moot as only the "linux" version will remain.
This comment block needs fixing up:
37 /*
38 * * Add gethrtime() implementation for launch time debug on Linux.
39 * */
Also a nit but the new function can be called whatever you like as you
are defining CounterGet, so calling it gethrtime() is a bit misleading -
I suggest getTimeMicros().
Thanks,
David
> BRs,
> Lin
>
> >On 2020/3/31, 8:05 AM, "David Holmes" <david.hol...@oracle.com> wrote:
>>
>> On 31/03/2020 4:08 am, Henry Jen wrote:
>> > Based on my understanding to gethrtime(), the main benefit is not
to be affected by settimeofday or adjtime. I think it is probably better to use
>> >
>> > clock_gettime(CLOCK_MONOTONIC_RAW, ts);
>> >
>> > which I checked seems to be available on both Linux and Mac.
Haven’t test it though.
>>
>> Not guaranteed to be available - either clock_gettime function or
that
>> particular clock - at build time or runtime. We use a check in the
build
>> system to determine build-time availability for hotspot, and then
use
>> dl_lookup etc at runtime to determine if actually available. We
should
>> be able to get rid of this one day but we checked fairly recently
and
>> there were still some issues.
>
>> gettimeofday is a lot better than returning 1. Otherwise call into
the
>> VM and use JVM_NanoTime.
>>
>> Cheers,
>> David
>> -----
>>
>> > Cheers,
>> > Henry
>> >
>> >> On Mar 30, 2020, at 1:37 AM, Alan Bateman
<alan.bate...@oracle.com> wrote:
>> >>
>> >> On 30/03/2020 03:41, linzang(臧琳) wrote:
>> >>> Dear All,
>> >>> May I ask your help to reivew this tiny patch? Thanks.
>> >>>
>> >>>
>> >>>
>> >>> BRs,
>> >>> Lin
>> >>>
>> >>> From: "linzang(臧琳)" <linz...@tencent.com>
>> >>> Date: Thursday, March 26, 2020 at 3:13 PM
>> >>> To: "core-libs-dev@openjdk.java.net"
<core-libs-dev@openjdk.java.net>
>> >>> Subject: RFR(S): 8241638: launcher time metrics alway report 1
on Linux when _JAVA_LAUNCHER_DEBUG set
>> >>>
>> >>> Dear All,
>> >>> May I ask your help to review this tiny fix?
>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8241638
>> >>> Webrev: http://cr.openjdk.java.net/~lzang/8241638/webrev01/
>> >>> Thanks!
>> >>>
>> >> Using gettimeofday on non-Solaris platforms seems reasonable here.
The comment in the patch suggests Linux but it's other Unix builds too. Also just a minor nit
that the code in java.base uses 4-space indent, not 2. Looking at the patch makes me wondering
if we should remove HAVE_GETHRTIME as it seems to be only used on Solaris >and the launcher
is already using #ifdef __solaris__ in several places. Henry, do you have any comments on this?
> > >>
> > >> -Alan
> > >
>
>
>
