JDK10 RFR: 8165437 Evaluate the use of gettimeofday in Networking code
Vyom Tewari
vyom.tewari at oracle.com
Thu Apr 27 13:21:27 UTC 2017
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-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
> <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.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/20170427/139f384a/attachment-0001.html>
More information about the net-dev
mailing list