Re: RFR [9]: 8034174 Remove use of JVM_* functions from java.net code
On 22 Feb 2014, at 09:33, Alan Bateman wrote: > On 22/02/2014 08:29, Chris Hegarty wrote: >> Interruptible I/O on Solaris has been highly problematic and the long >> standing plan has been to remove it from the JDK. In JDK6 the VM option >> UseVMInterruptibleIO was introduced to allow developers/customers experiment >> with disabling it. In JDK7 the default value of UseVMInterruptibleIO was >> changed to be "false" so that it is disabled by default. It is now finally >> being removed. >> >> This bug tracks changing the native in src/share/native/java/net and >> src/solaris/native/java/net so that the system calls are used directly >> rather than going through the JVM_* functions. >> >> http://cr.openjdk.java.net/~chegar/8034174/webrev.00/webrev/ >> > Thank for you for doing this, it's long over due. > > I've taken a first pass over this and it looks good. For NET_RecvFrom and > other that take a socket address then maybe the len should be a size_t rather > than int (they may have been in the JVM_* function due to difference in the > definitions of size_t, which is no longer a concern here I think). Yes, possibly. This will have to change across all “unix” platforms since there are shared definitions. I’ll see what impact this will have. > One comment on the Windows changes is that it might be clearer if they were > changed to check for SOCKET_ERROR rather than -1 as that matches the Windows > Sockets documentation. Yes, of course. Makes sense. > I also notice that several of the files include jvm.h and I wonder if this is > needed now. I’ll ensure that it is not included anywhere it is not needed, but there are some other usages of JVM_CurrentTimeMillis, etc ( which could also probably be cleaned up ). > In JNI_OnLoad then it uses GetEnv to check the JNI version and I assume this > code can just be removed. Right. We still need to get the JNIEnv, as it is needed further down in this function, so need to call GetEnv. I just changed this to handle errors, but maybe there is a better way? > In net_util.h the removal of JVM_* means it means "like the system > procedures", maybe it should be system calls or system functions. Thanks, I’ll update this to, “like the systems calls”. As always when you touch this native code, where to draw the line between resolving the actual problem and cleaning up! -Chris. > -Alan. > > > > > >
Re: RFR [9]: 8034174 Remove use of JVM_* functions from java.net code
On 22 Feb 2014, at 17:23, Dmitry Samersoff wrote: > Chris, > > Didn't look to windows part. Unix part looks good for me. See also below. > > I'm a bit concerned because of mixing NET_* abstractions and direct call > to OS functions. It might be better to create NET_socket etc. Me too. It is already a mess. System calls should be used directly, unless there is a reason not to do so. > We use NET_GetSockOpt/NET_SetSockOpt in one places and plain os > functions in other ones it have to be unified. If there is no reason to call the NET_ variant, then the system call should be used. > (not to your changes, but as far as you touched it) > > Doing socklen_t n = sizeof(m) it's better cast to socklen_t explicitly - > on some platform socklen_t expands to int but sizeof to unsigned so it > can cause a compiler warning. The code compiles on all platforms warning free, but I understand your concern. I’ll make the change. > It's better to unify check of return value of os functions either as == > -1 or as < 0 I agree. Let me see if I can clean up some of these, but I’m reluctant to do much under this JIRA issue. > 1. net_util.c > Do we really need to check JNI_VERSION ? Nope. Just need the JNIEnv. > 2. Inet4AddressImpl.c > > 73, 335 it's better to use NI_MAXHOST in both places > > 784 optlen should be socklen_t > > 3. Inet6AddressImpl.c > > 73, 143 it's better to use NI_MAXHOST in both places > > 4. net_util_md.c > > 235 gettimeofday is obsoleted and might be not available on all > platforms. So it's better to try clock_gettime first I’ll take a look at these, but if there are any potential behavioural changes, desirable or otherwise, then they should come under a separate JIRA issue. -Chris. > > -Dmitry > > > On 2014-02-22 12:29, Chris Hegarty wrote: >> Interruptible I/O on Solaris has been highly problematic and the long >> standing plan has been to remove it from the JDK. In JDK6 the VM option >> UseVMInterruptibleIO was introduced to allow developers/customers experiment >> with disabling it. In JDK7 the default value of UseVMInterruptibleIO was >> changed to be "false" so that it is disabled by default. It is now finally >> being removed. >> >> This bug tracks changing the native in src/share/native/java/net and >> src/solaris/native/java/net so that the system calls are used directly >> rather than going through the JVM_* functions. >> >> http://cr.openjdk.java.net/~chegar/8034174/webrev.00/webrev/ >> >> -Chris. >> > > > -- > Dmitry Samersoff > Oracle Java development team, Saint Petersburg, Russia > * I would love to change the world, but they won't give me the sources.
Re: RFR [9]: 8034174 Remove use of JVM_* functions from java.net code
Thanks for your comments Bernd. On 22 Feb 2014, at 14:03, Bernd Eckenfels wrote: > Hello, > >> Am 22.02.2014 um 10:33 schrieb Alan Bateman : >> >>> http://cr.openjdk.java.net/~chegar/8034174/webrev.00/webrev/ >> Thank for you for doing this, it's long over due. > > Hm, I actually like to have that JVM_ abstraction layer. Looks like it is now > replaced by NET_ in some parts, do we really want to remove it in others? > (the JVM_IO_ERR beeing an obvious advantage) I see Alan has already responded on this point. > Anyway, things I noticed: > > There are two exact same patches in Inet4AddrImpl to getLocalHostName. One > uses NI_MAXHOST and the other strlen - that should be unified (or use a > abstracted helper anyway?) Right. I’ll change it to use NI_MAXHOST. > 769: There is a comment about not needing NET_connect and then it is used > anyway. D’oh. I’ll fix this. > for Socket() it it tested for fd==-1 for getsockopt it is tested for rc < 0? There is a proliferation of styles in the networking native code. I can cleanup some as part of this issue, but I’m reluctant to do more than that. Others cleanup tasks should be done under a separate JIRA issue. > Inet6AddrImpl uses getsockopt() instead of NET_getsockopt? My preference is to use the system call, unless there is a reason not to. This is just a pass to remove the superfluous dependency on the VM interface. There are many many other cleanups and refactoring needed to bring the networking code into the twenty first century. -Chris > Greetings > Bernd
