Thanks Dmitry! On Wed, Apr 13, 2016 at 12:00 PM, Dmitry Samersoff < dmitry.samers...@oracle.com> wrote:
> 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. >
