<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Brad,<br>
<br>
Thanks for looking into this. Here's updated webrev:<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.02/">http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.02/</a><br>
<br>
...and answers are inline...<br>
<br>
On 12/23/2014 02:30 AM, Bradford Wetmore wrote:<br>
</div>
<blockquote cite="mid:5498C5A0.50104@oracle.com" type="cite">
<br>
Hi Peter,
<br>
<br>
Looks like you have a committer status, will you be pushing this?
<br>
</blockquote>
<br>
I can, yes. As soon as we clear-out the remaining questions, right?<br>
<br>
<blockquote cite="mid:5498C5A0.50104@oracle.com" type="cite">
<br>
On 12/18/2014 5:23 AM, Peter Levart wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
I propose a patch for the following issue:
<br>
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8047769">https://bugs.openjdk.java.net/browse/JDK-8047769</a>
<br>
<br>
Here's the webrev:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.01/">http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.01/</a>
<br>
</blockquote>
<br>
Looks good. A few minor comments.
<br>
<br>
In a couple places, there are a few lines > 80 chars. (It's a
pet peeve of mine, this makes side-by-side reviews difficult
without a wide monitor. I realize not everyone shares this view,
but they're probably not working on a laptop screen regularly.)
<br>
</blockquote>
<br>
I have wrapped the lines to contain them inside the 80 column
margin.<br>
<br>
<blockquote cite="mid:5498C5A0.50104@oracle.com" type="cite">
<br>
Do you need to close the InputStream when last holder is GC'd, or
do we just let the FileInputStream's finalizer take care of that?
<br>
</blockquote>
<br>
WeakReference<UncloseableInputStream> is enqueued when it is
cleared, so at that time we have no access to the referent
(UncloseableInputStream) any more. We could, in addition, "strongly"
reference the underlying FileInputStream in the WeakReference
subclass itself, but that would just prolong the life of
FileInputStream (possibly forever if no further calls to
FileInputStreamPool are made that expunge the references from the
queue). So yes, we rely on FileInputStream's finalizer, but I don't
see any other way that would be better than that. NativePRNG and
URLSeedGenerator don't have a means of closing underlying resources
either, so this is not making things any worse.<br>
<br>
<blockquote cite="mid:5498C5A0.50104@oracle.com" type="cite">
<br>
Both of these current calls are contained within a
AccessContrller.doPriviledged, the checkRead() seems redundant.
<br>
</blockquote>
<br>
That's right, but from encapsulation, uniformity, security and
future maintainability standpoint, I would keep this logic in. It
doesn't hurt. Another possibility is to move doPriviliged call to
FileInputStreamPool itself and declare it's API exposing security
capability (of reading any file).<br>
<br>
<blockquote cite="mid:5498C5A0.50104@oracle.com" type="cite">
<br>
In your test case, if assertions are not enabled, the tests at
46/50/51 are noops, e.g. when run by hand. Generally should
directly throw Exceptions.
<br>
</blockquote>
<br>
I modified the test to use jdk.testlibrary.<span
style="background-color:#344134;">Asserts class. Is this ok from
"run by hand" perspective or should the test be self-contained?<br>
<br>
</span>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<blockquote cite="mid:5498C5A0.50104@oracle.com" type="cite">
<br>
<blockquote type="cite">The patch uses a package-private
FileInputStreamPool class that caches
<br>
open FileInputStream(s) so that at most one file descriptor is
open for
<br>
a particular file. Reading from shared unbuffered
FileInputStream in
<br>
multiple threads should be safe, right?
<br>
</blockquote>
<br>
I would think (hope?) so, but I am not 100% sure. I tracked it
down into libjava native code:
<br>
<br>
io_util.c
<br>
=========
<br>
fd = GET_FD(this, fid);
<br>
if (fd == -1) {
<br>
JNU_ThrowIOException(env, "Stream Closed");
<br>
nread = -1;
<br>
} else {
<br>
nread = IO_Read(fd, buf, len);
<br>
<br>
which is then defined to handleRead, which calls something called
RESTARTABLE in io_util_md.c. Assuming I'm looking at the right
code.
<br>
<br>
Core-libs folks?
<br>
</blockquote>
<br>
The documentation doesn't mention threads anywhere in InputStream or
FileInputStream except in this piece of javadoc for available()
method:<br>
<br>
* Returns an estimate of the number of remaining bytes that can
be read (or<br>
* skipped over) from this input stream without blocking by the
next<br>
* invocation of a method for this input stream. Returns 0 when
the file<br>
* position is beyond EOF. The next invocation might be the same
<b>thread</b><br>
* or another <b>thread</b>. A single read or skip of this many
bytes will not<br>
* block, but may read or skip fewer bytes.<br>
<br>
<br>
...which indicates that multiple threads may be used to read from
FileInputStream(s)...<br>
<br>
Anyway it works correctly and not redundantly synchronizing in Java
seems to increase the mutithreaded throughput a bit (or more
acurately, does not decrease it compared to single-threaded case).
For example, the following JMH benchmark:<br>
<br>
<br>
@State(Scope.Thread)<br>
public class DevUrandomTest {<br>
<br>
private static final Object sharedLock = new Object();<br>
private static final InputStream sharedRndStream;<br>
<br>
static {<br>
try {<br>
sharedRndStream = new FileInputStream("/dev/urandom");<br>
} catch (FileNotFoundException e) {<br>
throw new RuntimeException(e);<br>
}<br>
}<br>
<br>
// per-thread instance<br>
private final byte[] buff = new byte[32];<br>
<br>
@Benchmark<br>
@BenchmarkMode(Mode.Throughput)<br>
@Fork(value = 1, warmups = 0)<br>
@Warmup(iterations = 5)<br>
@Measurement(iterations = 10)<br>
public byte testReadUnsynchronized() {<br>
try {<br>
sharedRndStream.read(buff);<br>
} catch (IOException e) {<br>
throw new RuntimeException(e);<br>
}<br>
return buff[0];<br>
}<br>
<br>
@Benchmark<br>
@BenchmarkMode(Mode.Throughput)<br>
@Fork(value = 1, warmups = 0)<br>
@Warmup(iterations = 5)<br>
@Measurement(iterations = 10)<br>
public byte testReadSynchronized() {<br>
try {<br>
synchronized (sharedLock) {<br>
sharedRndStream.read(buff);<br>
}<br>
} catch (IOException e) {<br>
throw new RuntimeException(e);<br>
}<br>
return buff[0];<br>
}<br>
}<br>
<br>
<br>
when run on:<br>
<br>
1 thread:<br>
<br>
Benchmark Mode Samples
Score Error Units<br>
j.t.DevUrandomTest.testReadSynchronized thrpt 10
553331.718 ± 309.350 ops/s<br>
j.t.DevUrandomTest.testReadUnsynchronized thrpt 10
556654.281 ± 1703.320 ops/s<br>
<br>
2 threads:<br>
<br>
Benchmark Mode Samples
Score Error Units<br>
j.t.DevUrandomTest.testReadSynchronized thrpt 10
346127.591 ± 73537.596 ops/s<br>
j.t.DevUrandomTest.testReadUnsynchronized thrpt 10
570414.143 ± 25134.664 ops/s<br>
<br>
4 threads:<br>
<br>
Benchmark Mode Samples
Score Error Units<br>
j.t.DevUrandomTest.testReadSynchronized thrpt 10
392531.196 ± 67848.064 ops/s<br>
j.t.DevUrandomTest.testReadUnsynchronized thrpt 10
500111.743 ± 62730.048 ops/s<br>
<br>
<br>
<blockquote cite="mid:5498C5A0.50104@oracle.com" type="cite">
<br>
<blockquote type="cite">sun/security/provider/PolicyFile/GrantAllPermToExtWhenNoPolicy.java:
<br>
Make sure that when no system policy and user policy files
exist, the
<br>
built-in default policy will be used, which - amongst other
things -
<br>
grants standard extensions the AllPermission.
<br>
sun/security/provider/PolicyParser/ExtDirsChange.java: standard
<br>
extensions path is hard-coded in default system policy file
<br>
sun/security/provider/PolicyParser/PrincipalExpansionError.java:
parser
<br>
incorrectly ignores a principal if the principal name expands to
nothing.
<br>
<br>
...they are unrelated to this patch - seems to be caused by
recent
<br>
layout changes for modular runtime images.
<br>
</blockquote>
<br>
Hopefully you saw my previous response. Repeating:
<br>
<br>
---begin---
<br>
They've been failing for a while:
<br>
<br>
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8039280">https://bugs.openjdk.java.net/browse/JDK-8039280</a>
<br>
<br>
These tests are all "/manual" so they don't show up in our regular
runs of JPRT/jtreg, which include -a.
<br>
<br>
-a | -automatic | -automagic
<br>
Any test with /manual will not be run
<br>
---end---
<br>
<br>
Thanks,
<br>
<br>
Brad
<br>
</blockquote>
<br>
Yes, I saw that. Thanks.<br>
<br>
Regards, Peter<br>
<br>
<br>
</body>
</html>