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

[Roger Riggs]

Hi Martin,

I added a link to these notes to the JDK-8144531 
<https://bugs.openjdk.java.net/browse/JDK-8144531> Cleanup of Cleaner 
implementation.

I'm not sure I understand what

"a CompletableFuture-based API for cleanup actions."

would look like or how it would be used.

Thanks, Roger


On 5/9/2016 12:34 PM, Martin Buchholz wrote:
> I haven't been following along with the long review thread, but I took
> a look at the new Cleaner stuff and have some belated comments:
>
> ---
>
> Please add missing semicolon:
>   *        private final Cleaner.Cleanable cleanable
>
> ---
>
> The below looks non-sensical; maybe should return EV_CLEAR if some
> clearing, not cleaning, should occur?
>
>           * @return EV_CLEAR if the cleanup should occur;
>           *         EV_CLEAN if the cleanup should occur;
>
> ---
>
>          private final int hash;
>          private final ConcurrentHashMap<WeakKey<K>, ?> map;
>          Cleaner.Cleanable cleanable;
>
> Why isn't cleanable also private final?
>
> ---
>
>          Assert.assertEquals(map.get(k2), data, "value should be found
> in the map");
>          key = null;
>          System.gc();
>          Assert.assertNotEquals(map.get(k2), data, "value should not be
> found in the map");
>
> I initially expected this to fail at least intermittently because a
> cleaner may not yet have removed the entry from the map.
>
> Why aren't we calling whitebox.fullGC() everywhere where System.gc()
> is being called?
>
> What's particularly confusing here is that the WeakKey is not actually
> surely removed from the map, but instead two WeakKeys that may have
> been equal become non-equal when they get cleared by the gc.  Which is
> generally bad practice.  Which suggests we wouldn't implement a real
> public concurrent map of weak keys this way.
>
> ---
>
> WeakCleanable and friends are defined in jdk.internal, don't appear to
> be used at all, and since there are fields in CleanerImpl, they impose
> a hidden tax on all users of Cleaner.  I don't see why there is both
> WeakCleanable (using subclassing) and WeakCleanableRef (using a
> provided Runnable).  Is a compelling use case for WeakCleanables
> forthcoming?
>
> ---
>
> I keep vaguely hoping to see a CompletableFuture-based API for cleanup actions.