Hi,
Even i thought the same, pass nanosecond timeout to "NET_Timeout" but if
you see the implementation it uses " *int poll(struct pollfd **/fds/*,
nfds_t */nfds/*, int */timeout/*);*
" where timeout is in millisecond so we have to do conversion anyway in
loop if we pass timeout in NS. So there will not be much difference,
will save just one conversion.
thanks,
Vyom
On Thursday 27 April 2017 04:58 PM, Bernd Eckenfels wrote:
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-boun...@openjdk.java.net
<mailto:net-dev-boun...@openjdk.java.net>> on behalf of Thomas Stüfe
<thomas.stu...@gmail.com <mailto:thomas.stu...@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.tew...@oracle.com
<mailto: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
<http://cr.openjdk.java.net/%7Evtewari/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 <mailto:vyom.tew...@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
