Long chains created by direct Buffer::slice
src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template contains this, unfortunately without further comments: | public $Type$Buffer slice() { | int pos = this.position(); | int lim = this.limit(); | assert (pos <= lim); | int rem = (pos <= lim ? lim - pos : 0); | int off = (pos << $LG_BYTES_PER_VALUE$); | assert (off >= 0); | return new Direct$Type$Buffer$RW$$BO$(this, -1, 0, rem, rem, off); | } ByteBuffer::duplicate and ByteBuffer::asReadOnlyBuffer have similar code. 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. 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(); | } 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.
2018/7/20 10:55:52 -0700, Florian Weimer <fw@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
+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@oracle.com wrote:
2018/7/20 10:55:52 -0700, Florian Weimer <fw@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
* Paul Sandoz:
I created this issue:
Thanks.
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.
Noticed that as well.
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);
This is essentially what I put through jtreg (jdk_core), without any obvious issues, but I had not time yet to file a bug and create a webrev. The parenthesis seem unnecessary. If this is an official JDK coding style, it is not widely used.
On Jul 27, 2018, at 1:09 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
* Paul Sandoz:
I created this issue:
Thanks.
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.
Noticed that as well.
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);
This is essentially what I put through jtreg (jdk_core), without any obvious issues,
Thanks for testing.
but I had not time yet to file a bug and create a webrev.
Ah, my apologies, i did not realize you had author status.
The parenthesis seem unnecessary. If this is an official JDK coding style, it is not widely used.
It is unusual but I chose to keep consistent with the style in the source. Paul.
Got caught up with JVMLS this week… I searched but could not find a test, so i added one, see: http://cr.openjdk.java.net/~psandoz/jdk/JDK-8208362-direct-buffer-att/webrev... <http://cr.openjdk.java.net/~psandoz/jdk/JDK-8208362-direct-buffer-att/webrev/> Paul.
On Jul 27, 2018, at 12:35 PM, Paul Sandoz <Paul.Sandoz@oracle.com> wrote:
On Jul 27, 2018, at 1:09 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
* Paul Sandoz:
I created this issue:
Thanks.
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.
Noticed that as well.
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);
This is essentially what I put through jtreg (jdk_core), without any obvious issues,
Thanks for testing.
but I had not time yet to file a bug and create a webrev.
Ah, my apologies, i did not realize you had author status.
The parenthesis seem unnecessary. If this is an official JDK coding style, it is not widely used.
It is unusual but I chose to keep consistent with the style in the source.
Paul.
On 03/08/2018 23:15, Paul Sandoz wrote:
Got caught up with JVMLS this week…
I searched but could not find a test, so i added one, see:
http://cr.openjdk.java.net/~psandoz/jdk/JDK-8208362-direct-buffer-att/webrev... <http://cr.openjdk.java.net/~psandoz/jdk/JDK-8208362-direct-buffer-att/webrev/>
One comment on the test is that the references to the intermediate refs may not be cleared immediately after the GC. It might be more reliable to loop, with a backoff, until at least one has been cleared. -Alan
Thanks, i uploaded a new webrev in place that uses a similar technique to some stuff in j.u.concurrent tests. Paul.
On Aug 5, 2018, at 11:59 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 03/08/2018 23:15, Paul Sandoz wrote:
Got caught up with JVMLS this week…
I searched but could not find a test, so i added one, see:
http://cr.openjdk.java.net/~psandoz/jdk/JDK-8208362-direct-buffer-att/webrev... <http://cr.openjdk.java.net/~psandoz/jdk/JDK-8208362-direct-buffer-att/webrev/>
One comment on the test is that the references to the intermediate refs may not be cleared immediately after the GC. It might be more reliable to loop, with a backoff, until at least one has been cleared.
-Alan
On 06/08/2018 19:13, Paul Sandoz wrote:
Thanks, i uploaded a new webrev in place that uses a similar technique to some stuff in j.u.concurrent tests.
The update looks good to me. -Alan
participants (4)
-
Alan Bateman
-
Florian Weimer
-
mark.reinhold＠oracle.com
-
Paul Sandoz