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.hega...@oracle.com> wrote: > > On Tue, Apr 18, 2017 at 7:08 AM, Vyom Tewari <vyom.tew...@oracle.com> > wrote: > > ... > > > > Thanks for review, please find the updated webrev( > http://cr.openjdk.java.net/~vtewari/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.stu...@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. > >
