RFR(S): 8241638: launcher time metrics alway report 1 on Linux when _JAVA_LAUNCHER_DEBUG set(Internet mail)

Thu Apr 2 10:21:41 UTC 2020

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.holmes at 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.holmes at 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.holmes at 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.Bateman at 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(臧琳)" <linzang at tencent.com>
    >      >>     >>> Date: Thursday, March 26, 2020 at 3:13 PM
    >      >>     >>> To: "core-libs-dev at openjdk.java.net" <core-libs-dev at 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
    >      >   >   >
    >      >
    >      >
    >      >
    >      
    >      
    > 