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
