>> No need for updated webrev.
P.S. I mode the webrev update so it is easy for me to record the changes 😝
Thanks!
Lin
On 2020/4/2, 6:21 PM, "linzang(臧琳)" <linz...@tencent.com> wrote:
Dear David,
Here is the updated webrev :
http://cr.openjdk.java.net/~lzang/8241638/webrev04/
Thanks for your help!
BRs,
Lin
On 2020/4/2, 6:06 PM, "David Holmes" <david.hol...@oracle.com> wrote:
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
> > > >
> >
> >
> >
>
>
>
