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 >> > >
