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

[Peter Levart]

Hi Mandy, Roger,

On 04/01/2016 06:07 AM, Mandy Chung wrote:
> Hi Peter,
>
> I found this thread has grown to discuss several relevant issues that can be separated.
> 1. Assist cleaner to deallocate direct byte buffer (JDK-6857566)
> 2. Replace direct byte buffer with java.lang.ref.Cleaner
> 3. java.lang.ref.Cleaner be an interface vs final class
> 4. Pending reference list lock (JDK-8055232)
>
> Roger has to speak for #3 which I don’t think he comments on that.  For #4, working out the VM and library new interface to transfer the pending references definitely has to take more time and prototype.  I’m interested in #4 but not sure if this can target in JDK 9 (given that FC in May).
>
> I’d like to address #1 to have the allocating thread to invoke cleaning actions to free native memory rather than any cleaning action.  #2 is not as critical in my opinion while it’d be nice to get to.  One possible option to move forward is to keep Cleaner as is and keep java.nio.Bits to invoke cleaning actions, i.e. webrev.08.part2 except that CleanerFactory will have two special cleaners - one for native memory cleaning and the other for anything else (there isn’t any client in JDK yet).  We will see what Panama would provide for timely deallocation and we could replace the fix in Bits with that when’s available.
>
> My comments inlined below that are related #1 and #2.
>
>> On Mar 28, 2016, at 10:18 AM, Peter Levart <peter.levart at gmail.com> wrote:
>>
>>
>> But first, let me reply to Mandy's comments...
>>
>>
>>> My uncomfort was the fix for JDK-6857566 - both enqueuing pending ref and invoking the cleaning code in an arbitrary thread.
>>>
>>> Looking at it again - enqueuing the pending reference is not so much of a concern (simply updating the link) but the common cleaner could be used by other code that may only expect to be invoked in system thread that’s still my concern (thinking of thread locals).
>>>
>> As you'll see in the webrev below, enqueueing is performed solely be ReferenceHandler thread. Allocating thread(s) just wait for it to do its job. There's a little synchronization action performed at the end of enqueueing a chunk of pending references that notifies waiters (allocating threads) so that they can continue. This actually improves throughput (compared to helping enqueue Reference(s) one by one) because there's not much actual work to be done (just swapping pointers) so synchronization dominates. The goal here is to minimize synchronization among threads and by executing enqueuing of the whole bunch of pending references in private by a single thread achieves a reduction in synchronization when lots of Reference(s) are discovered at once - precisely the situation when it matters.
>>
> I understand this and have no issue with this.
>
>> OTOH helping the Cleaner thread is beneficial as cleanup actions take time to execute and this is the easiest way to retry allocation while there's still chance it will succeed. As the common Cleaner is using InnocuousThread, cleanup actions can't rely on any thread locals to be preserved from invocation to invocation anyway - they are cleared after each cleanup action so each action gets empty thread locals. We could simulate this in threads that help execute cleanup actions by saving thread-locals to local variables, clearing thread-locals, executing cleanup action and then restoring thread-locals from local variables. Mandy, if you think this is important I'll add such save/clear/restore code to appropriate place.
> I’m comfortable of running unsafe::freeMemory in allocating thread.  That’s why I propose to have a specific cleaner for native memory allocation use that seems to be the simplest approach (but if it turns out changing to Cleaner as as interface - it’s a different question).  I can’t speak for NIO if we want to put save/clear/restore logic in java.nio.Bits.
>
> Mandy

Right, so let's focus on issue #1 for now. You would agree that to 
separate cleaning actions for DirectByteBuffers from other cleaning 
actions in the system, there has to be a special ReferenceQueue for 
them, therefore a special Cleaner instance. If we keep 
jdk.internal.ref.Cleaner (old sun.misc.Cleaner) just for 
DirectByteBuffer then we get similar guarantee - helping 
ReferenceHandler thread would just execute old Cleaners that deallocate 
or unmap DirectByteBuffer(s) and no other(s). But that would also keep 
helping threads enqueue other Reference(s) one by one and 
ReferenceHandler would keep executing old Cleaner(s) which is 
troublesome because of unresolved problems with OOME(s). It would also 
be harder to resolve a subtle dilemma described below...

So I'm going to propose a solution for #1 while still keeping the rest 
of webrev unchanged for now and will try to address other issuer later. 
Here's new webrev:

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

...the only change from webrev.11.part2 is in CleanerFactory and 
Direct-X-Buffer template. There are now two lazily initialized Cleaner 
instances. A dbbCleaner() which is used only for DirectByteBuffer(s) and 
has extended functionality and a common cleaner() which is used for all 
other purposes in JDK and doesn't have the extended functionality.

The subtle dilemma here is whether to use the dbbCleaner() for all 
DirectByteBuffer(s) or just for DirectByteBuffer(s) that are created by 
allocating the native memory and not by mapping it. Only the former 
DBB(s) use Bits.[un]reserveMemory to limit usage of direct memory. In 
the webrev above I used dbbCleaner() for both types of DBB(s) so DBB 
allocating threads also help unmap DBB(s). It is easy to separate those 
two types and use dbbCleaner() for allocated DBB(s) and common cleaner() 
for mapped DBB(s) because DirectByteBuffer has two distinct constructors 
for the two usages and the two cleaners share a common Cleaner.Cleanable 
type. Such easy separation would not be possible if you wanted to use 
old jdk.internal.ref.Cleaner for allocated DBBs and the new 
java.lang.ref.Cleaner for mapped DBBs.

I think that with above approach where helping threads are limited to 
DirectByteBuffer.Deallocator(s) and FileChannelImpl.Unmapper(s) (or only 
the former if desired) is quite safe. To guarantee (if supported by VM) 
that helping thread either executes the whole cleaning operation or no 
part of it, I added @ReservedStackAccess annotation on the critical 
CleanerImpl.Task.cleanNextEnqueued() method which is called by helping 
threads.

So, what do you think about this webrev from the issue #1 perspective?

@Roger:

I thought about how to instead of helping the background Cleaner thread, 
make allocation threads just synchronize with cleaner thread but I 
haven't yet come up with an elegant way that would not include lots of 
additional synchronization which would represent overhead in normal 
operation too. So if helping is limited to DBBs like suggested above, do 
you still have any concerns?

About entanglement between nio Bits and 
ExtendedCleaner.retryWhileHelpingClean(). It is the same level of 
entanglement as between the DirectByteBuffer constructor and 
Cleaner.register(). In both occasions an action is provided to the 
Cleaner. Cleaner.register() takes a cleanup action and 
ExtendedCleaner.retryWhileHelpingClean() takes a retriable "allocating" 
or "reservation" action. "allocation" or "reservation" is the opposite 
of cleanup. Both methods are encapsulated in the same object because 
those two functions must be coordinated. So I think that collocating 
them together makes sense. What do you think?

Regards, Peter