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
<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
