Hi Roger, Dmitry, May I ask you both to have a last look at this change before I commit? It took a while for this to go through our internal tests, hence the delay.
New version: http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.03/webrev/ Delta to last version: http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.02-webrev.03/webrev/ The changes are mostly cosmetic: - I tweaked a number of comments to make them clearer - If getrlimit(RLIMIT_NOFILE) returns an error, I now handle this correctly. - Just for readability, I explicitly initialize global variables instead of relying on static zero-initialization. - As Roger requested, I changed accesses to the entry table elements from using implicit pointer arithmetic to explicit array accesses with "&". Thank you for your time! Kind Regards, Thomas On Sat, Mar 12, 2016 at 8:38 AM, Thomas Stüfe <thomas.stu...@gmail.com> wrote: > Thank you Roger! > > On Fri, Mar 11, 2016 at 4:26 PM, Roger Riggs <roger.ri...@oracle.com> > wrote: > >> Hi Thomas, >> >> When returning the address of the fdentry, the style using &fdTable[fd] >> is preferred over >> the implicit pointer arithmetic (as it was in the previous version). >> >> Occurs in all 3 deltas: >> >> src/java.base/*/native/libnet/*_close.c: >> + result = fdTable + fd; >> >> and >> + result = fdOverflowTable[rootindex] + slabindex; >> >> The rest looks fine. >> >> Thanks, Roger >> >> >> >> >> On 3/10/2016 7:59 AM, Thomas Stüfe wrote: >> >>> Thank you Dmitry! >>> >>> I will fix the typo before comitting. >>> >>> Kind Regards, Thomas >>> >>> On Thu, Mar 10, 2016 at 9:19 AM, Dmitry Samersoff < >>> dmitry.samers...@oracle.com> wrote: >>> >>> 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.stu...@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. >>>> >>>> >> >
