Quantcast

We don't need jdk.internal.ref.Cleaner any more

classic Classic list List threaded Threaded
73 messages Options
1234
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

We don't need jdk.internal.ref.Cleaner any more

Peter Levart
Hi,

sun.misc.Cleaner has been moved to internal package jdk.internal.ref
recently [1] to clean-up sun.misc namespace. But now that:

- we have comparable public API (java.lang.ref.Cleaner & Cleanable) [2]
- we have an internal shared java.lang.ref.Cleaner instance
(jdk.internal.ref.CleanerFactory.cleaner())
- sun.misc.Cleaner is not a special kind of Reference any more in the
JVM [3]

...I think there's no reason to keep this special internal API any more.
It can be replaced with public API.

I propose to remove jdk.internal.ref.Cleaner class and replace its
usages with java.lang.ref.Cleaner and friends [4].

What do you say?

Regards, Peter


[1] https://bugs.openjdk.java.net/browse/JDK-8148117
[2] https://bugs.openjdk.java.net/browse/JDK-8138696
[3] https://bugs.openjdk.java.net/browse/JDK-8143847
[4]
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.01/






Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Jeremy Manson-4
Hadoop seems to use sun.misc.Cleaner:

http://grepcode.com/file/repo1.maven.org/maven2/org.apache.hadoop/hadoop-common/2.7.1/org/apache/hadoop/crypto/CryptoStreamUtils.java

So you may want to keep it around transitionally (a la Unsafe).

Jeremy

On Sun, Feb 7, 2016 at 2:53 AM, Peter Levart <[hidden email]> wrote:

> Hi,
>
> sun.misc.Cleaner has been moved to internal package jdk.internal.ref
> recently [1] to clean-up sun.misc namespace. But now that:
>
> - we have comparable public API (java.lang.ref.Cleaner & Cleanable) [2]
> - we have an internal shared java.lang.ref.Cleaner instance
> (jdk.internal.ref.CleanerFactory.cleaner())
> - sun.misc.Cleaner is not a special kind of Reference any more in the JVM
> [3]
>
> ...I think there's no reason to keep this special internal API any more.
> It can be replaced with public API.
>
> I propose to remove jdk.internal.ref.Cleaner class and replace its usages
> with java.lang.ref.Cleaner and friends [4].
>
> What do you say?
>
> Regards, Peter
>
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8148117
> [2] https://bugs.openjdk.java.net/browse/JDK-8138696
> [3] https://bugs.openjdk.java.net/browse/JDK-8143847
> [4]
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.01/
>
>
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Peter Levart


On 02/07/2016 08:01 PM, Jeremy Manson wrote:
> Hadoop seems to use sun.misc.Cleaner:
>
> http://grepcode.com/file/repo1.maven.org/maven2/org.apache.hadoop/hadoop-common/2.7.1/org/apache/hadoop/crypto/CryptoStreamUtils.java
>
> So you may want to keep it around transitionally (a la Unsafe).

JEP 260 [1] was listing sun.misc.Cleaner as a critical API, but
recently, it was decided to "hide" it away into jdk.internal.ref package
which will not be exported [2]. The reasoning was this:

"sun.misc.Cleaner ( was previously listed as a critical internal API,
but on further investigation has been moved to an open issue, for the
following reasons: 1) its primary use in the JDK is within NIO direct
buffers to release native memory. The base module cannot have a
dependency on jdk.unsupported so will need to be updated to use an
alternative cleaner, 2) the usage of Cleaner outside the JDK, as
determined by corpus analysis, has largely been observed to hack into
private fields of the internal NIO direct buffer classes to explicitly
release native memory. As stated in 1), the type of the cleaner used by
NIO direct buffers will have to change. Given this, and the fact that
JDK 9 has a new general purposed cleaner API, java.lang.ref.Cleaner, the
value of keep sun.misc.Cleaner is questionable."

If the decision to remove sun.misc.Cleaner was partly influenced by the
desire to maintain just 2 instead of 3 Cleaner(s), then my proposal to
migrate JDK code to the public API might enable Oracle to reconsider
keeping sun.misc.Cleaner.

[1] https://bugs.openjdk.java.net/browse/JDK-8132928
[2] https://bugs.openjdk.java.net/browse/JDK-8148117

Regards, Peter

>
> Jeremy
>
> On Sun, Feb 7, 2016 at 2:53 AM, Peter Levart <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi,
>
>     sun.misc.Cleaner has been moved to internal package
>     jdk.internal.ref recently [1] to clean-up sun.misc namespace. But
>     now that:
>
>     - we have comparable public API (java.lang.ref.Cleaner &
>     Cleanable) [2]
>     - we have an internal shared java.lang.ref.Cleaner instance
>     (jdk.internal.ref.CleanerFactory.cleaner())
>     - sun.misc.Cleaner is not a special kind of Reference any more in
>     the JVM [3]
>
>     ...I think there's no reason to keep this special internal API any
>     more. It can be replaced with public API.
>
>     I propose to remove jdk.internal.ref.Cleaner class and replace its
>     usages with java.lang.ref.Cleaner and friends [4].
>
>     What do you say?
>
>     Regards, Peter
>
>
>     [1] https://bugs.openjdk.java.net/browse/JDK-8148117
>     [2] https://bugs.openjdk.java.net/browse/JDK-8138696
>     [3] https://bugs.openjdk.java.net/browse/JDK-8143847
>     [4]
>     http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.01/
>     <http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/removeInternalCleaner/webrev.01/>
>
>
>
>
>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Peter Levart
In reply to this post by Peter Levart
Hi,

I 1st thought that my version of jtreg did not want to parse the new JDK
9 version strings as it kept saying:

Error: cannot determine version for JDK:
build/linux-x86_64-normal-server-release/images/jdk

...but the fact was that the patched JDK did not even want to boot-up.

So in order to make this actually work, I had to make some adjustments:

- lambdas and method references in the java.lang.ref.Cleaner
implementation had to be replaced with alternatives as the Cleaner is
needed early in the startup sequence.
- [sun.misc|jdk.internal.ref].Cleaner.create(referent, action) allowed
null action and just returned null in that case while
java.lang.ref.Cleaner.register(referent, action) throws NPE. There was
only one such usage where null action could get passed-in - when a
zero-sized MappedByteBuffer was constructed.

Here's the version which passes java/lang/ref and java/nio tests:

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/


Regards, Peter


On 02/07/2016 11:53 AM, Peter Levart wrote:

> Hi,
>
> sun.misc.Cleaner has been moved to internal package jdk.internal.ref
> recently [1] to clean-up sun.misc namespace. But now that:
>
> - we have comparable public API (java.lang.ref.Cleaner & Cleanable) [2]
> - we have an internal shared java.lang.ref.Cleaner instance
> (jdk.internal.ref.CleanerFactory.cleaner())
> - sun.misc.Cleaner is not a special kind of Reference any more in the
> JVM [3]
>
> ...I think there's no reason to keep this special internal API any
> more. It can be replaced with public API.
>
> I propose to remove jdk.internal.ref.Cleaner class and replace its
> usages with java.lang.ref.Cleaner and friends [4].
>
> What do you say?
>
> Regards, Peter
>
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8148117
> [2] https://bugs.openjdk.java.net/browse/JDK-8138696
> [3] https://bugs.openjdk.java.net/browse/JDK-8143847
> [4]
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.01/
>
>
>
>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Peter Levart
In reply to this post by Peter Levart


On 02/07/2016 11:20 PM, Peter Levart wrote:

>
>
> On 02/07/2016 08:01 PM, Jeremy Manson wrote:
>> Hadoop seems to use sun.misc.Cleaner:
>>
>> http://grepcode.com/file/repo1.maven.org/maven2/org.apache.hadoop/hadoop-common/2.7.1/org/apache/hadoop/crypto/CryptoStreamUtils.java
>>
>> So you may want to keep it around transitionally (a la Unsafe).
>
> JEP 260 [1] was listing sun.misc.Cleaner as a critical API, but
> recently, it was decided to "hide" it away into jdk.internal.ref
> package which will not be exported [2]. The reasoning was this:
>
> "sun.misc.Cleaner ( was previously listed as a critical internal API,
> but on further investigation has been moved to an open issue, for the
> following reasons: 1) its primary use in the JDK is within NIO direct
> buffers to release native memory. The base module cannot have a
> dependency on jdk.unsupported so will need to be updated to use an
> alternative cleaner, 2) the usage of Cleaner outside the JDK, as
> determined by corpus analysis, has largely been observed to hack into
> private fields of the internal NIO direct buffer classes to explicitly
> release native memory. As stated in 1), the type of the cleaner used
> by NIO direct buffers will have to change. Given this, and the fact
> that JDK 9 has a new general purposed cleaner API,
> java.lang.ref.Cleaner, the value of keep sun.misc.Cleaner is
> questionable."
>
> If the decision to remove sun.misc.Cleaner was partly influenced by
> the desire to maintain just 2 instead of 3 Cleaner(s), then my
> proposal to migrate JDK code to the public API might enable Oracle to
> reconsider keeping sun.misc.Cleaner.

OTOH, what hadoop is doing is exactly the usage described in the above
reasoning for removing sun.misc.Cleaner.

Hadoop use case:

     public static void freeDB(ByteBuffer buffer) {
         if (buffer instanceof sun.nio.ch.DirectBuffer) {
             final sun.misc.Cleaner bufferCleaner =
                 ((sun.nio.ch.DirectBuffer) buffer).cleaner();
             bufferCleaner.clean();
         }
     }

can be rewritten using reflection to be compatible with either
sun.misc.Cleaner (on JDK 8 or less) or java.lang.ref.Cleaner$Cleanable
(on JDK 9 or more):

     static final Method cleanMethod;

     static {
         try {
             Class<?> cleanerOrCleanable;
             try {
                 cleanerOrCleanable = Class.forName("sun.misc.Cleaner");
             } catch (ClassNotFoundException e1) {
                 try {
                     cleanerOrCleanable =
Class.forName("java.lang.ref.Cleaner$Cleanable");
                 } catch (ClassNotFoundException e2) {
                     e2.addSuppressed(e1);
                     throw e2;
                 }
             }
             cleanMethod = cleanerOrCleanable.getDeclaredMethod("clean");
         } catch (Exception e) {
             throw new Error(e);
         }
     }

     public static void freeDB_JDK8or9(ByteBuffer buffer) {
         if (buffer instanceof sun.nio.ch.DirectBuffer) {
             final Object bufferCleaner =
                 ((sun.nio.ch.DirectBuffer) buffer).cleaner();

             try {
                 if (bufferCleaner != null) {
                     cleanMethod.invoke(bufferCleaner);
                 }
             } catch (InvocationTargetException | IllegalAccessException
e) {
                 throw new Error(e);
             }
         }
     }


I thought about this use case and even kept the name of the method on
sun.nio.ch.DirectBuffer::cleaner to make things easier although the
return type in my patched DirectBufer is called
java.lang.ref.Cleaner$Cleanable.


Regards, Peter

>
> [1] https://bugs.openjdk.java.net/browse/JDK-8132928
> [2] https://bugs.openjdk.java.net/browse/JDK-8148117
>
> Regards, Peter
>
>>
>> Jeremy
>>
>> On Sun, Feb 7, 2016 at 2:53 AM, Peter Levart <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Hi,
>>
>>     sun.misc.Cleaner has been moved to internal package
>>     jdk.internal.ref recently [1] to clean-up sun.misc namespace. But
>>     now that:
>>
>>     - we have comparable public API (java.lang.ref.Cleaner &
>>     Cleanable) [2]
>>     - we have an internal shared java.lang.ref.Cleaner instance
>>     (jdk.internal.ref.CleanerFactory.cleaner())
>>     - sun.misc.Cleaner is not a special kind of Reference any more in
>>     the JVM [3]
>>
>>     ...I think there's no reason to keep this special internal API
>>     any more. It can be replaced with public API.
>>
>>     I propose to remove jdk.internal.ref.Cleaner class and replace
>>     its usages with java.lang.ref.Cleaner and friends [4].
>>
>>     What do you say?
>>
>>     Regards, Peter
>>
>>
>>     [1] https://bugs.openjdk.java.net/browse/JDK-8148117
>>     [2] https://bugs.openjdk.java.net/browse/JDK-8138696
>>     [3] https://bugs.openjdk.java.net/browse/JDK-8143847
>>     [4]
>>     http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.01/
>>     <http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/removeInternalCleaner/webrev.01/>
>>
>>
>>
>>
>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: We don't need jdk.internal.ref.Cleaner any more

Uwe Schindler-4
Hi Peter,

as discussed before in the other thread about move to jdk.internal: this looks fine to Apache Lucene. We have a separate issue to fix this for Java 9: https://issues.apache.org/jira/browse/LUCENE-6989

Currently the patch on the Lucene issue tries to cast the jdk.internal.Cleaner to Runnable; but with your patch, one would just need to cast to java.lang.ref.Cleaner$Cleanable (which is public anyways) and call clean().

The Runnable workaround was done for Lucene, Hadoop, and Netty, but with your current patch this extra hack will be obsolete:
http://cr.openjdk.java.net/~chegar/8148117/src/java.base/share/classes/jdk/internal/ref/Cleaner.java.udiff.html

So, I am fine with both solutions as workaround, if we can enforce unmapping - until an official and "safe" solution is found.

I was discussing with Andrew Haley and Mark Reinhold on FOSDEM about an "official way" to unmap ByteBuffers and there seems to be a really cool idea to make MappedByteBuffer/DirectByteBuffer implement Closeable, so unmapping may work without the horrible performance degradion on every access by using some extra safepoint and changed volatile semantics using a annotation in Hotspot.

Uwe

-----
Uwe Schindler
[hidden email]
ASF Member, Apache Lucene PMC / Committer
Bremen, Germany
http://lucene.apache.org/

> -----Original Message-----
> From: core-libs-dev [mailto:[hidden email]] On
> Behalf Of Peter Levart
> Sent: Sunday, February 07, 2016 11:57 PM
> To: Jeremy Manson <[hidden email]>
> Cc: Core-Libs-Dev <[hidden email]>
> Subject: Re: We don't need jdk.internal.ref.Cleaner any more
>
>
>
> On 02/07/2016 11:20 PM, Peter Levart wrote:
> >
> >
> > On 02/07/2016 08:01 PM, Jeremy Manson wrote:
> >> Hadoop seems to use sun.misc.Cleaner:
> >>
> >>
> http://grepcode.com/file/repo1.maven.org/maven2/org.apache.hadoop/ha
> doop-common/2.7.1/org/apache/hadoop/crypto/CryptoStreamUtils.java
> >>
> >> So you may want to keep it around transitionally (a la Unsafe).
> >
> > JEP 260 [1] was listing sun.misc.Cleaner as a critical API, but
> > recently, it was decided to "hide" it away into jdk.internal.ref
> > package which will not be exported [2]. The reasoning was this:
> >
> > "sun.misc.Cleaner ( was previously listed as a critical internal API,
> > but on further investigation has been moved to an open issue, for the
> > following reasons: 1) its primary use in the JDK is within NIO direct
> > buffers to release native memory. The base module cannot have a
> > dependency on jdk.unsupported so will need to be updated to use an
> > alternative cleaner, 2) the usage of Cleaner outside the JDK, as
> > determined by corpus analysis, has largely been observed to hack into
> > private fields of the internal NIO direct buffer classes to explicitly
> > release native memory. As stated in 1), the type of the cleaner used
> > by NIO direct buffers will have to change. Given this, and the fact
> > that JDK 9 has a new general purposed cleaner API,
> > java.lang.ref.Cleaner, the value of keep sun.misc.Cleaner is
> > questionable."
> >
> > If the decision to remove sun.misc.Cleaner was partly influenced by
> > the desire to maintain just 2 instead of 3 Cleaner(s), then my
> > proposal to migrate JDK code to the public API might enable Oracle to
> > reconsider keeping sun.misc.Cleaner.
>
> OTOH, what hadoop is doing is exactly the usage described in the above
> reasoning for removing sun.misc.Cleaner.
>
> Hadoop use case:
>
>      public static void freeDB(ByteBuffer buffer) {
>          if (buffer instanceof sun.nio.ch.DirectBuffer) {
>              final sun.misc.Cleaner bufferCleaner =
>                  ((sun.nio.ch.DirectBuffer) buffer).cleaner();
>              bufferCleaner.clean();
>          }
>      }
>
> can be rewritten using reflection to be compatible with either
> sun.misc.Cleaner (on JDK 8 or less) or java.lang.ref.Cleaner$Cleanable
> (on JDK 9 or more):
>
>      static final Method cleanMethod;
>
>      static {
>          try {
>              Class<?> cleanerOrCleanable;
>              try {
>                  cleanerOrCleanable = Class.forName("sun.misc.Cleaner");
>              } catch (ClassNotFoundException e1) {
>                  try {
>                      cleanerOrCleanable =
> Class.forName("java.lang.ref.Cleaner$Cleanable");
>                  } catch (ClassNotFoundException e2) {
>                      e2.addSuppressed(e1);
>                      throw e2;
>                  }
>              }
>              cleanMethod = cleanerOrCleanable.getDeclaredMethod("clean");
>          } catch (Exception e) {
>              throw new Error(e);
>          }
>      }
>
>      public static void freeDB_JDK8or9(ByteBuffer buffer) {
>          if (buffer instanceof sun.nio.ch.DirectBuffer) {
>              final Object bufferCleaner =
>                  ((sun.nio.ch.DirectBuffer) buffer).cleaner();
>
>              try {
>                  if (bufferCleaner != null) {
>                      cleanMethod.invoke(bufferCleaner);
>                  }
>              } catch (InvocationTargetException | IllegalAccessException
> e) {
>                  throw new Error(e);
>              }
>          }
>      }
>
>
> I thought about this use case and even kept the name of the method on
> sun.nio.ch.DirectBuffer::cleaner to make things easier although the
> return type in my patched DirectBufer is called
> java.lang.ref.Cleaner$Cleanable.
>
>
> Regards, Peter
>
> >
> > [1] https://bugs.openjdk.java.net/browse/JDK-8132928
> > [2] https://bugs.openjdk.java.net/browse/JDK-8148117
> >
> > Regards, Peter
> >
> >>
> >> Jeremy
> >>
> >> On Sun, Feb 7, 2016 at 2:53 AM, Peter Levart <[hidden email]
> >> <mailto:[hidden email]>> wrote:
> >>
> >>     Hi,
> >>
> >>     sun.misc.Cleaner has been moved to internal package
> >>     jdk.internal.ref recently [1] to clean-up sun.misc namespace. But
> >>     now that:
> >>
> >>     - we have comparable public API (java.lang.ref.Cleaner &
> >>     Cleanable) [2]
> >>     - we have an internal shared java.lang.ref.Cleaner instance
> >>     (jdk.internal.ref.CleanerFactory.cleaner())
> >>     - sun.misc.Cleaner is not a special kind of Reference any more in
> >>     the JVM [3]
> >>
> >>     ...I think there's no reason to keep this special internal API
> >>     any more. It can be replaced with public API.
> >>
> >>     I propose to remove jdk.internal.ref.Cleaner class and replace
> >>     its usages with java.lang.ref.Cleaner and friends [4].
> >>
> >>     What do you say?
> >>
> >>     Regards, Peter
> >>
> >>
> >>     [1] https://bugs.openjdk.java.net/browse/JDK-8148117
> >>     [2] https://bugs.openjdk.java.net/browse/JDK-8138696
> >>     [3] https://bugs.openjdk.java.net/browse/JDK-8143847
> >>     [4]
> >>     http://cr.openjdk.java.net/~plevart/jdk9-
> dev/removeInternalCleaner/webrev.01/
> >>     <http://cr.openjdk.java.net/%7Eplevart/jdk9-
> dev/removeInternalCleaner/webrev.01/>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Alan Bateman
In reply to this post by Peter Levart

On 07/02/2016 22:20, Peter Levart wrote:
> :
>
> If the decision to remove sun.misc.Cleaner was partly influenced by
> the desire to maintain just 2 instead of 3 Cleaner(s), then my
> proposal to migrate JDK code to the public API might enable Oracle to
> reconsider keeping sun.misc.Cleaner.

The main issue driving this is the design principles that we have listed
in JEP 200. We don't want a standard module (java.base in this case)
exporting a non-standard API. This means surgery to jettison the
sun.misc package from java.base and move it to its own module
(jdk.unsupported is the proposal in JEP 260). This is painful of course
but we are at least in good shape for the most critical internal API,
sun.misc.Unsafe.

For sun.misc.Cleaner then the original proposal was for it to be a
critical internal API too but it become clear that changing the type of
internal/private fields in the NIO buffer and channel classes would
break libraries that have been hacking into those fields. If they are
changing away then there seems little motive to keep sun.misc.Cleaner so
Chris moved into into jdk.internal.ref for now. As to whether we even
need to keep jdk.internal.ref.Cleaner then I think the only remaining
question was whether the special handling of Cleaner in the Reference
implementation was worth it or not. It may not be, in which case your
current proposal to just remove seems right.

-Alan.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Chris Hegarty

On 8 Feb 2016, at 06:27, Alan Bateman <[hidden email]> wrote:

>
> On 07/02/2016 22:20, Peter Levart wrote:
>> :
>>
>> If the decision to remove sun.misc.Cleaner was partly influenced by the desire to maintain just 2 instead of 3 Cleaner(s), then my proposal to migrate JDK code to the public API might enable Oracle to reconsider keeping sun.misc.Cleaner.
>
> The main issue driving this is the design principles that we have listed in JEP 200. We don't want a standard module (java.base in this case) exporting a non-standard API. This means surgery to jettison the sun.misc package from java.base and move it to its own module (jdk.unsupported is the proposal in JEP 260). This is painful of course but we are at least in good shape for the most critical internal API, sun.misc.Unsafe.
>
> For sun.misc.Cleaner then the original proposal was for it to be a critical internal API too but it become clear that changing the type of internal/private fields in the NIO buffer and channel classes would break libraries that have been hacking into those fields. If they are changing away then there seems little motive to keep sun.misc.Cleaner so Chris moved into into jdk.internal.ref for now. As to whether we even need to keep jdk.internal.ref.Cleaner then I think the only remaining question was whether the special handling of Cleaner in the Reference implementation was worth it or not. It may not be, in which case your current proposal to just remove seems right.

Alan’s summary of the situation is spot on.

Before moving sun.misc.Cleaner to jdk.internal.ref, I did enquire if it would be
possible to just remove it and use the new public Cleaner, but I got feedback
that there were some issues with failing NIO tests ( it appeared that Cleaners
were not running as quickly as possible ). I didn’t look further into that at the
time, since the VM still had special knowledge of these cleaners, but as you
say this is now removed.  It is probably a good time to revisit this.

-Chris.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Chris Hegarty
In reply to this post by Jeremy Manson-4

On 7 Feb 2016, at 19:01, Jeremy Manson <[hidden email]> wrote:

> Hadoop seems to use sun.misc.Cleaner:
>
> http://grepcode.com/file/repo1.maven.org/maven2/org.apache.hadoop/hadoop-common/2.7.1/org/apache/hadoop/crypto/CryptoStreamUtils.java

There is an issue in the Hadoop JIRA tracking this:

  https://issues.apache.org/jira/browse/HADOOP-12760

-Chris.

> So you may want to keep it around transitionally (a la Unsafe).
>
> Jeremy
>
> On Sun, Feb 7, 2016 at 2:53 AM, Peter Levart <[hidden email]> wrote:
>
>> Hi,
>>
>> sun.misc.Cleaner has been moved to internal package jdk.internal.ref
>> recently [1] to clean-up sun.misc namespace. But now that:
>>
>> - we have comparable public API (java.lang.ref.Cleaner & Cleanable) [2]
>> - we have an internal shared java.lang.ref.Cleaner instance
>> (jdk.internal.ref.CleanerFactory.cleaner())
>> - sun.misc.Cleaner is not a special kind of Reference any more in the JVM
>> [3]
>>
>> ...I think there's no reason to keep this special internal API any more.
>> It can be replaced with public API.
>>
>> I propose to remove jdk.internal.ref.Cleaner class and replace its usages
>> with java.lang.ref.Cleaner and friends [4].
>>
>> What do you say?
>>
>> Regards, Peter
>>
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8148117
>> [2] https://bugs.openjdk.java.net/browse/JDK-8138696
>> [3] https://bugs.openjdk.java.net/browse/JDK-8143847
>> [4]
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.01/
>>
>>
>>
>>
>>
>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Peter Levart
In reply to this post by Alan Bateman


On 02/08/2016 07:27 AM, Alan Bateman wrote:

>
> On 07/02/2016 22:20, Peter Levart wrote:
>> :
>>
>> If the decision to remove sun.misc.Cleaner was partly influenced by
>> the desire to maintain just 2 instead of 3 Cleaner(s), then my
>> proposal to migrate JDK code to the public API might enable Oracle to
>> reconsider keeping sun.misc.Cleaner.
>
> The main issue driving this is the design principles that we have
> listed in JEP 200. We don't want a standard module (java.base in this
> case) exporting a non-standard API. This means surgery to jettison the
> sun.misc package from java.base and move it to its own module
> (jdk.unsupported is the proposal in JEP 260). This is painful of
> course but we are at least in good shape for the most critical
> internal API, sun.misc.Unsafe.
>
> For sun.misc.Cleaner then the original proposal was for it to be a
> critical internal API too but it become clear that changing the type
> of internal/private fields in the NIO buffer and channel classes would
> break libraries that have been hacking into those fields. If they are
> changing away then there seems little motive to keep sun.misc.Cleaner
> so Chris moved into into jdk.internal.ref for now. As to whether we
> even need to keep jdk.internal.ref.Cleaner then I think the only
> remaining question was whether the special handling of Cleaner in the
> Reference implementation was worth it or not. It may not be, in which
> case your current proposal to just remove seems right.
>
> -Alan.

The special-casing of sun.misc.Cleaner in ReferenceHandler thread is
there to avoid bringing another thread into the processing pipeline,
right? Presumably to optimize the latency of clean-up operations. But it
seems that introducing another thread to the pipeline is not the source
of problematic latency when cleaning-up direct/mapped byte buffers. The
source of latency is the strategy of generational GC which promptly
discovers and enqueues young References with young referents, but is not
so prompt with old References having old referents.

If special-casing in ReferenceHandler is removed then an opportunity
opens to get rid of ReferenceHandler thread altogether. VM/GC could
enqueue Reference(s) to their respective ReferenceQueue(s) directly, so
everyone could enjoy the benefits sun.misc.Cleaner has...

Regards, Peter

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Peter Levart
In reply to this post by Chris Hegarty


On 02/08/2016 10:33 AM, Chris Hegarty wrote:

> On 8 Feb 2016, at 06:27, Alan Bateman <[hidden email]> wrote:
>
>> On 07/02/2016 22:20, Peter Levart wrote:
>>> :
>>>
>>> If the decision to remove sun.misc.Cleaner was partly influenced by the desire to maintain just 2 instead of 3 Cleaner(s), then my proposal to migrate JDK code to the public API might enable Oracle to reconsider keeping sun.misc.Cleaner.
>> The main issue driving this is the design principles that we have listed in JEP 200. We don't want a standard module (java.base in this case) exporting a non-standard API. This means surgery to jettison the sun.misc package from java.base and move it to its own module (jdk.unsupported is the proposal in JEP 260). This is painful of course but we are at least in good shape for the most critical internal API, sun.misc.Unsafe.
>>
>> For sun.misc.Cleaner then the original proposal was for it to be a critical internal API too but it become clear that changing the type of internal/private fields in the NIO buffer and channel classes would break libraries that have been hacking into those fields. If they are changing away then there seems little motive to keep sun.misc.Cleaner so Chris moved into into jdk.internal.ref for now. As to whether we even need to keep jdk.internal.ref.Cleaner then I think the only remaining question was whether the special handling of Cleaner in the Reference implementation was worth it or not. It may not be, in which case your current proposal to just remove seems right.
> Alan’s summary of the situation is spot on.
>
> Before moving sun.misc.Cleaner to jdk.internal.ref, I did enquire if it would be
> possible to just remove it and use the new public Cleaner, but I got feedback
> that there were some issues with failing NIO tests ( it appeared that Cleaners
> were not running as quickly as possible ). I didn’t look further into that at the
> time, since the VM still had special knowledge of these cleaners, but as you
> say this is now removed.  It is probably a good time to revisit this.
>
> -Chris.

Hi Chris,

Are you referring to the following test:

   test/java/nio/Buffer/DirectBufferAllocTest.java ?

This test was written specifically for the following issue:

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

...which exercises multi-threaded allocation of direct buffers. Even
sun.misc.Cleaner was not quick enough to promptly deallocate them, so
allocation path was patched to help ReferenceHandler thread while
re-attempting to allocate new direct buffers. Transitioning to
java.lang.ref.Cleaner which uses additional thread to process
Cleanable(s), direct-buffer allocation path must help the Cleaner thread
too. See:

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/src/java.base/share/classes/java/nio/Bits.java.sdiff.html

...for the place where this is performed...


Regards, Peter



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Chris Hegarty
Peter,

On 08/02/16 10:58, Peter Levart wrote:

> On 02/08/2016 10:33 AM, Chris Hegarty wrote:
>> On 8 Feb 2016, at 06:27, Alan Bateman<[hidden email]>  wrote:
>>
>>> On 07/02/2016 22:20, Peter Levart wrote:
>>>> :
>>>>
>>>> If the decision to remove sun.misc.Cleaner was partly influenced by the desire to maintain just 2 instead of 3 Cleaner(s), then my proposal to migrate JDK code to the public API might enable Oracle to reconsider keeping sun.misc.Cleaner.
>>> The main issue driving this is the design principles that we have listed in JEP 200. We don't want a standard module (java.base in this case) exporting a non-standard API. This means surgery to jettison the sun.misc package from java.base and move it to its own module (jdk.unsupported is the proposal in JEP 260). This is painful of course but we are at least in good shape for the most critical internal API, sun.misc.Unsafe.
>>>
>>> For sun.misc.Cleaner then the original proposal was for it to be a critical internal API too but it become clear that changing the type of internal/private fields in the NIO buffer and channel classes would break libraries that have been hacking into those fields. If they are changing away then there seems little motive to keep sun.misc.Cleaner so Chris moved into into jdk.internal.ref for now. As to whether we even need to keep jdk.internal.ref.Cleaner then I think the only remaining question was whether the special handling of Cleaner in the Reference implementation was worth it or not. It may not be, in which case your current proposal to just remove seems right.
>> Alan’s summary of the situation is spot on.
>>
>> Before moving sun.misc.Cleaner to jdk.internal.ref, I did enquire if it would be
>> possible to just remove it and use the new public Cleaner, but I got feedback
>> that there were some issues with failing NIO tests ( it appeared that Cleaners
>> were not running as quickly as possible ). I didn’t look further into that at the
>> time, since the VM still had special knowledge of these cleaners, but as you
>> say this is now removed.  It is probably a good time to revisit this.
>>
>> -Chris.
>
> Hi Chris,
>
> Are you referring to the following test:
>
>    test/java/nio/Buffer/DirectBufferAllocTest.java ?
>
> This test was written specifically for the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-6857566
>
> ...which exercises multi-threaded allocation of direct buffers. Even
> sun.misc.Cleaner was not quick enough to promptly deallocate them, so
> allocation path was patched to help ReferenceHandler thread while
> re-attempting to allocate new direct buffers. Transitioning to
> java.lang.ref.Cleaner which uses additional thread to process
> Cleanable(s), direct-buffer allocation path must help the Cleaner thread
> too. See:

Ah, I was not aware of this. So additional threads attempting to
allocate already help out with cleaning. So moving away from the
ReferenceHandler Thread is not such a big deal as I was thinking.

> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/src/java.base/share/classes/java/nio/Bits.java.sdiff.html
>
> ...for the place where this is performed...

I did look through your changes and I think they are good. I'd like
to spend a little more time on the details.

-Chris.

> Regards, Peter
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Chris Hegarty
Peter,

 >
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/

Given the changes in 6857566, I think your proposal to remove
jdk.internal.ref.Cleaner should be good. ( Note: third party libraries
digging into internals of direct buffers will need to change again ).

-Chris.

On 09/02/16 13:35, Chris Hegarty wrote:

> Peter,
>
> On 08/02/16 10:58, Peter Levart wrote:
>
>> On 02/08/2016 10:33 AM, Chris Hegarty wrote:
>>> On 8 Feb 2016, at 06:27, Alan Bateman<[hidden email]>  wrote:
>>>
>>>> On 07/02/2016 22:20, Peter Levart wrote:
>>>>> :
>>>>>
>>>>> If the decision to remove sun.misc.Cleaner was partly influenced by
>>>>> the desire to maintain just 2 instead of 3 Cleaner(s), then my
>>>>> proposal to migrate JDK code to the public API might enable Oracle
>>>>> to reconsider keeping sun.misc.Cleaner.
>>>> The main issue driving this is the design principles that we have
>>>> listed in JEP 200. We don't want a standard module (java.base in
>>>> this case) exporting a non-standard API. This means surgery to
>>>> jettison the sun.misc package from java.base and move it to its own
>>>> module (jdk.unsupported is the proposal in JEP 260). This is painful
>>>> of course but we are at least in good shape for the most critical
>>>> internal API, sun.misc.Unsafe.
>>>>
>>>> For sun.misc.Cleaner then the original proposal was for it to be a
>>>> critical internal API too but it become clear that changing the type
>>>> of internal/private fields in the NIO buffer and channel classes
>>>> would break libraries that have been hacking into those fields. If
>>>> they are changing away then there seems little motive to keep
>>>> sun.misc.Cleaner so Chris moved into into jdk.internal.ref for now.
>>>> As to whether we even need to keep jdk.internal.ref.Cleaner then I
>>>> think the only remaining question was whether the special handling
>>>> of Cleaner in the Reference implementation was worth it or not. It
>>>> may not be, in which case your current proposal to just remove seems
>>>> right.
>>> Alan’s summary of the situation is spot on.
>>>
>>> Before moving sun.misc.Cleaner to jdk.internal.ref, I did enquire if
>>> it would be
>>> possible to just remove it and use the new public Cleaner, but I got
>>> feedback
>>> that there were some issues with failing NIO tests ( it appeared that
>>> Cleaners
>>> were not running as quickly as possible ). I didn’t look further into
>>> that at the
>>> time, since the VM still had special knowledge of these cleaners, but
>>> as you
>>> say this is now removed.  It is probably a good time to revisit this.
>>>
>>> -Chris.
>>
>> Hi Chris,
>>
>> Are you referring to the following test:
>>
>>    test/java/nio/Buffer/DirectBufferAllocTest.java ?
>>
>> This test was written specifically for the following issue:
>>
>> https://bugs.openjdk.java.net/browse/JDK-6857566
>>
>> ...which exercises multi-threaded allocation of direct buffers. Even
>> sun.misc.Cleaner was not quick enough to promptly deallocate them, so
>> allocation path was patched to help ReferenceHandler thread while
>> re-attempting to allocate new direct buffers. Transitioning to
>> java.lang.ref.Cleaner which uses additional thread to process
>> Cleanable(s), direct-buffer allocation path must help the Cleaner thread
>> too. See:
>
> Ah, I was not aware of this. So additional threads attempting to
> allocate already help out with cleaning. So moving away from the
> ReferenceHandler Thread is not such a big deal as I was thinking.
>
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/src/java.base/share/classes/java/nio/Bits.java.sdiff.html
>>
>>
>> ...for the place where this is performed...
>
> I did look through your changes and I think they are good. I'd like
> to spend a little more time on the details.
>
> -Chris.
>
>> Regards, Peter
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Alan Bateman


On 15/02/2016 14:57, Chris Hegarty wrote:
> Peter,
>
> >
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/
>
> Given the changes in 6857566, I think your proposal to remove
> jdk.internal.ref.Cleaner should be good. ( Note: third party libraries
> digging into internals of direct buffers will need to change again ).
>
I think so too, the main think is that the fast path for cleaners has
been removed (and I see that it has).

-Alan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: We don't need jdk.internal.ref.Cleaner any more

Uwe Schindler-4
Hi,

> On 15/02/2016 14:57, Chris Hegarty wrote:
> > Peter,
> >
> > >
> > http://cr.openjdk.java.net/~plevart/jdk9-
> dev/removeInternalCleaner/webrev.02/
> >
> > Given the changes in 6857566, I think your proposal to remove
> > jdk.internal.ref.Cleaner should be good. ( Note: third party libraries
> > digging into internals of direct buffers will need to change again ).
> >
> I think so too, the main think is that the fast path for cleaners has
> been removed (and I see that it has).

What do you mean with fast path? The usual code pattern for forceful unmapping did not change:
- Make java.nio.DirectByteBuffer#cleaner() accessible (this is the thing where you need to fight against the Java access checks).
- call clean() (Runnable#run() in previous commit) on the returned object (with correct casting to interface before calling, this is where existing legacy code must change).

In preparation for these changes, I made a patch for Lucene (Solr/Elasticsearch) to "compile" the whole unmapping logic into a MethodHandle (this allows me to do all needed accesibility/security checks early on startup and before actually invoking unmapping). This allows us to tell user that forceful unmapping works *before* we actually need to call it: https://issues.apache.org/jira/secure/attachment/12787878/LUCENE-6989.patch

This will be part of Lucene 6.0 which will require Java 8 as minimum. Earlier Lucene version will not be compatible to Java 9 and can only be used in "slower" non-mmap mode.

Thanks,
Uwe

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Mandy Chung

> On Feb 15, 2016, at 9:06 AM, Uwe Schindler <[hidden email]> wrote:
>
> Hi,
>
>> On 15/02/2016 14:57, Chris Hegarty wrote:
>>> Peter,
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/
>>>

This patch looks correct to me.  The innocuous thread created for common cleaner is of Thread.MAX_PRIORITY - 2 priority whereas the reference handler thread is of MAX_PRIORITY priority.   I don’t know if this would impact any real-world application or whether worth having a dedicated thread with MAX_PRIORTY (David Holmes may have recommendation to this).

Another subtle difference is no more package access check whether running with security manager.  On the other hand, “suppressAccessChecks” permission is still checked.  I don’t have issue with this.

>>> Given the changes in 6857566, I think your proposal to remove
>>> jdk.internal.ref.Cleaner should be good. ( Note: third party libraries
>>> digging into internals of direct buffers will need to change again ).
>>>
>> I think so too, the main think is that the fast path for cleaners has
>> been removed (and I see that it has).
>
> What do you mean with fast path?

I believe Alan refers to the fast path handling jdk.internal.ref.Cleaner via the Reference Handler thread.

What this means to existing code is that the java.nio.DirectByteBuffer::cleaner method still exists but with java.lang.ref.Cleaner.Cleanable return type.  You still need to call setAccessible(true) on the cleaner method.  You can then statically reference java.lang.ref.Cleaner.Cleanable::clean method


Mandy

> The usual code pattern for forceful unmapping did not change:
> - Make java.nio.DirectByteBuffer#cleaner() accessible (this is the thing where you need to fight against the Java access checks).
> - call clean() (Runnable#run() in previous commit) on the returned object (with correct casting to interface before calling, this is where existing legacy code must change).
>
> In preparation for these changes, I made a patch for Lucene (Solr/Elasticsearch) to "compile" the whole unmapping logic into a MethodHandle (this allows me to do all needed accesibility/security checks early on startup and before actually invoking unmapping). This allows us to tell user that forceful unmapping works *before* we actually need to call it: https://issues.apache.org/jira/secure/attachment/12787878/LUCENE-6989.patch
>
> This will be part of Lucene 6.0 which will require Java 8 as minimum. Earlier Lucene version will not be compatible to Java 9 and can only be used in "slower" non-mmap mode.
>
> Thanks,
> Uwe
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

David Holmes
Hi Mandy,

On 16/02/2016 6:05 AM, Mandy Chung wrote:

>
>> On Feb 15, 2016, at 9:06 AM, Uwe Schindler <[hidden email]> wrote:
>>
>> Hi,
>>
>>> On 15/02/2016 14:57, Chris Hegarty wrote:
>>>> Peter,
>>>>
>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/
>>>>
>
> This patch looks correct to me.  The innocuous thread created for common cleaner is of Thread.MAX_PRIORITY - 2 priority whereas the reference handler thread is of MAX_PRIORITY priority.   I don’t know if this would impact any real-world application or whether worth having a dedicated thread with MAX_PRIORTY (David Holmes may have recommendation to this).

How did you know I would read this? :)

Thread priority has no meaning on Solaris, Linux or BSD/OSX by default.

Only Windows actually applies thread priorities under normal conditions.
The difference between MAX_PRIORITY and MAX_PRIORITY-2 is the former
uses THREAD_PRIORITY_HIGHEST and the latter
THREAD_PRIORITY_ABOVE_NORMAL. Without any real/reasonable workloads for
benchmarking it is all somewhat arbitrary. Arguably the Reference
handler thread has more work to do in general so might be better given
the higher priority.

Cheers,
David
-----

> Another subtle difference is no more package access check whether running with security manager.  On the other hand, “suppressAccessChecks” permission is still checked.  I don’t have issue with this.
>
>>>> Given the changes in 6857566, I think your proposal to remove
>>>> jdk.internal.ref.Cleaner should be good. ( Note: third party libraries
>>>> digging into internals of direct buffers will need to change again ).
>>>>
>>> I think so too, the main think is that the fast path for cleaners has
>>> been removed (and I see that it has).
>>
>> What do you mean with fast path?
>
> I believe Alan refers to the fast path handling jdk.internal.ref.Cleaner via the Reference Handler thread.
>
> What this means to existing code is that the java.nio.DirectByteBuffer::cleaner method still exists but with java.lang.ref.Cleaner.Cleanable return type.  You still need to call setAccessible(true) on the cleaner method.  You can then statically reference java.lang.ref.Cleaner.Cleanable::clean method
>
>
> Mandy
>
>> The usual code pattern for forceful unmapping did not change:
>> - Make java.nio.DirectByteBuffer#cleaner() accessible (this is the thing where you need to fight against the Java access checks).
>> - call clean() (Runnable#run() in previous commit) on the returned object (with correct casting to interface before calling, this is where existing legacy code must change).
>>
>> In preparation for these changes, I made a patch for Lucene (Solr/Elasticsearch) to "compile" the whole unmapping logic into a MethodHandle (this allows me to do all needed accesibility/security checks early on startup and before actually invoking unmapping). This allows us to tell user that forceful unmapping works *before* we actually need to call it: https://issues.apache.org/jira/secure/attachment/12787878/LUCENE-6989.patch
>>
>> This will be part of Lucene 6.0 which will require Java 8 as minimum. Earlier Lucene version will not be compatible to Java 9 and can only be used in "slower" non-mmap mode.
>>
>> Thanks,
>> Uwe
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Chris Hegarty
In reply to this post by Mandy Chung

On 15 Feb 2016, at 20:05, Mandy Chung <[hidden email]> wrote:

>
>> On Feb 15, 2016, at 9:06 AM, Uwe Schindler <[hidden email]> wrote:
>>
>> Hi,
>>
>>> On 15/02/2016 14:57, Chris Hegarty wrote:
>>>> Peter,
>>>>
>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/
>>>>
>
> This patch looks correct to me.  The innocuous thread created for common cleaner is of Thread.MAX_PRIORITY - 2 priority whereas the reference handler thread is of MAX_PRIORITY priority.

I see your point, but I don’t see this as an issue because threads failing to
allocate memory themselves get involved in cleaning. So pressure is
somewhat off the reference handler thread.

-Chris.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

Peter Levart
Hello everybody,

Thanks for looking into this and for all your comments. Now that it
seems we have a consensus that replacing internal Cleaner with public
Cleaner is not a wrong idea, I created an issue for it [1] so that we
may officially review the implementation. I also created an updated
webrev [2] which fixes some comments in usages that were not correct any
more after the change. I also took the liberty to modify the
CleanerImpl.InnocuousThreadFactory to give names to threads in
accordance to the style used in some other thread factories (Timer-0,
Timer-1, ..., Cleaner-0, Cleaner-1, ..., etc.). This gives the thread of
internal Cleaner instance, which is now constructed as part of the
boot-up sequence, a stable name: "Cleaner-0".

If you feel that internal Cleaner instance should have a Thread with
MAX_PRIORITY, I can incorporate that too.

[1] https://bugs.openjdk.java.net/browse/JDK-8149925
[2]
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.03/

I re-ran the java/lang/ref and java/nio tests and all pass except two
ignored tests:

java/nio/Buffer/LimitDirectMemory.sh: Test option to limit direct memory
allocation
java/nio/file/spi/SetDefaultProvider.java: Unit test for
java.nio.file.spi.FileSystemProvider

and the following two, which seem to not like my network config. or
something like that:

java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java: Unit
test for DatagramChannel's multicast support
java/nio/channels/DatagramChannel/Promiscuous.java: Test for
interference when two sockets are bound to the same port but joined to
different multicast groups


Regards, Peter


On 02/16/2016 03:02 PM, Chris Hegarty wrote:

> On 15 Feb 2016, at 20:05, Mandy Chung <[hidden email]> wrote:
>
>>> On Feb 15, 2016, at 9:06 AM, Uwe Schindler <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>>> On 15/02/2016 14:57, Chris Hegarty wrote:
>>>>> Peter,
>>>>>
>>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/
>>>>>
>> This patch looks correct to me.  The innocuous thread created for common cleaner is of Thread.MAX_PRIORITY - 2 priority whereas the reference handler thread is of MAX_PRIORITY priority.
> I see your point, but I don’t see this as an issue because threads failing to
> allocate memory themselves get involved in cleaning. So pressure is
> somewhat off the reference handler thread.
>
> -Chris.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: We don't need jdk.internal.ref.Cleaner any more

Mandy Chung
In reply to this post by David Holmes

> On Feb 15, 2016, at 9:24 PM, David Holmes <[hidden email]> wrote:
>
>> This patch looks correct to me.  The innocuous thread created for common cleaner is of Thread.MAX_PRIORITY - 2 priority whereas the reference handler thread is of MAX_PRIORITY priority.   I don’t know if this would impact any real-world application or whether worth having a dedicated thread with MAX_PRIORTY (David Holmes may have recommendation to this).
>
> How did you know I would read this? :)

Magic ball :)

>
> Thread priority has no meaning on Solaris, Linux or BSD/OSX by default.
>
> Only Windows actually applies thread priorities under normal conditions. The difference between MAX_PRIORITY and MAX_PRIORITY-2 is the former uses THREAD_PRIORITY_HIGHEST and the latter THREAD_PRIORITY_ABOVE_NORMAL. Without any real/reasonable workloads for benchmarking it is all somewhat arbitrary. Arguably the Reference handler thread has more work to do in general so might be better given the higher priority.


Thanks that’s useful. I never spent the time to look at the implementation.

Mandy
1234
Loading...