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

Thu Apr 27 11:28:54 UTC 2017

Hello,

It looks to me like using nanoseconds in the NET_Timeout Timeout Parameter would remove quite a few conversions. Callsides mostly already have timeoutNanoseconds for calculating reminder.

Gruss
Bernd
--
http://bernd.eckenfels.net

________________________________
From: net-dev <net-dev-bounces at openjdk.java.net<mailto:net-dev-bounces at openjdk.java.net>> on behalf of Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
Sent: Monday, April 24, 2017 1:07:52 PM
To: Vyom Tewari
Cc: net-dev
Subject: Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code

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.tewari at oracle.com<mailto:vyom.tewari at 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.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

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/20170427/f606662d/attachment-0001.html>
