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
