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