Long chains created by direct Buffer::slice

[Paul Sandoz]

+1 

I created this issue:

  https://bugs.openjdk.java.net/browse/JDK-8208362

The suggested fix requires a tweak though since an instance of a DirectBuffer interface is passed. This is required because views over direct ByteBuffers can be created.

Florian, i will push tomorrow and assign you as at the contributor.

Paul.

diff -r 448cd909c9e2 src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template
--- a/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template	Thu Jul 26 11:53:59 2018 -0700
+++ b/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template	Thu Jul 26 16:46:20 2018 -0700
@@ -194,7 +194,8 @@
 #if[byte]
         cleaner = null;
 #end[byte]
-        att = db;
+        Object attachment = db.attachment();
+        att = (attachment == null ? db : attachment);
 #else[rw]
         super(db, mark, pos, lim, cap, off);
         this.isReadOnly = true;

> On Jul 20, 2018, at 11:15 AM, mark.reinhold at oracle.com wrote:
> 
> 2018/7/20 10:55:52 -0700, Florian Weimer <fw at deneb.enyo.de>:
>> ...
>> 
>> The constructor invoked:
>> 
>> |     // For duplicates and slices
>> |     //
>> |     Direct$Type$Buffer$RW$$BO$(DirectBuffer db,         // package-private
>> |                                int mark, int pos, int lim, int cap,
>> |                                int off)
>> |     {
>> | #if[rw]
>> |         super(mark, pos, lim, cap);
>> |         address = db.address() + off;
>> | #if[byte]
>> |         cleaner = null;
>> | #end[byte]
>> |         att = db;
>> | #else[rw]
>> |         super(db, mark, pos, lim, cap, off);
>> |         this.isReadOnly = true;
>> | #end[rw]
>> |     }
>> 
>> The key part is the assignment to the att member.  If I understand
>> this correctly, it is needed to keep the backing object alive during
>> the lifetime of this buffer.
> 
> Correct.
> 
>>                              However, it causes the creation of a
>> long chain of buffer objects.  With -Xmx100m or so, the following test
>> will OOM fairly quickly for this reason:
>> 
>> |   volatile ByteBuffer buffer;
>> |   …
>> |   buffer = ByteBuffer.allocateDirect(16384);
>> |   while (true) {
>> |       buffer = buffer.duplicate();
>> |   }
> 
> Well spotted!  This bug has been lurking there for sixteen years.
> 
>> 
>> I wonder if it would be possible to change the setting of the att
>> member to this instead:
>> 
>> |         if (db.att == null) {
>> |             att = db;
>> |         } else {
>> |             att = db.att;
>> |         }
>> 
>> This would only keep the object alive which actually owns the backing
>> storage, as if Buffer::slice had been invoked on it directly.
> 
> Your suggested fix looks fine.
> 
> - Mark