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

[Peter Levart]

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
>>>
>>
>