Thomas, Looks good for me!
-Dmitry On 2016-04-13 12:12, Thomas Stüfe wrote: > 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 > <mailto:thomas.stu...@gmail.com>> wrote: > > Thank you Roger! > > On Fri, Mar 11, 2016 at 4:26 PM, Roger Riggs <roger.ri...@oracle.com > <mailto: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 > <mailto: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 > <mailto: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. > > > > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
