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

[Vyom Tewari]

pushed the 
code(http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/7cdde79d6a46).

Vyom


On Friday 28 April 2017 03:26 AM, Langer, Christoph wrote:
>
> Hi Vyom,
>
> I’ve just got a small formatting remark about the order of includes:
>
> Generally I tried to follow the rule 1. Common os headers, 2. Platform 
> os headers,  3. Jvm/jdk headers, 4. JNI headers in my latest 
> refactorings. So, to keep this up, can you move #include “jvm.h” in 
> the line before #include “net_util.h” in each file? And pull it before 
> the JNI headers. Like, e.g. in net_util_md.c you should move #include 
> "jvm.h" to line 52.
>
> Thanks & Best regards
>
> Christoph
>
> *From:*net-dev [mailto:net-dev-bounces at openjdk.java.net] *On Behalf Of 
> *Vyom Tewari
> *Sent:* Donnerstag, 27. April 2017 06:16
> *To:* Thomas Stüfe <thomas.stuefe at gmail.com>; Chris Hegarty 
> <chris.hegarty at oracle.com>
> *Cc:* net-dev <net-dev at openjdk.java.net>
> *Subject:* Re: JDK10 RFR: 8165437 Evaluate the use of gettimeofday in 
> Networking code
>
> Hi,
>
> please find the updated 
> webrev(http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html 
> <http://cr.openjdk.java.net/%7Evtewari/8165437/webrev0.7/index.html>).
>
> Thanks,
>
> Vyom
>
> On Tuesday 25 April 2017 07:34 PM, Thomas Stüfe wrote:
>
>     Hi Chris, Vyom,
>
>     I have preferences as expressed earlier, but no strong emotions. I
>     can live with the fix as it is now.
>
>     Thanks all, and Kind Regards, Thomas
>
>     On Tue, Apr 25, 2017 at 3:31 PM, Chris Hegarty
>     <chris.hegarty at oracle.com <mailto:chris.hegarty at oracle.com>> wrote:
>
>         > On Tue, Apr 18, 2017 at 7:08 AM, Vyom Tewari
>         <vyom.tewari at oracle.com <mailto:vyom.tewari at oracle.com>> wrote:
>         > ...
>         >
>         > 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>)
>
>         The changes mainly look good to me, just a few comments:
>
>         1) src/java.base/unix/native/libnet/PlainSocketImpl.c
>
>            L235     jlong nanoTimeout = timeout * NET_NSEC_PER_MSEC;
>
>            Can you please move this to the latest block of code that
>         requires it, i.e..
>            just after L327 if (connect_rv != 0) { …
>
>         2) src/java.base/share/native/libnet/net_util.h
>
>            Should these definitions be moved to
>         src/java.base/unix/native/libnet/net_util_md.h?
>
>
>         Regarding JVM_NanoTime being a JVM_LEAF and/or taking a JNIEnv (
>         both of which are, in todays hotspot VM, effectively ignored
>         ), this is a
>         separate issue. I have raised it off list with others from the
>         VM team,
>         without much interest. I will refrain from filing a JIRA issue
>         to track potential
>         changes in the VM for this. Others are welcome to do so, if
>         they wish. I
>         suggest that we simply continue to pass a valid JNIEnv, since
>         it is not
>         much extra effort to do so. ( this can be refactored later, if
>         the VM interface
>         is ever updated ).
>
>
>         > On 24 Apr 2017, at 12:07, Thomas Stüfe
>         <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>> wrote:
>         >
>         > ...
>         > 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?
>
>         I think that it may not be worth added back the simpler
>         variant. It
>         would only be used by PlainDatagramSocketImpl, correct? All other
>         usages would use the variant that accepts the current nano time.
>
>         -Chris.
>

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