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.lev...@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
