+1, looks fine to me also.
Roger
On 4/13/2016 6:46 AM, Thomas Stüfe wrote:
Thanks Dmitry!
On Wed, Apr 13, 2016 at 12:00 PM, Dmitry Samersoff
<dmitry.samers...@oracle.com <mailto: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/
<http://cr.openjdk.java.net/%7Estuefe/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/
<http://cr.openjdk.java.net/%7Estuefe/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>
> <mailto: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>
> <mailto: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>
> <mailto: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>
> <mailto: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/
<http://cr.openjdk.java.net/%7Estuefe/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.
