sun.nio.ch.Util: Don't cache an unlimited amount of memory

[Tony Printezis]

Hi Alan,

Thanks for the comments! 

On January 15, 2016 at 8:09:22 AM, Alan Bateman (alan.bateman at oracle.com) wrote:



On 12/01/2016 15:19, Tony Printezis wrote:
Alan and all,

Here’s the first version of this patch (along with a test):

http://cr.openjdk.java.net/~tonyp/nio-max-buffer-size/webrev.1/

A few points:

* I called the property java.nio.MaxCachedTempBufferSize. I’m happy to call it something else if you don’t like the name.

* I was not sure what the best way to read the property is. I took the same approach used for MaxDirectMemorySize and I read it from the saved properties on sun.misc.VM. Feel free to suggest a more appropriate alternative.

* I have compiled and run the test and it works as expected. But I haven’t run it within jtreg, though, given that I need to upgrade my jtreg to a more recent version. I’ll do that when I get a chance. But some tweaking on the metadata on the top comment might be required…


The property is JDK-specific so better to use jdk.* as prefix. A name such as jdk.nio.maxCachedBufferSize works for me. 


Sounds good. I’ll change it (and update any field / method names to drop the “Temp”).



If it's property only then we'll need to get the value from System.getProperty and that will need to be done in a privileged block.


OK. So I should drop the approach of getting it from the saved properties?




It would be good if maxCachedBufferSize could be final. 


Done.



Also as this is a static then the isTempBufferTooLarge methods could be static too. That would be arguably cleaner anyway as the limit is not specific to the thread's buffer cache. It would also mean that getTemporaryDirectBuffer and offeXXXTemporaryDirectBuffer can just check the size without needing the thread's cache.


I’m OK with this change. In fact, this is how it’s implemented in our codebase. However, I thought that maybe in the future it might be helpful to allow different threads to have a different max? But maybe we could do that change if we ever need to…



The test will need a bit of clean-up. I assume you will add a copyright header.


Yep, I forgot about it. :-) Done.



  It would be good to use try-with-resources as a disk full or other issue will leave the file open. 


Done. Should I also delete each temporary file that’s created?



There are really long (>200 chars) lines that are really hard to look at without scrolling so it would be good to get those down to sane lengths that would allow for future side-by-side diffs.


Fixed the long lines...

Should I also move the test to the test/sun/nio/ch dir (it’s in test/java/nio/channels)? Incidentally, I did run the test within jtreg and it worked as expected.

I’ll create a JIRA issue for this and post an updated webrev shortly.

Tony




Otherwise the approach seems okay to me.

-Alan










-----

Tony Printezis | JVM/GC Engineer / VM Team | Twitter

@TonyPrintezis
tprintezis at twitter.com

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20160115/61001133/attachment.html>