Re: code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
On 01/11/2011 09:47, Seán Coffey wrote: : Are you referring to the parent.close() call ? If otherParents is null, then we only have one reference to this FileDesriptor - i.e the Stream that has called close(). It's parent.close() that I'm wondering about. Suppose p1 is the parent, p2 is in otherParents. If p2.close is invoked then it looks like p1's close method will not be invoked, leaving p1 open and the underlying file descriptor closed. Any exception from releaser.close() becomes the main exception if one is seen there. Any exceptions from the earlier closes are then added as suppressed. I've run some tests on this and it looked to work as expected. (stack trace below) In this example then the IOException appears to be thrown suppressing one exception. That suppression exception in turn suppresses two others. I had expected that the primary IOException would have suppressed three exceptions. As I said, in practical terms this is much of a concern, just pointing out that it's not exactly as expected. I used hg rm/add. I guess hg mv would have worked too. You need to use hg mv so that the history can be followed. Also webrev handles it, at least before you create the changeset, so that you can easily see the changes. -Alan.
RE: Benchmarks for NIO buffer performance
Hi, Mark. You gave me a copy of those test when I was working on this bug: 4463011: (bf) View-buffer bulk get/put operations are slow If I recall correctly, I modified the tests for my particular problem and added them to that bug as attachments. iris -Original Message- From: Mark Reinhold Sent: Tuesday, November 01, 2011 8:48 PM To: David Holmes Cc: nio-disc...@openjdk.java.net; core-libs-dev@openjdk.java.net Subject: Re: Benchmarks for NIO buffer performance 2011/11/1 17:07 -0700, david.hol...@oracle.com: > I'm looking into refactoring all the generated buffer classes to reduce the > number of classes created. Part of that requires a performance comparison > between the old and new classes. So I'm looking for any benchmarks that may > exist to measure NIO buffer performance. There would have undoubtedly been > such > benchmarks back when NIO was first developed, but they seem to have been lost > - > unless someone on these lists know of them. ;-) You're correct -- we did write buffer benchmarks at the time, way back in 2000/2001. I'm sure I have them somewhere in my archive; I'll try to dig them out for you soon. - Mark
Re: Benchmarks for NIO buffer performance
2011/11/1 17:07 -0700, david.hol...@oracle.com: > I'm looking into refactoring all the generated buffer classes to reduce the > number of classes created. Part of that requires a performance comparison > between the old and new classes. So I'm looking for any benchmarks that may > exist to measure NIO buffer performance. There would have undoubtedly been > such > benchmarks back when NIO was first developed, but they seem to have been lost > - > unless someone on these lists know of them. ;-) You're correct -- we did write buffer benchmarks at the time, way back in 2000/2001. I'm sure I have them somewhere in my archive; I'll try to dig them out for you soon. - Mark
Re: code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
On 11/01/2011 11:09 PM, Seán Coffey wrote:
The fd count was only used if it was the finalizer thread. Any
explicit close() call not from finalizer meant that the FD got closed.
/*
* If FileDescriptor is still in use by another stream, the
finalizer
* will not close it.
*/
if ((useCount <= 0) || !isRunningFinalize()) {
close0();
}
regards,
Sean.
On 01/11/2011 14:57, Charles Lee wrote:
Does it change the original mechanism? IIRC, the original will remain
the other FileInputStream function well (can read from the under
stream), until the fd count become 0.
Hi Sean,
I finally got the background mail from Alan. No questions from me now.
--
Yours Charles
Benchmarks for NIO buffer performance
Need to cast a wide net here to try and catch past and present NIO developers/users. I'm looking into refactoring all the generated buffer classes to reduce the number of classes created. Part of that requires a performance comparison between the old and new classes. So I'm looking for any benchmarks that may exist to measure NIO buffer performance. There would have undoubtedly been such benchmarks back when NIO was first developed, but they seem to have been lost - unless someone on these lists know of them. ;-) Please reply to me direct, as well as the lists as I'm not subscribed to nio-discuss. Thanks, David Holmes
Re: Code Review Request for 6578042
Hi Darryl,
On 2/11/2011 3:51 AM, Darryl Mocek wrote:
Hello. Please review this patch to fix Bug #6578042
(System.clearProperty() throws ClassCastException if property value
isn't a String). The issue is that through the Hashtable methods, it's
possible to add a Property which isn't a String and set it through
System.setProperties. clearProperty cast the returned removed object as
a String, even if it wasn't a String. Test case provided.
Webrev: http://cr.openjdk.java.net/~sherman/darryl/6578042/webrev
This fix seems inconsistent with how this non-String problem is handled
elsewhere. Eg Properties itself does:
public String getProperty(String key) {
Object oval = super.get(key);
String sval = (oval instanceof String) ? (String)oval : null;
return ((sval == null) && (defaults != null)) ?
defaults.getProperty(key) : sval;
}
This will only return actual String values or else null - it won't
convert non-String values to Strings.
Seems to me that Properties should override remove(Object key) to
perform similar logic as getProperty(String key). That would fix the
System.clearProperty issue.
Or, if you want to confine the change to System do:
synchronized(props) {
String ret = props.getProperty(key);
props.remove(key);
return ret;
}
Not quite as efficient of course.
That said, I'd also support a spec change for clearProperty that states
that if the property has been set to a non-String value then
ClassCastException is thrown. I'm somewhat bemused that Properties
didn't override the necessary Hashtable methods to enforce Strings-only
in the first place.
Cheers,
David Holmes
hg: jdk8/tl/langtools: 7101933: langtools jtreg tests do not work with jprt on windows
Changeset: 9e2eb4bc49eb Author:jjh Date: 2011-11-01 15:49 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/9e2eb4bc49eb 7101933: langtools jtreg tests do not work with jprt on windows Summary: Fixed langtools/test/Makefile to work on cygwin. Updated jtreg to 4.1 and JCK to JCK8. Reviewed-by: jjg, ohair ! test/Makefile
Code Review Request for 6578042
Hello. Please review this patch to fix Bug #6578042 (System.clearProperty() throws ClassCastException if property value isn't a String). The issue is that through the Hashtable methods, it's possible to add a Property which isn't a String and set it through System.setProperties. clearProperty cast the returned removed object as a String, even if it wasn't a String. Test case provided. Webrev: http://cr.openjdk.java.net/~sherman/darryl/6578042/webrev Thanks, Darryl
Re: code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
On 11/01/2011 11:09 PM, Seán Coffey wrote:
If FileDescriptor is still
/*
* If FileDescriptor is still in use by another stream, the
finalizer
* will not close it.
*/
if ((useCount <= 0) || !isRunningFinalize()) {
close0();
}
Hi Sean,
Would you please lead me to the file which contains the above code? Is
it already in the openjdk8 repos? or is it in your patch? I kind of miss
those code
--
Yours Charles
Re: code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
The fd count was only used if it was the finalizer thread. Any explicit
close() call not from finalizer meant that the FD got closed.
/*
* If FileDescriptor is still in use by another stream, the
finalizer
* will not close it.
*/
if ((useCount <= 0) || !isRunningFinalize()) {
close0();
}
regards,
Sean.
On 01/11/2011 14:57, Charles Lee wrote:
Does it change the original mechanism? IIRC, the original will remain
the other FileInputStream function well (can read from the under
stream), until the fd count become 0.
Re: code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
On 11/01/2011 10:46 PM, Seán Coffey wrote: Charles, Yes - all three streams will now be closed. Previous to 7082769, when any of the streams calls close(), the underlying filedescriptor used get closed out via the native close0() call. A list of Closeables is maintained by FileDescriptor. regards, Sean. On 01/11/2011 13:54, Charles Lee wrote: On 10/29/2011 02:13 AM, Seán Coffey wrote: This is a second stab at cleaning up the close() and finalize() methods for FileInputStream / FileOutputStream / RandomAccessFile classes so that all parents/referents sharing the same native FileDescriptor are closed out correctly. With Alan's assistance, we have a better implementation in place where we avoid the use of counters and instead cycle through a list of shared closeables when a FileDescriptor is being shared. Bug report (not visible yet) http://bugs.sun.com/view_bug.do?bug_id=7105952 webrev : http://cr.openjdk.java.net/~coffeys/webrev.7105952/ regards, Sean. Hi Sean, If we have 3 FileInputStream which share the same FileDescriptor, and one of the FileInputStream calls its own close. Will other 2 FileInputStream close? I see fd.closeAll is called in the close of FileInputStream, I guess it will close all the things which shared the same fd. Does it change the original mechanism? IIRC, the original will remain the other FileInputStream function well (can read from the under stream), until the fd count become 0. -- Yours Charles
Re: code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
Charles, Yes - all three streams will now be closed. Previous to 7082769, when any of the streams calls close(), the underlying filedescriptor used get closed out via the native close0() call. A list of Closeables is maintained by FileDescriptor. regards, Sean. On 01/11/2011 13:54, Charles Lee wrote: On 10/29/2011 02:13 AM, Seán Coffey wrote: This is a second stab at cleaning up the close() and finalize() methods for FileInputStream / FileOutputStream / RandomAccessFile classes so that all parents/referents sharing the same native FileDescriptor are closed out correctly. With Alan's assistance, we have a better implementation in place where we avoid the use of counters and instead cycle through a list of shared closeables when a FileDescriptor is being shared. Bug report (not visible yet) http://bugs.sun.com/view_bug.do?bug_id=7105952 webrev : http://cr.openjdk.java.net/~coffeys/webrev.7105952/ regards, Sean. Hi Sean, If we have 3 FileInputStream which share the same FileDescriptor, and one of the FileInputStream calls its own close. Will other 2 FileInputStream close? I see fd.closeAll is called in the close of FileInputStream, I guess it will close all the things which shared the same fd.
Re: code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
On 10/29/2011 02:13 AM, Seán Coffey wrote: This is a second stab at cleaning up the close() and finalize() methods for FileInputStream / FileOutputStream / RandomAccessFile classes so that all parents/referents sharing the same native FileDescriptor are closed out correctly. With Alan's assistance, we have a better implementation in place where we avoid the use of counters and instead cycle through a list of shared closeables when a FileDescriptor is being shared. Bug report (not visible yet) http://bugs.sun.com/view_bug.do?bug_id=7105952 webrev : http://cr.openjdk.java.net/~coffeys/webrev.7105952/ regards, Sean. Hi Sean, If we have 3 FileInputStream which share the same FileDescriptor, and one of the FileInputStream calls its own close. Will other 2 FileInputStream close? I see fd.closeAll is called in the close of FileInputStream, I guess it will close all the things which shared the same fd. -- Yours Charles
Re: code review request : 7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
On 30/10/2011 16:33, Alan Bateman wrote: On 28/10/2011 19:13, Seán Coffey wrote: This is a second stab at cleaning up the close() and finalize() methods for FileInputStream / FileOutputStream / RandomAccessFile classes so that all parents/referents sharing the same native FileDescriptor are closed out correctly. With Alan's assistance, we have a better implementation in place where we avoid the use of counters and instead cycle through a list of shared closeables when a FileDescriptor is being shared. Bug report (not visible yet) http://bugs.sun.com/view_bug.do?bug_id=7105952 webrev : http://cr.openjdk.java.net/~coffeys/webrev.7105952/ regards, Sean. Thanks for persevering with this somewhat hairy issue. I think the proposed solution is probably the best approach and it's also very simple/easy to understand. Folks may ask why FileDescriptor isn't keeping weak references to the enclosing streams and that's to keep it simple and avoid the complications that FileInputStream and FileOutputStream finalizers bring (they are specified to close the stream which is what lead to the previously messy solution). For the probably <1% of usages where applications create a stream and then construct a new stream with its FileDescriptor then it just means that the otherwise unreferenced stream will remain reachable while the other stream is reachable. One thing that I think would be good is to clarify the javadoc on exactly how "sharing" of file descriptors is intended to work. I'm not suggesting you do this as part of this change, but just a reminder that the javadoc needs improvement. Another point is that I think this fix should bake for while before thinking about 7u (I realize your primary interest is 7u but this one clearly needs bake time). I'll add a comment explaining how the sharing should work. Yes - best to have this in jdk8 for a few weeks first before backporting. On the changes then it's a pity that the additions to FileDescriptor have to be duplicated to both implementations. I think we need to look at going back to one implementation, possibly injecting the field for the handle at build time. Is closeAll missing other.close? I'm also not sure that the suppressed exception handle is completely right - consider the case that releaser.close fails after the close of at least two other streams fails. In that case I think we want the IOException from releaser.close to be thrown with the IOExcepton from the two streams to be the suppressed exceptions. If I read the code correctly then one of them will be the suppressed exception and in turn this will have the other exceptions as suppressed exceptions. In practical terms I don't think this is a big deal as the stack trace will have everything. Are you referring to the parent.close() call ? If otherParents is null, then we only have one reference to this FileDesriptor - i.e the Stream that has called close(). Any exception from releaser.close() becomes the main exception if one is seen there. Any exceptions from the earlier closes are then added as suppressed. I've run some tests on this and it looked to work as expected. (stack trace below) Minor nit but shouldn't "closed" be private. Also probably should put the standard comments on attach and closeaAll yes - I'll make that private and clean up the comments. The webrev makes it hard to read the changes to the test (did you hg mv or hg rm/add?). I think moving it from etc to FileDescriptor is a good idea and you can probably rename it simply to Sharing.java. I used hg rm/add. I guess hg mv would have worked too. The testcase has been modified to test the new closeAll functionality (TestCloseReferentHandling()) - I'll rename as suggested. Might be a day or two before I get to cleaning this up. A few other issues on my work list. regards, Sean. -Alan. v245-sus $/suspool/home/sean/jdk8-tl/jdk/build/solaris-sparc/bin/java SC java.io.IOException: close0 exception!! at java.lang.Throwable.fillInStackTrace(Throwable.java:782) at java.lang.Throwable.(Throwable.java:265) at java.lang.Exception.(Exception.java:66) at java.io.IOException.(IOException.java:58) at java.io.FileInputStream$1.close(FileInputStream.java:302) at java.io.FileDescriptor.closeAll(FileDescriptor.java:208) at java.io.FileInputStream.close(FileInputStream.java:299) at SC.TestCloseReferentHandling(SC.java:295) at SC.main(SC.java:43) Suppressed: java.io.IOException: Bad close operation at java.lang.Throwable.fillInStackTrace(Throwable.java:782) at java.lang.Throwable.(Throwable.java:265) at java.lang.Exception.(Exception.java:66) at java.io.IOException.(IOException.java:58) at SC$BadFileInputStream.close(SC.java:358) at java.io.FileDescriptor.closeAll(FileDescriptor.java:198) ... 3 more
