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

[Peter Levart]

Hi Mandy and others,

After jake integration into jdk9-dev and a fix for 
NativeBuffer/InnocuousThread bootstrap issue that arose after part1 of 
this patch was pushed, which is planned for next week, we can continue 
with discussion about part2 of this patch - the DirectByteBuffer part.

I have prepared another variant which is similar to webrev.07.part2, 
with a twist - the DBB allocating thread does not help the 
ReferenceHandler thread to enqueue pending Reference(s) any more, but 
just synchronizes with it and blocks until the ReferenceHandler thread 
finishes its job. Hopefully this is more robust, since external threads 
don't modify any state so any possible asynchronous exception thrown by 
the external thread can not affect the correct processing of 
ReferenceHandler thread. Everything else is mostly unchanged apart from 
the "optimistic" 1st try of direct memory reservation in nio Bits. In 
webrev.07 this reservation was opportunistic. Threads could barge and 
steal the memory from the thread holding the lock, retrying reservation 
and helping with cleaning so that the later thread could fail and throw 
OOME while there were many uncleaned and unreferenced DBB(s) left.

I have observed such occasional failure when running the 
DirectBufferAllocTest for extended period of time with many allocating 
threads (16 or 32 threads on 4-core CPU). The "optimistic" 1st 
reservation attempt in webrev.08 is now guarded with a non-blocking 
attempt to obtain a shared (READ) lock, while the reservation retries + 
helping is guarded by an exclusive (WRITE) mode of the same lock. This 
prevents barging. I haven't been able to observe a test failure with 
this arrangement and the throughput is still good.

Here's the webrev:

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

So take your time and review at your convenience.

Regards, Peter


On 03/09/2016 10:16 PM, Peter Levart wrote:
> Hi Mandy, Chris, Kim, Roger and others,
>
> Hearing no objections for a day, two Reviewers saying it looks ok and 
> successfully re-running the tests, I pushed webrev.07.part1 to jdk9-dev.
>
> Thanks for reviews and comments.
>
> Now to the 2nd part...
>
> On 03/07/2016 07:35 PM, Mandy Chung wrote:
>> ...
>>> And here's the 2nd part that applies on top of part 1:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.07.part2/
>>>
>>>
>>> Together they form functionally equivalent change as in webrev.06priv with only two additional cosmetic changes to part 2 (renaming of method Cleaner.cleanNextPending -> Cleaner.cleanNextEnqueued and removal of an obsolete comment in nio Bits).
>>>
>> I studied webrev.06priv and the history of JDK-6857566.
>>
>> I’m not comfortable for any arbitrary thread to handle the enqueuing of the pending references (this change is more about the fix for JDK-6857566).
>
> Why? A Thread is a Thread is a Thread... When legacy Cleaner is 
> removed, ReferenceHandler thread will be left with swapping pointers 
> only - no custom code will be involved. The only things I can think of 
> against using arbitrary thread are:
>
> - the thread could have lower priority than RaferenceHandler thread, 
> so stealing a chunk of references from the pending list and enqueueing 
> them in low-priority thread might lead to reduced throughput.
> - the thread could have used almost all of it's stack before calling 
> the ByteBuffer.allocateDirect() so there's a danger of 
> StackOverflowError(s) when executing the sections of 
> Reference.enqueuePendingReferences() method, loosing in effect a chunk 
> of pending References that have been unhooked from the pending list.
>
> If this is what you are concerned about then maybe we just need a way 
> to synchronize with ReferenceHandler thread to wait until it enqueues 
> all pending references discovered by the time the synchronization was 
> requested, but otherwise not do any enqueuing... I'll think about that 
> approach.
>
>>   I like your proposed change to take over handling the whole chain of pending references at once.  The unhookPhase and enqueuePhase add the complexity that I think we can avoid.
>
> That's necessary in my approach for the synchronization with threads 
> that do the concurrent enqueueing (ReferenceHandler thread, for 
> example). For example, a thread that comes before us and unhooks a 
> chunk of pending references might still be enqueuing them while we 
> discover that the pending list is empty and think that all references 
> discovered so far have already been enqueued. We must wait for them to 
> be enqueued before continuing. Reference.enqueuePendingReferences() is 
> meant to be called in pair right after System.gc():
>
> System.gc(); // discover Reference(s)
> Reference.enqueuePendingReferences(); // enqueue Reference(s) 
> discovered by System.gc() above
> // if ReferenceHandler thread steals a chunk of pending references 
> before us,
> // we must wait for ReferenceHandler thread to enqueue them before 
> continuing...
>
> // ... now see what is enqueued...
>
>> I’m okay for only system's cleaner thread to help the reference handler thread doing its job.  Would you consider having the special cleaner thread to help the enqueuing before waiting on the cleaner's ReferenceQueue?
>
> As I explained to Kim, the trick is not as much about helping than it 
> is about synchronizing with ReferenceHandler thread. The allocating 
> thread must know when all References discovered up to a certain point 
> in time are processed before giving up with OOME. If it helps 
> processing or not is of 2nd importance. It can help if that improves 
> throughput but needs not.
>
>> The allocating thread may do a System.gc() that may discover phantom reachable references.  All it’s interested is only the direct byte buffer ones so that it can deallocate the native memory.  What is the downside of having a dedicated Cleaner for direct byte buffer that could special case for it?
>
> A dedicated Cleaner for direct buffers might be a good idea if other 
> uses of shared Cleaner in JDK become heavy. So that helping process 
> Cleanable(s) does not involve other unrelated Cleanable(s). But it 
> comes with a price of another dedicated background thread.
>
> So let me think about these things for a little more. I'll try to 
> address above concerns.
>
> Regards, Peter
>
>