Hi Peter,
Stepping back just a bit.
The old Cleaner running on the Reference processing thread had a few (2)
very well controlled
functions, reference processing and deallocating DirectByteBuffers.
Maybe we can't do too much
better than that.
The old worst case performance/latency wise was the reference processing
thread
did the work and the allocating thread did very little synchronizing and
just did the retries.
In the best case, all the real work was done by the allocating thread,
if the interactions with GC
work out perfectly. But it was still the case that the buffer
alloc/dealloc throughput was
met with the division of work separating the reference processing thread
and the allocating thread.
The function that can only be provided by CleanerImpl / Reference
processing thread state is
knowledge that the cleaning queue is empty.
The helping functions were/are a bit troublesome because of mixing
execution
environments of the thread allocating direct buffers and the cleanables
and it seemed that
more than a little complexity was needed to compensate.
If the bottleneck in processing is between the reference processing and
cleanup
then it should be ok (based on previous comments) for the CleanerImpl to
help with
reference processing (after it has an empty queue and before it blocks
waiting or in every loop).
Though if you already tried this combination, I don't recall the results.
As you pointed out it would be more efficient if the allocating thread
could be aware
when it was known there was nothing ready to cleanup so it can retry and
invoke GC or
throw out of memory if appropriate.
Adding a method that returned the count of completed cleaning cycles (or
similar)
to CleanerImpl could exist with a minimal of coupling and still provide
the information needed without commingling the execution threads.
I don't see the need to change Cleaner to an interface to be able to provide
an additional method on CleanerImpl or a subclass and a factory method
could
provide for a clean and very targeted interface to Bits/Direct buffer.
I'm sorry I haven't had time to try out concretely what I have in mind.
Please correct or remind me of missing salient considerations.
Thanks, Roger
On 4/2/2016 7:24 AM, Peter Levart wrote:
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
