Hi Vyom, sorry for the late response, I had vacation.
Thanks for taking my suggestions! Here some remarks: --- I looked a little bit closer into the question why JVM_LEAF is used to wrap simple little functions like JVM_NanoTime or JVM_CurrentTimeMillis (among others). There is no hard technical reason why a function could not just be exported from the libjvm.so using JNIEXPORT, in any form one wishes too. (In fact, we do this in our jvm port a lot). However, JVM_NanoTime() and JVM_CurrentTimeMillis() are used as direct native implementations for System.currentTimeMillis() and System.nanoTime() (see share/native/libjava/System.c), using RegisterNatives(). So, the original author saved the glue functions he would have to write otherwise (Java_java_lang_System_currentTimeMillis etc). The comments in System.c indicate that this was done for performance reasons (you save one call frame by calling JVM_NanoTime directly). Because they are used as direct JNI implementations for static java methods in System, JVM_NanoTime() and JVM_CurrentTimeMillis() have to carry around JNIEnv* and jclass parameters. But they are ignored by the functions - the jclass argument is even called "ignored" in jvm.h. And it seems to be perfectly okay to call JVM_CurrentTimeMillis() with NULL as JNIEnv* argument, which is done in many places in the jdk. Presumably this would be okay too for JVM_NanoTime(), so you could at least avoid the new JNIEnv* argument in NET_Timeout and just call JVM_NanoTime with NULL as first parameter. ----- That aside, I am not a big fan of the removal of the old NET_Timeout. Before, there were two functions, "NET_Timeout" just taking the timeout value, "NET_Timeout0" taking a timeout and a start value. You removed the first variant and therefore added the many additional JVM_NanoTime() calls at NET_Timeout callsites. This makes the code harder to read and also kind of exposes the internal implementation of NET_Timeout (namely the fact that NET_Timeout uses JVM_NanoTime for time measurement). Could you please let both variants live, optionally with better names? ----- Otherwise, I think the change looks good. Thanks for your patience! Kind Regards, Thomas On Tue, Apr 18, 2017 at 7:08 AM, Vyom Tewari <vyom.tew...@oracle.com> wrote: > Hi Thomas, > > Thanks for review, please find the updated webrev(http://cr.openjdk.java. > net/~vtewari/8165437/webrev0.6/index.html) i incorporated all the review > comments. Regarding why JVM_NanoTime is defined JVM_LEAF i don't know much, > but all the functions which are defined in jvm.h used some sort of > macro(JVM_LEAF,JVM_ENTRY,JVM_ENTRY_NO_ENV etc). You can not call > "os::javaTimeNanos()" directly as this is not visible, i did a small > prototype which simply export this without any JVM macro as below. > > This prototype did worked but i am not sure if this is right way to do. I > need some more input, why all jvm.h functions are defined with macro and if > it OK to defined function as below, maybe some one from JVM team can give > some more comment on this. I decided not to include this prototype as part > of review because i am not sure if this is right way. > > Thanks, > > Vyom > > ########################################## > > diff -r 26d689c621f6 make/symbols/symbols-unix > --- a/make/symbols/symbols-unix Wed Apr 12 19:28:47 2017 -0700 > +++ b/make/symbols/symbols-unix Tue Apr 18 08:40:25 2017 +0530 > @@ -51,6 +51,7 @@ > JVM_CurrentLoadedClass > JVM_CurrentThread > JVM_CurrentTimeMillis > +JVM_CurrentTimeNano > JVM_DefineClass > JVM_DefineClassWithSource > JVM_DesiredAssertionStatus > diff -r 26d689c621f6 src/share/vm/prims/jvm.cpp > --- a/src/share/vm/prims/jvm.cpp Wed Apr 12 19:28:47 2017 -0700 > +++ b/src/share/vm/prims/jvm.cpp Tue Apr 18 08:40:25 2017 +0530 > @@ -273,7 +273,11 @@ > JVMWrapper("JVM_NanoTime"); > return os::javaTimeNanos(); > JVM_END > - > + > +long JVM_CurrentTimeNano(){ > + return os::javaTimeNanos(); > +} > + > // The function below is actually exposed by jdk.internal.misc.VM and not > // java.lang.System, but we choose to keep it here so that it stays next > // to JVM_CurrentTimeMillis and JVM_NanoTime > diff -r 26d689c621f6 src/share/vm/prims/jvm.h > --- a/src/share/vm/prims/jvm.h Wed Apr 12 19:28:47 2017 -0700 > +++ b/src/share/vm/prims/jvm.h Tue Apr 18 08:40:25 2017 +0530 > @@ -119,6 +119,9 @@ > JNIEXPORT jlong JNICALL > JVM_NanoTime(JNIEnv *env, jclass ignored); > > +JNIEXPORT jlong > +JVM_CurrentTimeNano(); > + > JNIEXPORT jlong JNICALL > JVM_GetNanoTimeAdjustment(JNIEnv *env, jclass ignored, jlong > offset_secs); > ###################################################### > > On Thursday 13 April 2017 12:40 PM, Thomas Stüfe wrote: > > Hi Vyom, > > Thank you for fixing this! > > In addition to Rogers remarks: > > aix_close.c: > Could you please also update the SAP copyright? > > style nit: > + //nanoTimeout has to be >= 1 millisecond to iterate again. > I thought we use old C style comments? Could you please leave a space > between comment and text? > > net_util.h: It could be more readable if you used the "1000 * 1000..." > notation to define the constants. > > -- > > Apart from this, I have some small reservations about JVM_NanoTime: I see > that JVM_NanoTime is defined using the JVM_LEAF macro. I am not sure why > this is necessary. It has a number of disadvantages: > > - we need to hand in JVMEnv*, which is not necessary for the time > measurement itself (which is a simple os::javaTimeNanos() call). This > requires us to funnel the JVMEnv* thru to NET_Timeout, which makes the > caller code more complicated. > > - JVM_LEAF does a number of verifications in the debug VM, which is not > ideal for a time measure function > > - Also, it blocks on VM exit. Probably not a problem, but a cause for > potential problems. > > os::javaTimeNanos is a simple time measuring function which does not > depend on any internal VM state, as far as I see... so, I do not think it > is necessary to wrap it with JVM_LEAF, no? > > Kind Regards, Thomas > > > On Tue, Apr 11, 2017 at 3:04 PM, Vyom Tewari <vyom.tew...@oracle.com> > wrote: > >> Hi, >> >> Please review the code change for below issue. >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8165437 >> >> Webrev: http://cr.openjdk.java.net/~vtewari/8165437/webrev0.5/index.html >> >> This change will replace the "gettimeofday" to use the monotonic >> increasing clock(i used the existing function "JVM_NanoTime" instead of >> writing my own). >> >> Thanks, >> >> Vyom >> >> >> > >
