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

[Peter Levart]

Hi Kim, Roger, Mandy,

Resending with correct From: field to reach the list too...

On 02/24/2016 12:22 AM, Kim Barrett wrote:
>> On Feb 23, 2016, at 11:35 AM, Peter Levart <peter.levart at gmail.com> wrote:
>>
>> Hi Roger, Mandy,
>>
>> Here's another webrev:
>>
>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.05/
>>
>> I renamed the method and reworded the specification. Is this better now?
>>
>>
>> On 02/22/2016 10:56 PM, Roger Riggs wrote:
>>> Hi Mandy,
>>>
>>> On 2/22/2016 4:41 PM, Mandy Chung wrote:
>>>
>>>> The existing way to do that is to register phantom references in your own ReferenceQueue and then drain the queue at appropriate point.  Would you consider having a method to return ReferenceQueue maintained by the cleaner instead?
>>> If the queue is exposed, there is no assurance that the cleanable function would be called.
>>> Any caller could introduce a bug by not doing the proper cleaning.
>>>
>>> I was more concerned with the crossing of Reference.tryHandlePending with the cleaning thread.
>>> The method description does not mention anything related to the Reference processing thread
>>> though that is all implementation.  The @implNote might be a bit more concise and less informal.
>>>
>>> Roger
>> Yes, ReferenceHandler thread is just an implementation detail. The specification of java.lang.Reference subclasses doesn't even mention it. It talks about GC enqueueing Reference objects:
>>
>> "Suppose that the garbage collector determines at a certain point in time...
>> ...At the same time or at some later time it will enqueue those newly-cleared weak references that are registered with reference queues."
>>
>> So in this respect ReferenceHandler is just an extension of GC Reference discovery/enqueuing logic, delegated to a background thread on Java side. The Cleaner.cleanNextPending() method tries to be true to its specification - it tries to invoke next cleanup action for which GC has determined that its monitored object is phantom reachable without waiting for ReferenceHandler thread to transfer it from pending list to the queue.
>>
>> Since Reference.tryHandlePending is just transfering Reference objects from one list to another and does that using two locks (the Reference.lock and the ReferenceQueue.lock) but never holds them both together or calls any outside code while holding any of the locks, there's no danger of dead-locking, if that was your concern.
>>
>> Regards, Peter
> ------------------------------------------------------------------------------
> src/java.base/share/classes/java/lang/ref/Cleaner.java
>   242     public boolean cleanNextPending() {
>   243         while (!impl.cleanNextEnqueuedCleanable()) {
>   244             if (!Reference.tryHandlePending(false)) {
>   245                 return false;
>   246             }
>   247         }
>   248         return true;
>   249     }
>
> This can loop for an arbitrarily long time if there are many pending
> references that are unrelated to the invoking Cleaner.  It could even
> loop arbitrarily long if there are lots of pending references and only
> a few scattered ones are for the invoking Cleaner, because the
> Cleaner's thread might take care of each of them before the helping
> thread re-checks the queue.

That could only happen if the rate of Reference discovery by STW GC 
phases was greater than the processing in this loop so that the loop 
could never drain the Reference.pending list. But that's a good point.

>
> The Disposer class that was mentioned as a use-case for this new
> function doesn't try to do anything like that; it just loops until
> there aren't any more in the queue.  The caller there is only affected
> by the number of enqueued objects, not by the number of unrelated
> pending objects.
>
> OTOH, while not structured exactly the same, I think this has a
> similar effect to the existing Direct-X-Buffer support provided by
> Bits.  Helping the single reference processing thread could be useful,
> though contention for the pending list lock could be a problem.  And
> this isn't *quite* the same as what we have now, because the reference
> will be enqueued and the Cleaner will be notified, so that the helper
> and the Cleaner compete to process the queued cleanup; with
> sun.misc.Cleaner the helper just directly invokes the cleanup.
>
> I'm not sure what to do about any of this; I'm just feeling like maybe
> the proposed API / implementation isn't quite right yet.

sun.misc.Cleaner was executed directly by the ReferenceHandler thread or 
whichever thread was helping it by invoking 
Reference.tryHandlePending(). Such helping was beneficial in nio Bits 
for two reasons:
- the cleanup work was done by more threads. Deallocating native memory 
does take some time > 0 as I have found out.
- some work was done synchronously in the allocating thread, so 
re-attempts to reserve native memory were scheduled immediately after 
releasing some memory which increased fairness and chances of success in 
spite of multiple threads trying to reserve concurrently.

If we remove special casing for sun.misc.Cleaner, then ReferenceHandler 
thread is left with just swapping pointers - no actual cleanup work. So 
I thought, maybe we could manage without helping it and just help the 
new Cleaner thread dequeue and execute Cleanables. This mostly works for 
4 concurrent allocating threads on my 4-core i7 PC (using 
DirectBufferAllocTest) which is better than nothing 
(DirectBufferAllocTest reliably fails with 2 allocating threads when not 
helping the Cleaner thread). But when overcommitting the 4 cores with 8 
or more allocating threads and helping just the Cleaner thread, the test 
still fails.

So I created another variant where I used a combined approach:

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

I kept the public boolean Cleaner.cleanNextPending() method which now 
only deals with enqueued Cleanable(s). I think this method might still 
be beneficial for public use in situations where cleanup actions take 
relatively long time to execute so that the rate of cleanup falls behind 
the rate of registration of new cleanup actions.

I resurrected the JavaLangRefAccess interface to patch into the 
ReferenceHandler thread processing from nio Bits, but I redesigned the 
logic. The redesigned method Reference.enqueuePendingReferences() takes 
a snapshot of current pending references in one go and enqueues them all 
without holding or reobtaining the Reference.lock. In addition, it waits 
for concurrent threads that took a snapshot of pending references before 
that to finish enqueuing them. The method therefore guarantees that all 
Reference(s) discovered by the time the method was called, are enqueued 
before returning.

Combining this with a fair lock in Bits.reserveMemory() taken after 1st 
optimistic attempt to reserve memory fails, works quite good. There's no 
sleeping necessary in Bits.reserveMemory and the DirectBufferAllocTest 
passes no matter how many allocating threads are involved. The 
throughput of allocation+deallocation of direct buffers is at least as 
good as before or even better as shown in this JMH benchmark test:

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

So what do you think of this variant?

>
> ------------------------------------------------------------------------------
> src/java.base/share/classes/java/nio/Direct-X-Buffer.java
>    34 import jdk.internal.ref.CleanerFactory;
> &etc.
>
> It might make sense to give Direct-X-Buffer its own dedicated Cleaner,
> rather than sharing the jdk.internal Cleaner.
>
> ------------------------------------------------------------------------------
>

Yes, it might make sense because direct buffers can be a heavy user. 
Each Cleaner uses it's own background thread though, so the number of 
JDK threads would increase. I wonder if it is possible to redesign the 
internals of ReferenceQueue so that a single thread could remove 
elements from multiple queues in a fair (round-robin) fashion. In such 
case, multiple Cleaners could share a thread but still enable draining 
just a chosen queue from the outside...

Regards, Peter