Ping... Could I have reviewer and a sponsor, please?
Thanks you! Thomas On Thu, Feb 25, 2016 at 5:51 PM, Thomas Stüfe <thomas.stu...@gmail.com> wrote: > Hi Roger, > > thank you for the review! > > New webrev: > http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.01/webrev/ > > Please find my comments inline. > > On Wed, Feb 24, 2016 at 8:28 PM, Roger Riggs <roger.ri...@oracle.com> > wrote: > >> Hi Thomas, >> >> On 2/24/2016 12:30 PM, Thomas Stüfe wrote: >> >>> Hi all, >>> >>> please take a look at this proposed fix. >>> >>> The bug: https://bugs.openjdk.java.net/browse/JDK-8150460 >>> The Webrev: >>> >>> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.00/webrev/ >>> >>> Basically, the file descriptor table implemented in linux_close.c may not >>> work for RLIMIT_NO_FILE=infinite or may grow very large (I saw a 50MB >>> table) for high values for RLIMIT_NO_FILE. Please see details in the bug >>> description. >>> >>> The proposed solution is to implement the file descriptor table not as >>> plain array, but as a twodimensional sparse array, which grows on demand. >>> This keeps the memory footprint small and fixes the corner cases >>> described >>> in the bug description. >>> >>> Please note that the implemented solution is kept simple, at the cost of >>> somewhat higher (some kb) memory footprint for low values of >>> RLIMIT_NO_FILE. >>> This can be optimized, if we even think it is worth the trouble. >>> >>> Please also note that the proposed implementation now uses a mutex lock >>> for >>> every call to getFdEntry() - I do not think this matters, as this is all >>> in >>> preparation for an IO system call, which are usually way more expensive >>> than a pthread mutex. But again, this could be optimized. >>> >> I would suggest preallocating the index[0] array and then skip the mutex >> for that case. >> That would give the same as current performance. >> >> > I did this. > > >> And I'd suggest a different hi/low split of the indexes to reduce the >> size of pre-allocated memory. >> Most processes are going to use a lot fewer than 16384 fd's. How about >> 2048? >> > > I did this too. Now I calculate the split point based on RLIMIT_NO_FILE. > For small values of RLIMIT_NO_FILE > (<64K), I effectivly fall back to a one-dimensional array by making the > first level table a size 1. For larger values, > multiple second level tables, each 64K size, will be allocated on demand > (save for the first one which is preallocated). > > >> I have my doubts about needing to cover fd's up to the full range of 32 >> bits. >> Can the RLIMIT_NO_FILE be used too parametrize the allocation of the >> first level table? >> >> > I did this. > > Interesting note, I found nowhere in the Posix specs a mentioning that > socked descriptors have to be handed out > sequentially and therefore cannot be larger than RLIMIT_NO_FILE. But in > reality on all operating systems file descriptors > seem to be [0, RLIMIT_NO_FILE). > > > Not specific to your change but it would nice to see consistency between >> libnio and libnet on >> the name of the sigWakeup/INTERRUPT_SIGNAL constant. > > > I agree, but this is out of the scope of this bug fix. > > >> >> >>> This is an implementation proposal for Linux; the same code found its way >>> to BSD and AIX. Should you approve of this fix, I will modify those files >>> too. >>> >> yes please. >> >> $.02, Roger >> >> >> > Thanks, Thomas > > >> >>> Thank you and Kind Regards, Thomas >>> >> >> >
