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

[Roger Riggs]

Hi Peter,

Stepping back just a bit.

The old Cleaner running on the Reference processing thread had a few (2) 
very well controlled
functions, reference processing and deallocating DirectByteBuffers. 
Maybe we can't do too much
better than that.

The old worst case performance/latency wise was the reference processing 
thread
did the work and the allocating thread did very little synchronizing and 
just did the retries.
In the best case, all the real work was done by the allocating thread, 
if the interactions with GC
work out perfectly.  But it was still the case that the buffer 
alloc/dealloc throughput was
met with the division of work separating the reference processing thread 
and the allocating thread.

The function that can only be provided by CleanerImpl / Reference 
processing thread state is
knowledge that the cleaning  queue is empty.
The helping functions were/are a bit troublesome because of mixing 
execution
environments of the thread allocating direct buffers and the cleanables 
and it seemed that
more than a little complexity was needed to compensate.

If the bottleneck in processing is between the reference processing and 
cleanup
then it should be ok (based on previous comments) for the CleanerImpl to 
help with
reference processing (after it has an empty queue and before it blocks 
waiting or in every loop).
Though if you already tried this combination, I don't recall the results.

As you pointed out it would be more efficient if the allocating thread 
could be aware
when it was known there was nothing ready to cleanup so it can retry and 
invoke GC or
throw out of memory if appropriate.
Adding a method that returned the count of completed cleaning cycles (or 
similar)
to CleanerImpl could exist with a minimal of coupling and still provide
the information needed without commingling the execution threads.

I don't see the need to change Cleaner to an interface to be able to provide
an additional method on CleanerImpl or a subclass and a factory method 
could
provide for a clean and very targeted interface to Bits/Direct buffer.

I'm sorry I haven't had time to try out concretely what I have in mind.
Please correct or remind me of missing salient considerations.

Thanks, Roger


On 4/2/2016 7:24 AM, Peter Levart wrote:
> Hi Roger,
>
> Thanks for looking at the patch.
>
> On 04/02/2016 01:31 AM, Roger Riggs wrote:
>> Hi Peter,
>>
>> I overlooked the introduction of another nested class (Task) to 
>> handle the cleanup.
>> But there are too many changes to see which ones solve a single problem.
>>
>> Sorry to make more work, but I think we need to go back to the 
>> minimum necessary
>> change to make progress on this. Omit all of the little cleanups 
>> until the end
>> or do them first and separately.
>>
>> Thanks, Roger
>
> No Problem. I understand. So let's proceed in stages. Since part1 is 
> already pushed, I'll call part2 stages with names: part2.1, part2.2, 
> ... and I'll start counting webrev revisions from 01 again, so webrev 
> names will be in the form: webrev.part2.1.rev01. Each part will be an 
> incremental change to the previous one.
>
> part2.1: This is preparation work to be able to have an extended 
> java.lang.ref.Cleaner type for internal use. Since 
> java.lang.ref.Cleaner is a final class, I propose to make it an 
> interface instead:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.part2.1.rev01/
>
> This is a source-compatible change and it also simplifies 
> implementation (no injection of Cleaner.impl access function into 
> CleanerImpl needed any more). What used to be java.lang.ref.Cleaner is 
> renamed to jdk.internal.ref.CleanerImpl. What used to be 
> jdk.internal.ref.CleanerImpl is now a nested static class 
> jdk.internal.ref.CleanerImpl.Task (because it implements Runnable). 
> Otherwise nothing has changed in the overall architecture of the 
> Cleaner except that public-facing API is now an interface instead of a 
> final class. This allows specifying internal extension interface and 
> internal extension implementation.
>
> CleanerTest passes with this change.
>
> So what do you think?
>
> Regards, Peter
>
>>
>>
>>
>>
>> On 4/1/16 5:51 PM, Roger Riggs wrote:
>>> Hi Peter,
>>>
>>> Thanks for the diffs to look at.
>>>
>>> Two observations on the changes.
>>>
>>> - The Cleaner instance was intentionally and necessarily different 
>>> than the CleanerImpl to enable
>>> the CleanerImpl and its thread to terminate if the Cleaner is not 
>>> longer referenced.
>>> Folding them into a single object breaks that.
>>>
>>> Perhaps it is not too bad for ExtendedCleaner to subclass 
>>> CleanerImpl with the cleanup helper/supplier behavior
>>> and expose itself to Bits. There will be fewer moving parts. There 
>>> is no need for two factory methods for
>>> ExtendedCleaner unless you are going to use  a separate ThreadFactory.
>>>
>>> - The Deallocator (and now Allocator) nested classes are identical, 
>>> and there is a separate copy for each
>>> type derived from the Direct-X-template.  But it may not be worth 
>>> fixing until the rest of it is settled to avoid
>>> more moving parts.
>>>
>>> I don't have an opinion on the code changes in Reference, that's 
>>> different kettle of fish.
>>>
>>> More next week.
>>>
>>> Have a good weekend, Roger
>>>
>>>
>>> On 4/1/2016 12:46 PM, Peter Levart wrote:
>>>>
>>>>
>>>> On 04/01/2016 06:08 PM, Peter Levart wrote:
>>>>>
>>>>>
>>>>> On 04/01/2016 05:18 PM, Peter Levart wrote:
>>>>>> @Roger:
>>>>>>
>>>>>> ...
>>>>>>
>>>>>> 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?
>>>>>
>>>>> ...to illustrate what I mean, here's a variant that totally 
>>>>> untangles Bits from Cleaner and moves the whole Cleaner 
>>>>> interaction into the DirectByteBuffer itself:
>>>>>
>>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.13.part2/ 
>>>>>
>>>>>
>>>>> Notice the symmetry between Cleaner.retryWhileHelpingClean : 
>>>>> Cleaner.register and Allocator : Deallocator ?
>>>>>
>>>>>
>>>>> Regards, Peter
>>>>>
>>>>
>>>> And here's also a diff between webrev.12.part2 and webrev.13.part2:
>>>>
>>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.diff.12to13.part2/ 
>>>>
>>>>
>>>> Regards, Peter
>>>>
>>>
>>
>