(Round 2) RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Dmitry Samersoff]

Thomas,

Looks good for me. But please wait for someone from core-libs team.

PS: Could you also fix a typeo at 79, 51, 53?

    s/initialized/initialization/

 51  * Heap allocated during initialization - one entry per fd

-Dmitry

On 2016-03-10 10:59, Thomas Stüfe wrote:
> Hi,
> 
> may I have a review of this new iteration for this fix?
> 
> Thank you!
> 
> Thomas
> 
> On Thu, Mar 3, 2016 at 5:26 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
> 
>> Hi all,
>>
>> https://bugs.openjdk.java.net/browse/JDK-8150460
>>
>> thanks to all who took the time to review the first version of this fix!
>>
>> This is the new version:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.02/webrev/
>>
>> I reworked the fix, trying to add in all the input I got: This fix uses a
>> simple one-dimensional array, preallocated at startup, for low-value file
>> descriptors. Like the code did before. Only for large values of file
>> descriptors it switches to an overflow table, organized as two dimensional
>> sparse array of fixed sized slabs, which are allocated on demand. Only the
>> overflow table is protected by a lock.
>>
>> For 99% of all cases we will be using the plain simple fdTable structure
>> as before. Only for unusually large file descriptor values we will be using
>> this overflow table.
>>
>> Memory footprint is kept low: for small values of RLIMIT_NOFILE, we will
>> only allocate as much space as we need. Only if file descriptor values get
>> large, memory is allocated in the overflow table.
>>
>> Note that I avoided the proposed double-checked locking solution: I find
>> it too risky in this place and also unnecessary. When calling getFdEntry(),
>> we will be executing a blocking IO operation afterwards, flanked by two
>> mutex locks (in startOp and endOp). So, I do not think the third mutex lock
>> in getFdEntry will add much, especially since it is only used in case of
>> larger file descriptor values.
>>
>> I also added the fix to bsd_close.c and aix_close.c. I do not like this
>> code triplication. I briefly played around with unifying this code, but
>> this is more difficult than it seems: implementations subtly differ between
>> the three platforms, and solaris implementation is completely different. It
>> may be a worthwhile cleanup, but that would be a separate issue.
>>
>> I did some artificial tests to check how the code does with many and large
>> file descriptor values, all seemed to work well. I also ran java/net jtreg
>> tests on Linux and AIX.
>>
>> Kind Regards, Thomas
>>
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.