JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

[Vyom Tewari]

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.tewari at oracle.com 
> <mailto:vyom.tewari at oracle.com>> wrote:
>
>     Hi,
>
>     Please review the code change for below issue.
>
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8165437
>     <https://bugs.openjdk.java.net/browse/JDK-8165437>
>
>     Webrev:
>     http://cr.openjdk.java.net/~vtewari/8165437/webrev0.5/index.html
>     <http://cr.openjdk.java.net/%7Evtewari/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
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20170418/d35aec18/attachment.html>