Re: RFR[9](M): 8035949 : Remove unused macro USE_SELECT and clean up Unix version of net_util_md.{c,h}
Volker,
The changes look fine to me, and in line with what I was thinking too.
Quite trivially you can remove the block '{' from net_util_md.c Line
1637 and 1650. Also, the function comment says "Wrapper for select/poll
..." ( can remove 'select' ).
I ran your patch through our internal build and test system, and all
looks good.
Thanks,
-Chris.
On 27/02/14 16:46, Volker Simonis wrote:
Hi,
could you please review the following clean-up change (for a better
formatted description please see the webrev):
http://cr.openjdk.java.net/~simonis/webrevs/8035949/
https://bugs.openjdk.java.net/browse/JDK-8035949
This change removes some unused code and macros from
src/solaris/native/java/net/net_util_md.{c,h} and its dependent files.
It also fixes a small bug which was introduced by 7112670
Inet4AddressImpl should use getaddrinfo/getnameinfo after which we
didn't call gai_strerror() any more.
The change has been build and tested (with the jdk JTreg regression
tests) on Linux/x86_64/ppc64, AIX/ppc64, Solaris/sparcv9 and MacOS X
but I won't be upset if somebody wants to run this through JPRT or any
other automatic test suite :)
Remove USE_SELECT macro.
The macro isn't defined in any Makefile so it can never be defined in
the actual build. So I simply removed all preprocessor tests for
USE_SELECT by:
- unconditionally making the code available which was guarded by
#ifndef USE_SELECT
- removing all code which was guarded by #ifdef USE_SELECT.
- in src/solaris/native/java/net/solaris_close.c I removed the
definition of NET_Select() although this was not guarded by #ifdef
USE_SELECT like the other platform implementations. But NET_Select()
isn't used anywhere else in the code so that should be OK.
src/solaris/native/java/net/net_util_md.{c,h}
- Remove unused typdefs for getaddrinfo_f, freeaddrinfo_f,
getnameinfo_f and gai_strerror_f. gai_strerror_f was actually still
used in src/solaris/native/java/net/net_util_md.c to declare
gai_strerror_ptr. But after the change "7112670: Inet4AddressImpl
should use getaddrinfo/getnameinfo" removed its initialization,
gai_strerror_ptr was always NULL. I think the right solution is to
just call gai_strerror() directly, like this is done with
getaddrinfo()/getnameinfo() after 7112670. And that's exactly what
this change does.
- Removed unused globals getaddrinfo_ptr, freeaddrinfo_ptr and getnameinfo_ptr.
- Rename ThrowUnknownHostExceptionWithGaiError() to
NET_ThrowUnknownHostExceptionWithGaiError() to conform to the other
functions declared in this file (especially
NET_ThrowByNameWithLastError()).
- Reorder the functions such that all NET_ functions are declared at
the beginning of the file and the other utility functions at the end
(as indicated by the comments).
src/aix/native/java/net/aix_close.c
src/solaris/native/java/net/bsd_close.c
src/solaris/native/java/net/linux_close.c
src/solaris/native/java/net/solaris_close.c
- Remove unused NET_Select() function which was previously guarded by
#ifdef USE_SELECT.
src/solaris/native/java/net/Inet4AddressImpl.c
src/solaris/native/java/net/Inet6AddressImpl.c
- Rename ThrowUnknownHostExceptionWithGaiError() to
NET_ThrowUnknownHostExceptionWithGaiError().
Thank you and best regards,
Volker
Re: RFR[9](M): 8035949 : Remove unused macro USE_SELECT and clean up Unix version of net_util_md.{c,h}
On 28/02/2014 14:08, Chris Hegarty wrote:
Volker,
The changes look fine to me, and in line with what I was thinking too.
Quite trivially you can remove the block '{' from net_util_md.c Line
1637 and 1650. Also, the function comment says "Wrapper for
select/poll ..." ( can remove 'select' ).
It looks okay to me too.
AFAIK, USE_SELECT was a build time option a long time ago (when the JDK
had the HPI). It's good to see this dead code go away now.
-Alan.
RFR [9] 8035897 : FD_SETSIZE should be set on macosx
[ Volker: there are some trivial AIX changes here, maybe you could verify them? ] JDK-8021820 adds -D_DARWIN_UNLIMITED_SELECT to the build, but the fd_set struct is still limited to 1024. Snippet from man select(2): "Although the provision of getdtablesize(2) was intended to allow user programs to be written independent of the kernel limit on the number of open files, the dimension of a sufficiently large bit field for select remains a problem. The default size FD_SETSIZE (currently 1024) is somewhat smaller than the current kernel limit to the number of open files. However, in order to accommodate programs which might potentially use a larger number of open files with select, it is possible to increase this size within a program by providing a larger definition of FD_SETSIZE before the inclusion of . Either: 1) FD_SETSIZE needs to be set to a larger value, but what value, the kernel limit, or other? This is wasteful for most typical apps that don't use large numbers of file descriptors. Or, 2) If fd is greater than 1024, then an appropriate amount of memory could be allocated and cast to an fd_set. The FD_SET macro will write past FD_SETSIZE. I think option 2 is preferable: http://cr.openjdk.java.net/~chegar/8035897/webrev.00/webrev/ I'm still checking to see it an automatic regression test is possible, but I wanted to circulate the changes for comment first. Thanks, -Chris.
Re: RFR [9] 8035897 : FD_SETSIZE should be set on macosx
On 28/02/14 14:40, Chris Hegarty wrote: [ Volker: there are some trivial AIX changes here, maybe you could verify them? ] JDK-8021820 adds -D_DARWIN_UNLIMITED_SELECT to the build, but the fd_set struct is still limited to 1024. Snippet from man select(2): "Although the provision of getdtablesize(2) was intended to allow user programs to be written independent of the kernel limit on the number of open files, the dimension of a sufficiently large bit field for select remains a problem. The default size FD_SETSIZE (currently 1024) is somewhat smaller than the current kernel limit to the number of open files. However, in order to accommodate programs which might potentially use a larger number of open files with select, it is possible to increase this size within a program by providing a larger definition of FD_SETSIZE before the inclusion of . Either: 1) FD_SETSIZE needs to be set to a larger value, but what value, the kernel limit, or other? This is wasteful for most typical apps that don't use large numbers of file descriptors. Or, 2) If fd is greater than 1024, then an appropriate amount of memory could be allocated and cast to an fd_set. The FD_SET macro will write past FD_SETSIZE. I think option 2 is preferable: http://cr.openjdk.java.net/~chegar/8035897/webrev.00/webrev/ I'm still checking to see it an automatic regression test is possible, but I wanted to circulate the changes for comment first. Thanks, -Chris. I agree option 2 sounds better since adding (say) 4k * sizeof(fd_set) to the stack is quite significant given that it would rarely be required. On the change itself I noticed one path in bsd_close.c NET_Timeout where the malloc'd buffer is not freed (line 418) Is it potentially more efficient to use calloc() rather than malloc() + memset()? Maybe a test might need a shell script to call "ulimit -n 4096" or something to be able to test it. Michael
Re: RFR [9] 8035897 : FD_SETSIZE should be set on macosx
Thanks for looking at this Michael, comments inline... On 28/02/14 15:55, Michael McMahon wrote: ... I agree option 2 sounds better since adding (say) 4k * sizeof(fd_set) to the stack is quite significant given that it would rarely be required. Agreed. On the change itself I noticed one path in bsd_close.c NET_Timeout where the malloc'd buffer is not freed (line 418) D'oh. Fixed. Is it potentially more efficient to use calloc() rather than malloc() + memset()? Changed to calloc. Maybe a test might need a shell script to call "ulimit -n 4096" or something to be able to test it. It maybe shell related, but even with 'ulimit -n' = 256, the test in the latest webrev seems to blow through this soft limit without problem. Updated webrev: http://cr.openjdk.java.net/~chegar/8035897/webrev.01/webrev/ -Chris. Michael
Re: RFR[9](M): 8035949 : Remove unused macro USE_SELECT and clean up Unix version of net_util_md.{c,h}
Hi Alan, Chris,
thanks for the review. I've just pushed the change with the additions
proposed by you (I also removed the similar blocks/comments in
PlainSocketImpl.c).
Regards,
Volker
On Fri, Feb 28, 2014 at 3:14 PM, Alan Bateman wrote:
> On 28/02/2014 14:08, Chris Hegarty wrote:
>>
>> Volker,
>>
>> The changes look fine to me, and in line with what I was thinking too.
>> Quite trivially you can remove the block '{' from net_util_md.c Line 1637
>> and 1650. Also, the function comment says "Wrapper for select/poll ..." (
>> can remove 'select' ).
>>
> It looks okay to me too.
>
> AFAIK, USE_SELECT was a build time option a long time ago (when the JDK had
> the HPI). It's good to see this dead code go away now.
>
> -Alan.
Re: RFR [9] 8035897 : FD_SETSIZE should be set on macosx
On Feb 28, 3:55pm, michael.x.mcma...@oracle.com (Michael McMahon) wrote: -- Subject: Re: RFR [9] 8035897 : FD_SETSIZE should be set on macosx | On 28/02/14 14:40, Chris Hegarty wrote: | > [ Volker: there are some trivial AIX changes here, maybe you could | > verify them? ] | > | > JDK-8021820 adds -D_DARWIN_UNLIMITED_SELECT to the build, but the | > fd_set struct is still limited to 1024. | > | > Snippet from man select(2): | > | >"Although the provision of getdtablesize(2) was intended to allow | > user programs to be written | > independent of the kernel limit on the number of open files, the | > dimension of a sufficiently | > large bit field for select remains a problem. The default size | > FD_SETSIZE (currently 1024) is | > somewhat smaller than the current kernel limit to the number of | > open files. However, in order | > to accommodate programs which might potentially use a larger | > number of open files with select, | > it is possible to increase this size within a program by | > providing a larger definition of | > FD_SETSIZE before the inclusion of . | > | > Either: | > 1) FD_SETSIZE needs to be set to a larger value, but what value, the | > kernel limit, or other? This is wasteful for most typical apps that | > don't use large numbers of file descriptors. Or, | > 2) If fd is greater than 1024, then an appropriate amount of memory | > could be allocated and cast to an fd_set. The FD_SET macro will | > write past FD_SETSIZE. | > | > I think option 2 is preferable: | > http://cr.openjdk.java.net/~chegar/8035897/webrev.00/webrev/ | > | > I'm still checking to see it an automatic regression test is possible, | > but I wanted to circulate the changes for comment first. Can we instead please switch to poll? Select is problematic for many reasons: 1. Expensive because you need do operations on bitmasks [ffs/shifts]. 2. Expensive because you need to reset the masks before each call. 2. Can only report 3 types of events, read/write/except. 4. Non portable behavior for >= 256 fd's. All systems need source recompilation; some older systems need kernel recompilation. 5. Non-portable behavior with respect to ERESTART; it is unspecified what happens when the interrupting signal has SA_RESTART set. 6. Non-portable behavior when running out of resources. The only way to fix this is using non-blocking-io which is a bit of a pain. 7. Non-portable behavior with respect to "struct timeval *timeout". This is non-const, and it was originally intended to return the time left. Most implementations did not change "timeout", but one or two did, so it is always good to re-initialize "timeout". 8. Non-portable behavior as to the maximum timeout value supported. 9. Non-portable behavior for descriptor types other than regular files and sockets christos
Re: RFR [9] 8035897 : FD_SETSIZE should be set on macosx
On 28 Feb 2014, at 17:28, chris...@zoulas.com wrote: > On Feb 28, 3:55pm, michael.x.mcma...@oracle.com (Michael McMahon) wrote: > -- Subject: Re: RFR [9] 8035897 : FD_SETSIZE should be set on macosx > > | On 28/02/14 14:40, Chris Hegarty wrote: > | > [ Volker: there are some trivial AIX changes here, maybe you could > | > verify them? ] > | > > | > JDK-8021820 adds -D_DARWIN_UNLIMITED_SELECT to the build, but the > | > fd_set struct is still limited to 1024. > | > > | > Snippet from man select(2): > | > > | >"Although the provision of getdtablesize(2) was intended to allow > | > user programs to be written > | > independent of the kernel limit on the number of open files, the > | > dimension of a sufficiently > | > large bit field for select remains a problem. The default size > | > FD_SETSIZE (currently 1024) is > | > somewhat smaller than the current kernel limit to the number of > | > open files. However, in order > | > to accommodate programs which might potentially use a larger > | > number of open files with select, > | > it is possible to increase this size within a program by > | > providing a larger definition of > | > FD_SETSIZE before the inclusion of . > | > > | > Either: > | > 1) FD_SETSIZE needs to be set to a larger value, but what value, the > | > kernel limit, or other? This is wasteful for most typical apps that > | > don't use large numbers of file descriptors. Or, > | > 2) If fd is greater than 1024, then an appropriate amount of memory > | > could be allocated and cast to an fd_set. The FD_SET macro will > | > write past FD_SETSIZE. > | > > | > I think option 2 is preferable: > | > http://cr.openjdk.java.net/~chegar/8035897/webrev.00/webrev/ > | > > | > I'm still checking to see it an automatic regression test is possible, > | > but I wanted to circulate the changes for comment first. > > Can we instead please switch to poll? Select is problematic for many > reasons: We are using select on OS X, only, because of a bug in poll, see https://bugs.openjdk.java.net/browse/JDK-7131399 Michael ran into this in the original OpenJDK Mac port. -Chris. > > 1. Expensive because you need do operations on bitmasks [ffs/shifts]. > 2. Expensive because you need to reset the masks before each call. > 2. Can only report 3 types of events, read/write/except. > 4. Non portable behavior for >= 256 fd's. All systems need source > recompilation; some older systems need kernel recompilation. > 5. Non-portable behavior with respect to ERESTART; it is unspecified what > happens when the interrupting signal has SA_RESTART set. > 6. Non-portable behavior when running out of resources. The only > way to fix this is using non-blocking-io which is a bit of a pain. > 7. Non-portable behavior with respect to "struct timeval *timeout". > This is non-const, and it was originally intended to return > the time left. Most implementations did not change "timeout", but > one or two did, so it is always good to re-initialize "timeout". > 8. Non-portable behavior as to the maximum timeout value supported. > 9. Non-portable behavior for descriptor types other than regular files > and sockets > > christos
RFR [9] 8035868: Check for JNI pending exception in windows/native/sun/net/spi/DefaultProxySelector.c
Some more trivial checking in the Windows native code for JNI pending exceptions. http://cr.openjdk.java.net/~chegar/8035868/webrev.00/webrev/ -Chris.
Re: RFR [9] 8035897 : FD_SETSIZE should be set on macosx
On Feb 28, 5:41pm, chris.hega...@oracle.com (Chris Hegarty) wrote: -- Subject: Re: RFR [9] 8035897 : FD_SETSIZE should be set on macosx | We are using select on OS X, only, because of a bug in poll, see | https://bugs.openjdk.java.net/browse/JDK-7131399 | | Michael ran into this in the original OpenJDK Mac port. Ouch! What did apple say about it? This is pretty bad, and they should fix it. christos
