Java 9 build 148 causes trouble in Apache Lucene/Solr/Elasticsearch

[Uwe Schindler]

Hi,

How about the following:

 

-          Check that the buffer is direct, if not throw IAE(“not direct buffer”)

-          Check that buffer has attachment==null (this tells you that it’s not a slice/dup), if not throw IAE(“not allowed to free duplicates/slices”)

-          Finally do the standard if (cleaner!=null) cleaner.clean(), but don’t throw any exceptions if cleaner is null (as this is implementation detail)

 

This allows for empty buffers without cleaner that are still marked as direct. But it disallows all slices or duplicates. 

 

I am fine with Alan’s proposal to restrict to MappedByteBuffer but that’s out of my interest – I am happy to unmap mapped byte buffers. I would also place the method in the legacy sun.misc.Unsafe only, the JDK-internal private one is not accessible to the outside. Of course for consistency it could be in both, but primarily it must be in sun.misc.Unsafe – that’s also what most code is using anyways.

 

Uwe

 

 

From: Peter Levart [mailto:peter.levart at gmail.com] 
Sent: Saturday, December 10, 2016 10:23 PM
To: Uwe Schindler <uschindler at apache.org>; Chris Hegarty <chris.hegarty at oracle.com>
Cc: jigsaw-dev at openjdk.java.net; Core-Libs-Dev <core-libs-dev at openjdk.java.net>
Subject: Re: Java 9 build 148 causes trouble in Apache Lucene/Solr/Elasticsearch

 

 

 

On 12/10/2016 09:00 PM, Uwe Schindler wrote:

Hi,

We noticed that buffers with zero length also have no cleaner. This is why we also have the null check in our code (see Github) and the guardWithTest in the MethodHandle, although we never free duplicates. So a noop is better imho.


Oh yes, good catch. Then what about being noop just for zero length? I don't know, maybe I'm just being paranoid and those who would use this API know perfectly well what they are doing. I'm just imagining a situation where one would create and keep just a duplicate of a direct buffer and afterwards use it to try to deallocate the native memory. This would be a noop, but the developer would think it works as GC would finally do it for him. I think it's better to throw an exception to prevent such situations...

Regards, Peter





I like the Unsafe approach. To me both variants are fine.

Uwe

Am 10. Dezember 2016 20:47:46 MEZ schrieb Peter Levart  <mailto:peter.levart at gmail.com> <peter.levart at gmail.com>: 

Hi Chris,

 

On 12/10/2016 06:11 PM, Chris Hegarty wrote:

How about: Unsafe::deallocate(ByteBuffer directBuffer)?
  http://cr.openjdk.java.net/~chegar/Unsafe_deallocate/ <http://cr.openjdk.java.net/%7Echegar/Unsafe_deallocate/> 


Apart from the fact that Unsafe is (was?) reserved for low-level stuff, I think this approach is reasonable. Is the method in jdk.internal.misc.Unsafe needed? You could add the method just to the sun.misc.Unsafe (to keep internal Unsafe free from hacks) and export the two packages selectively to jdk.unsupported.
 



We could attempt to limit this to the direct buffer that "owns" the
memory, i.e. not a duplicate or a slice, but I'm not sure it is worth
it.


What you have here *is* limited to direct ByteBuffer(s) that "own" the memory. Derived buffer(s) (duplicated or sliced) do not have a Cleaner instance (they have an 'attachment' to keep the 1st-level buffer reachable while they are reachable). I would even make it more unforgiving by throwing an IAE if the passed-in buffer didn't have a Cleaner. In addition I would specify this behavior. For example:

"Deallocates the underlying memory associated with given directBuffer if the buffer was obtained from either {@link ByteBuffer#allocateDirect} or {@link FileChannel#map} methods. In any other case (when the buffer is not a direct buffer or was obtained by  {@link ByteBuffer#duplicate() duplicating} or {@link ByteBuffer#slice(int, int) slicing} a direct buffer), the method throws {@code IllegalArgumentException}.

Regards, Peter