Hi Mandy, Roger,
On 04/01/2016 06:07 AM, Mandy Chung wrote:
Hi Peter,
I found this thread has grown to discuss several relevant issues that can be
separated.
1. Assist cleaner to deallocate direct byte buffer (JDK-6857566)
2. Replace direct byte buffer with java.lang.ref.Cleaner
3. java.lang.ref.Cleaner be an interface vs final class
4. Pending reference list lock (JDK-8055232)
Roger has to speak for #3 which I don’t think he comments on that. For #4,
working out the VM and library new interface to transfer the pending references
definitely has to take more time and prototype. I’m interested in #4 but not
sure if this can target in JDK 9 (given that FC in May).
I’d like to address #1 to have the allocating thread to invoke cleaning actions
to free native memory rather than any cleaning action. #2 is not as critical
in my opinion while it’d be nice to get to. One possible option to move
forward is to keep Cleaner as is and keep java.nio.Bits to invoke cleaning
actions, i.e. webrev.08.part2 except that CleanerFactory will have two special
cleaners - one for native memory cleaning and the other for anything else
(there isn’t any client in JDK yet). We will see what Panama would provide for
timely deallocation and we could replace the fix in Bits with that when’s
available.
My comments inlined below that are related #1 and #2.
On Mar 28, 2016, at 10:18 AM, Peter Levart <peter.lev...@gmail.com> wrote:
But first, let me reply to Mandy's comments...
My uncomfort was the fix for JDK-6857566 - both enqueuing pending ref and
invoking the cleaning code in an arbitrary thread.
Looking at it again - enqueuing the pending reference is not so much of a
concern (simply updating the link) but the common cleaner could be used by
other code that may only expect to be invoked in system thread that’s still my
concern (thinking of thread locals).
As you'll see in the webrev below, enqueueing is performed solely be
ReferenceHandler thread. Allocating thread(s) just wait for it to do its job.
There's a little synchronization action performed at the end of enqueueing a
chunk of pending references that notifies waiters (allocating threads) so that
they can continue. This actually improves throughput (compared to helping
enqueue Reference(s) one by one) because there's not much actual work to be
done (just swapping pointers) so synchronization dominates. The goal here is to
minimize synchronization among threads and by executing enqueuing of the whole
bunch of pending references in private by a single thread achieves a reduction
in synchronization when lots of Reference(s) are discovered at once - precisely
the situation when it matters.
I understand this and have no issue with this.
OTOH helping the Cleaner thread is beneficial as cleanup actions take time to
execute and this is the easiest way to retry allocation while there's still
chance it will succeed. As the common Cleaner is using InnocuousThread, cleanup
actions can't rely on any thread locals to be preserved from invocation to
invocation anyway - they are cleared after each cleanup action so each action
gets empty thread locals. We could simulate this in threads that help execute
cleanup actions by saving thread-locals to local variables, clearing
thread-locals, executing cleanup action and then restoring thread-locals from
local variables. Mandy, if you think this is important I'll add such
save/clear/restore code to appropriate place.
I’m comfortable of running unsafe::freeMemory in allocating thread. That’s why
I propose to have a specific cleaner for native memory allocation use that
seems to be the simplest approach (but if it turns out changing to Cleaner as
as interface - it’s a different question). I can’t speak for NIO if we want to
put save/clear/restore logic in java.nio.Bits.
Mandy
Right, so let's focus on issue #1 for now. You would agree that to
separate cleaning actions for DirectByteBuffers from other cleaning
actions in the system, there has to be a special ReferenceQueue for
them, therefore a special Cleaner instance. If we keep
jdk.internal.ref.Cleaner (old sun.misc.Cleaner) just for
DirectByteBuffer then we get similar guarantee - helping
ReferenceHandler thread would just execute old Cleaners that deallocate
or unmap DirectByteBuffer(s) and no other(s). But that would also keep
helping threads enqueue other Reference(s) one by one and
ReferenceHandler would keep executing old Cleaner(s) which is
troublesome because of unresolved problems with OOME(s). It would also
be harder to resolve a subtle dilemma described below...
So I'm going to propose a solution for #1 while still keeping the rest
of webrev unchanged for now and will try to address other issuer later.
Here's new webrev:
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.12.part2/
...the only change from webrev.11.part2 is in CleanerFactory and
Direct-X-Buffer template. There are now two lazily initialized Cleaner
instances. A dbbCleaner() which is used only for DirectByteBuffer(s) and
has extended functionality and a common cleaner() which is used for all
other purposes in JDK and doesn't have the extended functionality.
The subtle dilemma here is whether to use the dbbCleaner() for all
DirectByteBuffer(s) or just for DirectByteBuffer(s) that are created by
allocating the native memory and not by mapping it. Only the former
DBB(s) use Bits.[un]reserveMemory to limit usage of direct memory. In
the webrev above I used dbbCleaner() for both types of DBB(s) so DBB
allocating threads also help unmap DBB(s). It is easy to separate those
two types and use dbbCleaner() for allocated DBB(s) and common cleaner()
for mapped DBB(s) because DirectByteBuffer has two distinct constructors
for the two usages and the two cleaners share a common Cleaner.Cleanable
type. Such easy separation would not be possible if you wanted to use
old jdk.internal.ref.Cleaner for allocated DBBs and the new
java.lang.ref.Cleaner for mapped DBBs.
I think that with above approach where helping threads are limited to
DirectByteBuffer.Deallocator(s) and FileChannelImpl.Unmapper(s) (or only
the former if desired) is quite safe. To guarantee (if supported by VM)
that helping thread either executes the whole cleaning operation or no
part of it, I added @ReservedStackAccess annotation on the critical
CleanerImpl.Task.cleanNextEnqueued() method which is called by helping
threads.
So, what do you think about this webrev from the issue #1 perspective?
@Roger:
I thought about how to instead of helping the background Cleaner thread,
make allocation threads just synchronize with cleaner thread but I
haven't yet come up with an elegant way that would not include lots of
additional synchronization which would represent overhead in normal
operation too. So if helping is limited to DBBs like suggested above, do
you still have any concerns?
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?
Regards, Peter
