Hi Brad,
Thanks for looking into this. Here's updated webrev:
http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.02/
...and answers are inline...
On 12/23/2014 02:30 AM, Bradford Wetmore wrote:
Hi Peter,
Looks like you have a committer status, will you be pushing this?
I can, yes. As soon as we clear-out the remaining questions, right?
On 12/18/2014 5:23 AM, Peter Levart wrote:
Hi,
I propose a patch for the following issue:
https://bugs.openjdk.java.net/browse/JDK-8047769
Here's the webrev:
http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.01/
Looks good. A few minor comments.
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.)
I have wrapped the lines to contain them inside the 80 column margin.
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?
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.
Both of these current calls are contained within a
AccessContrller.doPriviledged, the checkRead() seems redundant.
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).
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.
I modified the test to use jdk.testlibrary.Asserts class. Is this ok
from "run by hand" perspective or should the test be self-contained?
The patch uses a package-private FileInputStreamPool class that caches
open FileInputStream(s) so that at most one file descriptor is open for
a particular file. Reading from shared unbuffered FileInputStream in
multiple threads should be safe, right?
I would think (hope?) so, but I am not 100% sure. I tracked it down
into libjava native code:
io_util.c
=========
fd = GET_FD(this, fid);
if (fd == -1) {
JNU_ThrowIOException(env, "Stream Closed");
nread = -1;
} else {
nread = IO_Read(fd, buf, len);
which is then defined to handleRead, which calls something called
RESTARTABLE in io_util_md.c. Assuming I'm looking at the right code.
Core-libs folks?
The documentation doesn't mention threads anywhere in InputStream or
FileInputStream except in this piece of javadoc for available() method:
* Returns an estimate of the number of remaining bytes that can be
read (or
* skipped over) from this input stream without blocking by the next
* invocation of a method for this input stream. Returns 0 when the
file
* position is beyond EOF. The next invocation might be the same
*thread*
* or another *thread*. A single read or skip of this many bytes
will not
* block, but may read or skip fewer bytes.
...which indicates that multiple threads may be used to read from
FileInputStream(s)...
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:
@State(Scope.Thread)
public class DevUrandomTest {
private static final Object sharedLock = new Object();
private static final InputStream sharedRndStream;
static {
try {
sharedRndStream = new FileInputStream("/dev/urandom");
} catch (FileNotFoundException e) {
throw new RuntimeException(e);
}
}
// per-thread instance
private final byte[] buff = new byte[32];
@Benchmark
@BenchmarkMode(Mode.Throughput)
@Fork(value = 1, warmups = 0)
@Warmup(iterations = 5)
@Measurement(iterations = 10)
public byte testReadUnsynchronized() {
try {
sharedRndStream.read(buff);
} catch (IOException e) {
throw new RuntimeException(e);
}
return buff[0];
}
@Benchmark
@BenchmarkMode(Mode.Throughput)
@Fork(value = 1, warmups = 0)
@Warmup(iterations = 5)
@Measurement(iterations = 10)
public byte testReadSynchronized() {
try {
synchronized (sharedLock) {
sharedRndStream.read(buff);
}
} catch (IOException e) {
throw new RuntimeException(e);
}
return buff[0];
}
}
when run on:
1 thread:
Benchmark Mode Samples Score
Error Units
j.t.DevUrandomTest.testReadSynchronized thrpt 10 553331.718
± 309.350 ops/s
j.t.DevUrandomTest.testReadUnsynchronized thrpt 10 556654.281 ±
1703.320 ops/s
2 threads:
Benchmark Mode Samples Score
Error Units
j.t.DevUrandomTest.testReadSynchronized thrpt 10 346127.591 ±
73537.596 ops/s
j.t.DevUrandomTest.testReadUnsynchronized thrpt 10 570414.143 ±
25134.664 ops/s
4 threads:
Benchmark Mode Samples Score
Error Units
j.t.DevUrandomTest.testReadSynchronized thrpt 10 392531.196 ±
67848.064 ops/s
j.t.DevUrandomTest.testReadUnsynchronized thrpt 10 500111.743 ±
62730.048 ops/s
sun/security/provider/PolicyFile/GrantAllPermToExtWhenNoPolicy.java:
Make sure that when no system policy and user policy files exist, the
built-in default policy will be used, which - amongst other things -
grants standard extensions the AllPermission.
sun/security/provider/PolicyParser/ExtDirsChange.java: standard
extensions path is hard-coded in default system policy file
sun/security/provider/PolicyParser/PrincipalExpansionError.java: parser
incorrectly ignores a principal if the principal name expands to
nothing.
...they are unrelated to this patch - seems to be caused by recent
layout changes for modular runtime images.
Hopefully you saw my previous response. Repeating:
---begin---
They've been failing for a while:
https://bugs.openjdk.java.net/browse/JDK-8039280
These tests are all "/manual" so they don't show up in our regular
runs of JPRT/jtreg, which include -a.
-a | -automatic | -automagic
Any test with /manual will not be run
---end---
Thanks,
Brad
Yes, I saw that. Thanks.
Regards, Peter
