subject:"RFR\: JDK\-8149925 We don't need jdk.internal.ref.Cleaner any more"

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-05-16 Thread Peter Levart


Hi Martin,

As I have been studying the CleanerTest too recently, let me comment on 
some questions...


On 05/09/2016 06:34 PM, Martin Buchholz wrote:

---

 Assert.assertEquals(map.get(k2), data, "value should be found
in the map");
 key = null;
 System.gc();
 Assert.assertNotEquals(map.get(k2), data, "value should not be
found in the map");

I initially expected this to fail at least intermittently because a
cleaner may not yet have removed the entry from the map.


...The cleaner need not remove the entry from the map in order for this 
code to succeed. The sufficient condition is that GC discovers the k1 
and k2 WeakReference(s) and clears them. When a WeakKey is cleared it is 
only equal to itself since the referent is gone (see equals() method). 
So it is important to ensure that the referent remains reachable at 
least until the 1st assert above (perhaps by inserting 
Reference.reachabilityFence(key) just after it)...




Why aren't we calling whitebox.fullGC() everywhere where System.gc()
is being called?

What's particularly confusing here is that the WeakKey is not actually
surely removed from the map, but instead two WeakKeys that may have
been equal become non-equal when they get cleared by the gc.  Which is
generally bad practice.  Which suggests we wouldn't implement a real
public concurrent map of weak keys this way.


Why not? We don't need to lookup the value after the WeakKey(s) are 
cleared, since by definition, we can't reach their referent any more 
(the 'key'). But the instance of the WeakKey that is inserted in the map 
can still be removed because it is the same instance that is referenced 
by the WeakCleanable and a cleared WeakKey is still equal to itself. So 
we have the following invariants:


- while the key (the referent) is still reachable, multiple WeakKeys 
with the same referent are equal among themselves so a WeakKey can be 
constructed that is equal to some other WeakKey in the map.
- after the key (the referent) becomes weakly-reachable and GC kicks-in, 
all WeakReferences with referents from the selected set are cleared 
atomically, including WeakKey(s) and WeakCleanable(s) refering to the 
same key referent, so only the instance of the WeakKey that is inserted 
in the map can be used to remove the stale entry from it.


You can find a simpler implementation of WeakKey in my "Cleaner cleanup" 
proposal where WeakKey is a subclass of WeakCleanable and simply removes 
itself from the map when its clean() method is called by the Cleaner 
thread...




---

WeakCleanable and friends are defined in jdk.internal, don't appear to
be used at all, and since there are fields in CleanerImpl, they impose
a hidden tax on all users of Cleaner.  I don't see why there is both
WeakCleanable (using subclassing) and WeakCleanableRef (using a
provided Runnable).  Is a compelling use case for WeakCleanables
forthcoming?


I have a few ideas that I would like to present, but I first wanted to 
propose making [Soft|Weak]Cleanable(s) even more low-level to make those 
use cases feasible. See the "Cleaner cleanup" thread for more info on that.



Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-05-09 Thread Martin Buchholz

On Mon, May 9, 2016 at 11:10 AM, Roger Riggs  wrote:
> I'm not sure I understand what
>
> "a CompletableFuture-based API for cleanup actions."
>
> would look like or how it would be used.

I'm fuzzy myself, but the idea is that any asynchronous action that
will be completed in the future should be represented by a
CompletableFuture.  In this case, the asynchronous actions are
performed by the GC.  I imagine an API on e.g. WeakReference

CompletableFuture whenCleared()

and then a user can schedule follow-on actions in the executor of their choice.

WeakReference r = ...
r.whenCleared().thenRunAsync(...)

One idea is to throw away as much Cleaner machinery as possible to
reuse CompletableFuture machinery.  Or build on top...  All
concurrency machinery is hard for humans to understand.

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-05-09 Thread Roger Riggs


Hi Martin,

I added a link to these notes to the JDK-8144531 
 Cleanup of Cleaner 
implementation.


I'm not sure I understand what

"a CompletableFuture-based API for cleanup actions."

would look like or how it would be used.

Thanks, Roger


On 5/9/2016 12:34 PM, Martin Buchholz wrote:

I haven't been following along with the long review thread, but I took
a look at the new Cleaner stuff and have some belated comments:

---

Please add missing semicolon:
  *private final Cleaner.Cleanable cleanable

---

The below looks non-sensical; maybe should return EV_CLEAR if some
clearing, not cleaning, should occur?

  * @return EV_CLEAR if the cleanup should occur;
  * EV_CLEAN if the cleanup should occur;

---

 private final int hash;
 private final ConcurrentHashMap map;
 Cleaner.Cleanable cleanable;

Why isn't cleanable also private final?

---

 Assert.assertEquals(map.get(k2), data, "value should be found
in the map");
 key = null;
 System.gc();
 Assert.assertNotEquals(map.get(k2), data, "value should not be
found in the map");

I initially expected this to fail at least intermittently because a
cleaner may not yet have removed the entry from the map.

Why aren't we calling whitebox.fullGC() everywhere where System.gc()
is being called?

What's particularly confusing here is that the WeakKey is not actually
surely removed from the map, but instead two WeakKeys that may have
been equal become non-equal when they get cleared by the gc.  Which is
generally bad practice.  Which suggests we wouldn't implement a real
public concurrent map of weak keys this way.

---

WeakCleanable and friends are defined in jdk.internal, don't appear to
be used at all, and since there are fields in CleanerImpl, they impose
a hidden tax on all users of Cleaner.  I don't see why there is both
WeakCleanable (using subclassing) and WeakCleanableRef (using a
provided Runnable).  Is a compelling use case for WeakCleanables
forthcoming?

---

I keep vaguely hoping to see a CompletableFuture-based API for cleanup actions.

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-05-09 Thread Martin Buchholz

I haven't been following along with the long review thread, but I took
a look at the new Cleaner stuff and have some belated comments:

---

Please add missing semicolon:
 *private final Cleaner.Cleanable cleanable

---

The below looks non-sensical; maybe should return EV_CLEAR if some
clearing, not cleaning, should occur?

 * @return EV_CLEAR if the cleanup should occur;
 * EV_CLEAN if the cleanup should occur;

---

private final int hash;
private final ConcurrentHashMap map;
Cleaner.Cleanable cleanable;

Why isn't cleanable also private final?

---

Assert.assertEquals(map.get(k2), data, "value should be found
in the map");
key = null;
System.gc();
Assert.assertNotEquals(map.get(k2), data, "value should not be
found in the map");

I initially expected this to fail at least intermittently because a
cleaner may not yet have removed the entry from the map.

Why aren't we calling whitebox.fullGC() everywhere where System.gc()
is being called?

What's particularly confusing here is that the WeakKey is not actually
surely removed from the map, but instead two WeakKeys that may have
been equal become non-equal when they get cleared by the gc.  Which is
generally bad practice.  Which suggests we wouldn't implement a real
public concurrent map of weak keys this way.

---

WeakCleanable and friends are defined in jdk.internal, don't appear to
be used at all, and since there are fields in CleanerImpl, they impose
a hidden tax on all users of Cleaner.  I don't see why there is both
WeakCleanable (using subclassing) and WeakCleanableRef (using a
provided Runnable).  Is a compelling use case for WeakCleanables
forthcoming?

---

I keep vaguely hoping to see a CompletableFuture-based API for cleanup actions.

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-06 Thread Roger Riggs


Hi Peter,

Since the current implementation of the old Cleaner and DirectByteBuffer 
works well and there is a
lot of work piling up against the FC date, I'm in favor of keeping it as 
it is until the possibilities
with Panama settle out.  It would best if reclamation was not GC based, 
except as a last resort.


As for the Cleaner API, it is intended to have a specific function and API.
It may be that there is some advantage/optimization in the cleaner 
implementation
at some future point but the API for general use should be concrete and 
specific.
For more specific purposes, especially internal ones like the topic 
here, separate factory methods

would be more appropriate than potential overloading and mix-in interfaces.

Roger


On 4/6/2016 2:43 AM, Peter Levart wrote:

Hi Roger,

On 04/05/2016 04:41 PM, Peter Levart wrote:

On 04/04/2016 11:50 PM, Roger Riggs wrote:
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 would like this to be an instance method so it would naturally 
pertain to a particular Cleaner instance. Or it could be a static 
method that takes a Cleaner instance. One of my previous webrevs did 
have such method on the CleanerImpl, but I was advised to move it to 
Cleaner as a package-private method and expose it via SharedSecrets 
to internal code. I feel such "camouflage" is very awkward now that 
we have modules and other mechanisms exist. So I thought it would be 
most elegant to make Cleaner an interface so it can be extended with 
an internal interface to communicate intent in a type-safe and 
auto-discoverable way. The change to make it interface:


http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.part2.1.rev01/ 



...actually simplifies implementation (33 lines removed in total) and 
could be seen as an improvement in itself.


Are you afraid that if Cleaner was an interface, others would attempt 
to make implementations of it? Now that we have default methods on 
interfaces it is easy to compatibly extend the API even if it is an 
interface so that no 3rd party implementations are immediately 
broken. Are you thinking of security implications when some code is 
handed a Cleaner instance that it doesn't trust? I don't think there 
is a utility for Cleaner instances to be passed from untrusted to 
trusted code, do you?


...even if we don't do anything for DirectByteBuffer(s) in 
java.lang.ref.Cleaner and leave things as they stand (using 
jdk.internal.ref.Cleaner), I would still make this transition to 
interface to enable possible future internal extensions to 
java.lang.ref.Cleaner. As I said, this is a source compatible change, 
but not binary compatible, so if we do it, it must be performed before 
JDK 9 ships.


Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-06 Thread Peter Levart


Hi Roger,

On 04/05/2016 04:41 PM, Peter Levart wrote:

On 04/04/2016 11:50 PM, Roger Riggs wrote:
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 would like this to be an instance method so it would naturally 
pertain to a particular Cleaner instance. Or it could be a static 
method that takes a Cleaner instance. One of my previous webrevs did 
have such method on the CleanerImpl, but I was advised to move it to 
Cleaner as a package-private method and expose it via SharedSecrets to 
internal code. I feel such "camouflage" is very awkward now that we 
have modules and other mechanisms exist. So I thought it would be most 
elegant to make Cleaner an interface so it can be extended with an 
internal interface to communicate intent in a type-safe and 
auto-discoverable way. The change to make it interface:


http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.part2.1.rev01/

...actually simplifies implementation (33 lines removed in total) and 
could be seen as an improvement in itself.


Are you afraid that if Cleaner was an interface, others would attempt 
to make implementations of it? Now that we have default methods on 
interfaces it is easy to compatibly extend the API even if it is an 
interface so that no 3rd party implementations are immediately broken. 
Are you thinking of security implications when some code is handed a 
Cleaner instance that it doesn't trust? I don't think there is a 
utility for Cleaner instances to be passed from untrusted to trusted 
code, do you?


...even if we don't do anything for DirectByteBuffer(s) in 
java.lang.ref.Cleaner and leave things as they stand (using 
jdk.internal.ref.Cleaner), I would still make this transition to 
interface to enable possible future internal extensions to 
java.lang.ref.Cleaner. As I said, this is a source compatible change, 
but not binary compatible, so if we do it, it must be performed before 
JDK 9 ships.


Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-05 Thread Mandy Chung

Hi Peter,

> On Apr 5, 2016, at 7:41 AM, Peter Levart  wrote:
> 
> The bottom line is that we need a mechanism that:
> 
> - triggers reference discovery when native memory limit is approached or 
> reached
> - retires native memory reservation at appropriate time slots until 
> succeeding or until all pending references have been processed and Cleanables 
> executed at which time native memory reservation can fail with OOME.
> - if possible, doesn't execute cleanup functions by the allocating thread but 
> just waits for system threads to do the job.
> - when triggered, does not make native memory allocation a bottleneck.
> 
> I think that what I did in my latest webrevs with ReferenceHandler thread is 
> an improvement in minimizing contended synchronization and interference of 
> allocating thread(s) with Reference enqueue-ing. But interaction of 
> allocating thread(s) with Cleaner background thread could be improved and I 
> have a couple of ideas to explore.

This is about timely native memory deallocation.  Since direct byte buffer is 
the only one using jdk.internal.ref.Cleaner, I am inclined to suggest keep 
jdk.internal.ref.Cleaner as is and replaced it with a better mechanism when 
it’s available, either panama or better pending reference enqueuing that you 
and Per discussed.   Given the time we have, I think it’s likely post JDK 9 
timeframe.

Minimizing contention and interference of allocating threads and 
ReferenceHandler may worth exploring.

Mandy

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-05 Thread Peter Levart


Hi Roger,

On 04/04/2016 11:50 PM, Roger Riggs wrote:

Hi Peter,

Stepping back just a bit.


Right, let's clear up.



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.


...yes, at the beginning, until it was (re)used for other purposes too, 
in: java.lang.ProcessImpl, 
java.lang.invoke.MethodHandleNatives.CallSiteContext, 
jdk.internal.perf.Perf, sun.nio.ch.IOVecWrapper, sun.nio.fs.NativeBuffer 
and sun.java2d.marlin.OffHeapArray.


But those other usages have been converted to use new 
java.lang.ref.Cleaner so old cleaner is now back to basics - 
DirectByteBuffers. And with that, DirectByteBuffers allocating threads 
only help ReferenceHandler thread enqueue References and execute 
DirectByteBuffer deallocators, which is an improvement.


But should we keep that status quo? It's nothing wrong with it as it is, 
except I think we can do better.




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.


The number of retries was exactly the same as the number of References 
helped to be enqueued or in case of Cleaner(s), executed:


// retry while helping enqueue pending Reference objects
// which includes executing pending Cleaner(s) which includes
// Cleaner(s) that free direct buffer memory
while (jlra.tryHandlePendingReference()) {
if (tryReserveMemory(size, cap)) {
return;
}
}

If the share of pending References that are also Cleaners was high, 
chances were higher that not much helping was needed as one cleaned 
DBB.Deallocator could unreserve enough memory for next reservation 
attempt to succeed. So allocating thread helped only until it succeeded 
in reserving the native memory leaving the rest of work to another 
allocating request/thread or to ReferenceHandler.


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.


Yes, whichever thread was quicker. If ReferenceHandler thread had been 
waking up from wait() for a long time, allocating thread could have 
already processed all the References before ReferenceHandler finally 
started to look around. If there were lots of new pending referenced 
discovered, ReferenceHandler thread could finally join the party and 
fight for the same lock...




The function that can only be provided by CleanerImpl / Reference 
processing thread state is

knowledge that the cleaning  queue is empty.


...and that the discovered pending references have actually been 
enqueued before that...


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.


I totally agree.



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.


I don't thing there is a problem because of any bottleneck. And if there 
was a bottleneck we would only have a problem with 
allocation/deallocation throughput and not with OOME(s). The problem is 
because reference discovery is not triggered as a result of native 
memory reservation approaching or reaching the limit. There is no heap 
memory pressure from DirectByteBuffer(s) because they are small objects. 
So a mechanism must be in place that triggers reference discovery and 
waits for discovered references to be processed before failing the 
native memory allocation. A mechanism that tries to simulate what 
happens with GC when there is heap memory pressure. GC guarantees that 
full-GC is executed and heap allocation retried after that before 
finally giving up with OOME. We need a mechanism that attempts to do the 
same for direct memory. Throughput is a nice property but we are not 
directly seeking its improvement. We just not want to make things much 
worse.


Helping the Cleaner thread to process cleanup functions is the easiest 
way to wait for cleanup functions to be processed and for queue to 
drain. Simply because of ReferenceQueue API. If you poll() next 
Reference from the queue and get null, you know the queue is empty, but 
if you get something, you have to execute it and not just ignore it. 
Maybe we could patch into the ReferenceQueue implementation and extend 
its API with an

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-04 Thread Roger Riggs


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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-02 Thread Peter Levart


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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-01 Thread Mandy Chung


> On Apr 1, 2016, at 4:31 PM, 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.
> 

+1.

That’d really help the reviewers. 

thanks
Mandy

> Thanks, Roger
> 
> 
> 
> 
> 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
>>> 
>> 
>

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-01 Thread Roger Riggs

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

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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-01 Thread Roger Riggs

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.

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:

...

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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-01 Thread Peter Levart




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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-01 Thread Peter Levart




On 04/01/2016 05:18 PM, Peter Levart wrote:
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.


Here's also a diff between webrev.11.part2 and webrev.12.part2 for 
easier reviewing:


http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.diff.11to12.part2/

Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-01 Thread Peter Levart




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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-04-01 Thread Peter Levart


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  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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-31 Thread Mandy Chung

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  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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-31 Thread Peter Levart


Hi Roger,

On 03/31/2016 09:12 PM, Roger Riggs wrote:

Hi Peter,

It would be simpler to understand the changes if we solve the problems 
one at a time,

at least for review purposes.


Right. We can focus on one aspect at a time, but I'm still trying to 
keep the whole thing in a working condition at all times...




To your question in the 2nd part about the Cleaner. (webrev.11.part2)

I don't think the communication between the memory reserving thread 
and the unreserving thread
should be mixed into the Cleaner design or implementation.  The logic 
for the communication
between reserveMemory and unreserveMemory methods should be in those 
two methods
and isolated to Bits.java.  I understand the intent for the reserving 
thread to poll for available memory
and it might as well do something useful while it is waiting and get a 
hint about unreserved memory.

But it mixes together the implementations. (too much)


The problem with reserving thread getting information from unreserving 
thread solely by communication implemented in methods reserveMemory and 
unreserveMemory is that this information is not enough. Reserving thread 
must also get information about when all the pending unreservations have 
been performed so that it can either:

- trigger System.gc() to discover fresh pending unreservations, or
- finally give up with OOME

In the absence of this information, all reserving thread can do is 
speculate about this information by observing the timings of 
unreservations happening or not happening - to time out on waiting for 
unreservations to happen. This works (as shown in webrev.10.part2), but 
is not very robust or agile - it introduces unnecessary delays. This 
information is hidden in ReferenceHandler thread (have all pending 
References been enqueued?) and in the Cleaner (has the queue drained out?).


I moved the retrial and helping logic to ExtendedCleaner because I think 
it is reusable for other situations. But if you think it doesn't belong 
to ExtendedCleaner, I can move it back to Bits. We don't strictly need 
to help the Cleaner thread with cleanups. I did it that way because this 
seemed an easy way to communicate the information about the drained 
queue back to allocator thread and to retry reservations at appropriate 
intervals. But let me think about a way to just get this information 
without helping - similarly to what I've done it in ReferenceHandler...




Having an arbitrary thread (the one trying to allocate a DirectBuffer) 
help with the Cleaning
puts an unknown thread perhaps with limited stack or 
AccessControlContext in place to call the
cleaning functions is unappealing at best.  The cleaning functions are 
less predictable than
the Reference enqueuing functions already discussed but are not much 
more complex.
In most cases they are about the complexity of the Deallocator in 
Direct-X-Buffer, etc.


Allocating thread could be "conditioned" before calling the cleanup 
action by:

- saving and clearing thread-locals
- saving and setting AccessControlContext to unprivileged.
...and restoring these back after the action

The problem with unsufficient stack is more difficult solve though. 
Isn't there a new annotation designed to help with that (mainly intended 
for critical sections of java.util.concurrent classes). The problem with 
using it here would be in that we don't know what cleanup action(s) 
might be executed since Cleaner is a general-purpose API and this 
annotation is only designed for parts of code that are known in advance...




Can the pieces be disentangled and still pass the DirectBufferAllocTest?


If we want to get that additional piece of information from 
ReferenceHandler and Cleaner, they must be entangled with Bits, but I 
might be able to loosen this entanglement a bit. Will try these ideas 
tomorrow. Stay tuned.


Regards, Peter



Roger




On 3/28/2016 1:18 PM, Peter Levart wrote:

Hi Mandy, Kim, Per and Roger

I'd like to continue the discussion about the 2nd part of removing 
jdk.internal.ref.Cleaner in this discussion thread.


There was some discussion about whether to synchronize with 
ReferenceHandler thread and wait for it to enqueue the Reference(s) 
or simply detect that there are no more pending Reference(s) by 
timing out on waiting for cleanup actions in discussion thread: "Re: 
Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java 
fails intermittently". Based on that discussion, I have prepared a 
webrev that uses an approach where the detection is performed using 
timeout:


http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.10.part2/

While this webrev passes the DirectBufferAllocTest, I don't have a 
good feeling about this approach since it is not very robust. I can 
imagine situations where it would not behave optimally - it would 
either trigger reference discovery (System.gc()) more frequently that 
necessary or it would cause delays in execution. So I still prefer 
the approach where

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-31 Thread Roger Riggs


Hi Peter,

It would be simpler to understand the changes if we solve the problems 
one at a time,

at least for review purposes.

To your question in the 2nd part about the Cleaner. (webrev.11.part2)

I don't think the communication between the memory reserving thread and 
the unreserving thread
should be mixed into the Cleaner design or implementation.  The logic 
for the communication
between reserveMemory and unreserveMemory methods should be in those two 
methods
and isolated to Bits.java.  I understand the intent for the reserving 
thread to poll for available memory
and it might as well do something useful while it is waiting and get a 
hint about unreserved memory.

But it mixes together the implementations. (too much)

Having an arbitrary thread (the one trying to allocate a DirectBuffer) 
help with the Cleaning
puts an unknown thread perhaps with limited stack or 
AccessControlContext in place to call the
cleaning functions is unappealing at best.  The cleaning functions are 
less predictable than
the Reference enqueuing functions already discussed but are not much 
more complex.
In most cases they are about the complexity of the Deallocator in 
Direct-X-Buffer, etc.


Can the pieces be disentangled and still pass the DirectBufferAllocTest?

Roger




On 3/28/2016 1:18 PM, Peter Levart wrote:

Hi Mandy, Kim, Per and Roger

I'd like to continue the discussion about the 2nd part of removing 
jdk.internal.ref.Cleaner in this discussion thread.


There was some discussion about whether to synchronize with 
ReferenceHandler thread and wait for it to enqueue the Reference(s) or 
simply detect that there are no more pending Reference(s) by timing 
out on waiting for cleanup actions in discussion thread: "Re: Analysis 
on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails 
intermittently". Based on that discussion, I have prepared a webrev 
that uses an approach where the detection is performed using timeout:


http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.10.part2/

While this webrev passes the DirectBufferAllocTest, I don't have a 
good feeling about this approach since it is not very robust. I can 
imagine situations where it would not behave optimally - it would 
either trigger reference discovery (System.gc()) more frequently that 
necessary or it would cause delays in execution. So I still prefer the 
approach where allocating thread(s) explicitly synchronize with 
ReferenceHandler thread and wait for it to enqueue pending 
Reference(s). Luckily this can be performed in an easy way (as I will 
show you shortly). Waiting on discovery of pending references by 
ReferenceHandler thread and handing them to it could be moved to 
native code so that no notification would have to be performed in 
native code from the ReferenceHandler thread to the allocating thread(s).


But first, let me reply to Mandy's comments...


On 03/25/2016 11:20 PM, Mandy Chung wrote:

On Mar 19, 2016, at 7:00 AM, Peter Levart  wrote:

Here's the webrev:

 
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.08.part2/


On 03/07/2016 07:35 PM, Mandy Chung wrote:

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:
:

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.


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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-29 Thread Peter Levart


Hi Per,

On 03/29/2016 04:03 PM, Per Liden wrote:

Hi Peter,

On 2016-03-28 19:18, Peter Levart wrote:
[...]

And now a few words about ReferenceHandler thread and synchronization
with it (for Kim and Per mostly). I think it should not be a problem to
move the following two java.lang.ref.Reference methods to native code if
desired:

 static Reference getPendingReferences(int[] 
discoveryPhaseHolder)

 static int getDiscoveryPhase()

The 1st one is only invoked by a ReferenceHandler thread while the 2nd
is invoked by arbitrary thread. The difference between this and
webrev.09.part2 is that there's no need any more for ReferenceHandler
thread to notify the thread executing the 2nd method and that there's no
need for the 2nd method to perform any waiting. It just needs to obtain
the lock briefly so that it can read the consistent state of two
fields.  Those two fields are Java static fields currently:
Reference.pending & Reference.discoveryPhase and those two methods are
Java methods, but they could be moved to native code if desired to make
the protocol between VM and Java code more robust.

So Kim, Per, what do you think of supporting those 2 methods in native
code? Would that present any problem?


In the best of worlds I'd like the VM to be agnostic to how the 
pending list is processed on the core-libs side. However, after 
looking at it briefly I'm not sure if we can get all they way with 
only providing a getPendingReferences() call.


Anyway, assuming we really need something more than just 
getPendingReferences(), I'm not so keen on exposing a phase counter in 
the API. I think I'd rather have something like this:


/** Get the pending list from the VM, blocking until a list exists.
  * Only used by ReferenceHandler.
  */
Reference getPendingReference();

/** Signal that all references has been enqueued.
  * Only used by ReferenceHandler.
  */
void notifyEnqueuedReferences();

/** If references are pending, wait for a notification from
  * ReferenceHandler that they have been enqueued.
  */
void waitForEnqueuedReferences();


The VM would (roughly) implement this as:


JVM_ENTRY(jobject, JVM_GetPendingReferences(JNIEnv* env))
  // Wait until list becomes non-empty
  {
MonitorLockerEx ml(Heap_lock);
while (!Universe::has_reference_pending_list()) {
  ml.wait();
}

_references_pending++;
  }

  // Detach and return list
  oop list = Universe::swap_reference_pending_list(NULL);
  return JNIHandles::make_local(env, list);
JVM_END


JVM_ENTRY(void, JVM_notifyEnqueuedReferences(JNIEnv* env))
  MonitorLockerEx ml(Heap_lock);
  _references_enqueued = _references_pending;
  ml.notify_all();
JVM_END


JVM_ENTRY(void, JVM_WaitForEnqueuedReferences(JNIEnv* env))
  MonitorLockerEx ml(Heap_lock);
  while (Universe::has_reference_pending_list() ||
 _references_pending != _references_enqueued) {
ml.wait();
  }
JVM_END


And the ReferenceHandler would do something like:


...

// Get pending references from the VM
Reference pending_list = getPendingReferences();

// Enqueue references
while (pending_list != null) {
// Unlink
Reference r = pending_list;
pending_list = r.discovered;
r.discovered = null;

// Enqueue
ReferenceQueue q = r.queue;
if (q != ReferenceQueue.NULL) {
q.enqueue(r);
}
}

notifyEnqueuedReferences();

...


And a helper thread would do something like:

   ...
   System.gc();

   waitForEnqueuedReferences();
   ...


So, this should be fairly similar to what you proposed Peter, but with 
a slightly different API.


But I'm still kind of hoping we can find some way to avoid exposing 
the wait/notify functions, for the sake of keeping the protocol minimal.


cheers,
Per


It could work equally well with a single getPendingReferences() method 
if that method would guarantee a return after each reference discovery 
cycle regardless of whether any references were discovered or not (it 
would simply return null in case no references were discovered), but it 
would not just spuriously return with null either.


Would something like that be possible?

This can't be simulated with current protocol. But what about the following:

PhantomReference ref = new PhantomReference(new Object(), null);
System.gc();
Reference.reachabilityFence(ref);

Does anything guarantee that at least 'ref' is discovered during 
System.gc() above?


Regards, Peter





With webrev.11.part2 I get a 40% improvement in throughput vs.
webrev.10.part2 executing DirectBufferAllocTest in 16 allocating threads
on a 4-core i7 CPU.

Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-29 Thread Per Liden


Hi Peter,

On 2016-03-28 19:18, Peter Levart wrote:
[...]

And now a few words about ReferenceHandler thread and synchronization
with it (for Kim and Per mostly). I think it should not be a problem to
move the following two java.lang.ref.Reference methods to native code if
desired:

 static Reference getPendingReferences(int[] discoveryPhaseHolder)
 static int getDiscoveryPhase()

The 1st one is only invoked by a ReferenceHandler thread while the 2nd
is invoked by arbitrary thread. The difference between this and
webrev.09.part2 is that there's no need any more for ReferenceHandler
thread to notify the thread executing the 2nd method and that there's no
need for the 2nd method to perform any waiting. It just needs to obtain
the lock briefly so that it can read the consistent state of two
fields.  Those two fields are Java static fields currently:
Reference.pending & Reference.discoveryPhase and those two methods are
Java methods, but they could be moved to native code if desired to make
the protocol between VM and Java code more robust.

So Kim, Per, what do you think of supporting those 2 methods in native
code? Would that present any problem?


In the best of worlds I'd like the VM to be agnostic to how the pending 
list is processed on the core-libs side. However, after looking at it 
briefly I'm not sure if we can get all they way with only providing a 
getPendingReferences() call.


Anyway, assuming we really need something more than just 
getPendingReferences(), I'm not so keen on exposing a phase counter in 
the API. I think I'd rather have something like this:


/** Get the pending list from the VM, blocking until a list exists.
  * Only used by ReferenceHandler.
  */
Reference getPendingReference();

/** Signal that all references has been enqueued.
  * Only used by ReferenceHandler.
  */
void notifyEnqueuedReferences();

/** If references are pending, wait for a notification from
  * ReferenceHandler that they have been enqueued.
  */
void waitForEnqueuedReferences();


The VM would (roughly) implement this as:


JVM_ENTRY(jobject, JVM_GetPendingReferences(JNIEnv* env))
  // Wait until list becomes non-empty
  {
MonitorLockerEx ml(Heap_lock);
while (!Universe::has_reference_pending_list()) {
  ml.wait();
}

_references_pending++;
  }

  // Detach and return list
  oop list = Universe::swap_reference_pending_list(NULL);
  return JNIHandles::make_local(env, list);
JVM_END


JVM_ENTRY(void, JVM_notifyEnqueuedReferences(JNIEnv* env))
  MonitorLockerEx ml(Heap_lock);
  _references_enqueued = _references_pending;
  ml.notify_all();
JVM_END


JVM_ENTRY(void, JVM_WaitForEnqueuedReferences(JNIEnv* env))
  MonitorLockerEx ml(Heap_lock);
  while (Universe::has_reference_pending_list() ||
 _references_pending != _references_enqueued) {
ml.wait();
  }
JVM_END


And the ReferenceHandler would do something like:


...

// Get pending references from the VM
Reference pending_list = getPendingReferences();

// Enqueue references
while (pending_list != null) {
// Unlink
Reference r = pending_list;
pending_list = r.discovered;
r.discovered = null;

// Enqueue
ReferenceQueue q = r.queue;
if (q != ReferenceQueue.NULL) {
q.enqueue(r);
}
}

notifyEnqueuedReferences();

...


And a helper thread would do something like:

   ...
   System.gc();

   waitForEnqueuedReferences();
   ...


So, this should be fairly similar to what you proposed Peter, but with a 
slightly different API.


But I'm still kind of hoping we can find some way to avoid exposing the 
wait/notify functions, for the sake of keeping the protocol minimal.


cheers,
Per



With webrev.11.part2 I get a 40% improvement in throughput vs.
webrev.10.part2 executing DirectBufferAllocTest in 16 allocating threads
on a 4-core i7 CPU.

Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-28 Thread Peter Levart


Hi Mandy, Kim, Per and Roger

I'd like to continue the discussion about the 2nd part of removing 
jdk.internal.ref.Cleaner in this discussion thread.


There was some discussion about whether to synchronize with 
ReferenceHandler thread and wait for it to enqueue the Reference(s) or 
simply detect that there are no more pending Reference(s) by timing out 
on waiting for cleanup actions in discussion thread: "Re: Analysis on 
JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails 
intermittently". Based on that discussion, I have prepared a webrev that 
uses an approach where the detection is performed using timeout:


http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.10.part2/

While this webrev passes the DirectBufferAllocTest, I don't have a good 
feeling about this approach since it is not very robust. I can imagine 
situations where it would not behave optimally - it would either trigger 
reference discovery (System.gc()) more frequently that necessary or it 
would cause delays in execution. So I still prefer the approach where 
allocating thread(s) explicitly synchronize with ReferenceHandler thread 
and wait for it to enqueue pending Reference(s). Luckily this can be 
performed in an easy way (as I will show you shortly). Waiting on 
discovery of pending references by ReferenceHandler thread and handing 
them to it could be moved to native code so that no notification would 
have to be performed in native code from the ReferenceHandler thread to 
the allocating thread(s).


But first, let me reply to Mandy's comments...


On 03/25/2016 11:20 PM, Mandy Chung wrote:

On Mar 19, 2016, at 7:00 AM, Peter Levart  wrote:

Here's the webrev:

 
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.08.part2/


On 03/07/2016 07:35 PM, Mandy Chung wrote:

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:
:

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.


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.



   On the other hand, invoking Deallocator::run (deallocating the native 
memory) in arbitrary threads has no problem.  Consider me being paranoid of the 
fix for JDK-6857566.  The current list of clients using CleanerFactory::cleaner 
may be safe being called from arbitrary threads but I can’t say what will be 
added in the future.


Right, save/clear/restore thread locals then (left for next webrev)...


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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-25 Thread Mandy Chung

> On Mar 19, 2016, at 7:00 AM, Peter Levart  wrote:
> 
> Here's the webrev:
> 
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.08.part2/
> 
>> 
>> On 03/07/2016 07:35 PM, Mandy Chung wrote:
>>> 
>>> 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:
>> :

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).  On the other hand, invoking 
Deallocator::run (deallocating the native memory) in arbitrary threads has no 
problem.  Consider me being paranoid of the fix for JDK-6857566.  The current 
list of clients using CleanerFactory::cleaner may be safe being called from 
arbitrary threads but I can’t say what will be added in the future.

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

Perhaps provide one Cleaner specific for native memory deallocation or anything 
safe to be called in arbitrary thread.  It could provide the entry point for 
the allocating thread to assist the cleaning (i.e. Bits::reserveMemory could 
call it).  That will make it explicit that this cleaner provides explicit 
control for other threads to assist the cleaning action (and JavaLangRefAccess 
would only be used by this special cleaner and not in NIO).

All clients of Unsafe.freeMemory could use that special cleaner for native 
memory deallocation use such as IOVecWrapper, DirectByteBuffer, Marlin’s 
OffHeapArray.

The common cleaner would be kept for other things to use and it should be 
lazily created to avoid another thread.

Does this sound reasonable?

Mandy

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-19 Thread Peter Levart


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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-09 Thread Peter Levart


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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-07 Thread Kim Barrett

> On Mar 6, 2016, at 8:00 AM, Peter Levart  wrote:
> 
> Hi,
> 
> I have been asked to split the changes needed to remove 
> jdk.internal.ref.Cleaner into two changesets. The first one is to contain the 
> straightforward non-controversial changes that remove the references to 
> jdk.internal.ref.Cleaner and swaps them with java.lang.ref.Cleaner in all 
> places but Direct-X-Buffer. This part also contains changes that replace use 
> of lambdas and method references with alternatives.
> 
> Here's the 1st part:
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.07.part1/

webrev.07.part1 looks good to me.  (Consider me a “reviewer" and not a 
“Reviewer" for this.)

> 
> 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/

I’ve only briefly skimmed part2; I agree with Mandy that this part needs more 
discussion.

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-07 Thread Mandy Chung

> On Mar 6, 2016, at 5:00 AM, Peter Levart  wrote:
> 
> Hi,
> 
> I have been asked to split the changes needed to remove 
> jdk.internal.ref.Cleaner into two changesets. The first one is to contain the 
> straightforward non-controversial changes that remove the references to 
> jdk.internal.ref.Cleaner and swaps them with java.lang.ref.Cleaner in all 
> places but Direct-X-Buffer. This part also contains changes that replace use 
> of lambdas and method references with alternatives.
> 
> Here's the 1st part:
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.07.part1/
> 

webrev.07.part1 looks okay.

> 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).  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.

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?  

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?

> If part2 is to be developed further, I would like to 1st push part1 so that 
> maintenance of part2 changeset will be easier.

It’s okay with me to push part1.  I’d like to see different prototypes for 
part2 being explored and evaluate the pros and cons of each one.  Sorry I 
realize this may require additional works.

Mandy

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-07 Thread Chris Hegarty


On 06/03/16 13:00, Peter Levart wrote:

Hi,

I have been asked to split the changes needed to remove
jdk.internal.ref.Cleaner into two changesets. The first one is to
contain the straightforward non-controversial changes that remove the
references to jdk.internal.ref.Cleaner and swaps them with
java.lang.ref.Cleaner in all places but Direct-X-Buffer. This part also
contains changes that replace use of lambdas and method references with
alternatives.

Here's the 1st part:

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.07.part1/


Looks good to me.


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/


From what I can see. I think this is good.

-Chris.



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).

If part2 is to be developed further, I would like to 1st push part1 so
that maintenance of part2 changeset will be easier.

Regards, Peter

On 02/26/2016 10:39 PM, Roger Riggs wrote:

Hi Peter,

I think this cleans up all the points raised earlier.
The optimization for enqueuing from the reference queue seems ok to me
and should be
more efficient than the previous implementation but I think Mandy or
Alan should look at it also.

Thanks, Roger


On 2/25/2016 4:17 AM, Peter Levart wrote:

Hi Alan,

On 02/25/2016 09:00 AM, Alan Bateman wrote:



On 25/02/2016 07:24, Peter Levart wrote:

:

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 think we need also need to look at the option where this is not
public. I have concerns that it is exposing implementation to some
extent and that may become an attractive nuisance in the future.
This shouldn't be an issue for the NIO buffer usage, we can keep the
usage via the shared secrets mechanism. I think this is what Mandy
is suggesting too.

-Alan.


Sure, no problem. Here's a variant that keeps the
Cleaner.cleanNextPending() method private and exposed via
SharedSecrets to nio Bits but is otherwise equivalent to webrev.06:

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.06priv/


Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-03-06 Thread Peter Levart


Hi,

I have been asked to split the changes needed to remove 
jdk.internal.ref.Cleaner into two changesets. The first one is to 
contain the straightforward non-controversial changes that remove the 
references to jdk.internal.ref.Cleaner and swaps them with 
java.lang.ref.Cleaner in all places but Direct-X-Buffer. This part also 
contains changes that replace use of lambdas and method references with 
alternatives.


Here's the 1st part:

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.07.part1/

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).


If part2 is to be developed further, I would like to 1st push part1 so 
that maintenance of part2 changeset will be easier.


Regards, Peter

On 02/26/2016 10:39 PM, Roger Riggs wrote:

Hi Peter,

I think this cleans up all the points raised earlier.
The optimization for enqueuing from the reference queue seems ok to me 
and should be
more efficient than the previous implementation but I think Mandy or 
Alan should look at it also.


Thanks, Roger


On 2/25/2016 4:17 AM, Peter Levart wrote:

Hi Alan,

On 02/25/2016 09:00 AM, Alan Bateman wrote:



On 25/02/2016 07:24, Peter Levart wrote:

:

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 think we need also need to look at the option where this is not 
public. I have concerns that it is exposing implementation to some 
extent and that may become an attractive nuisance in the future. 
This shouldn't be an issue for the NIO buffer usage, we can keep the 
usage via the shared secrets mechanism. I think this is what Mandy 
is suggesting too.


-Alan.


Sure, no problem. Here's a variant that keeps the 
Cleaner.cleanNextPending() method private and exposed via 
SharedSecrets to nio Bits but is otherwise equivalent to webrev.06:


http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.06priv/ 



Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-26 Thread Roger Riggs


Hi Peter,

I think this cleans up all the points raised earlier.
The optimization for enqueuing from the reference queue seems ok to me 
and should be
more efficient than the previous implementation but I think Mandy or 
Alan should look at it also.


Thanks, Roger


On 2/25/2016 4:17 AM, Peter Levart wrote:

Hi Alan,

On 02/25/2016 09:00 AM, Alan Bateman wrote:



On 25/02/2016 07:24, Peter Levart wrote:

:

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 think we need also need to look at the option where this is not 
public. I have concerns that it is exposing implementation to some 
extent and that may become an attractive nuisance in the future. This 
shouldn't be an issue for the NIO buffer usage, we can keep the usage 
via the shared secrets mechanism. I think this is what Mandy is 
suggesting too.


-Alan.


Sure, no problem. Here's a variant that keeps the 
Cleaner.cleanNextPending() method private and exposed via 
SharedSecrets to nio Bits but is otherwise equivalent to webrev.06:


http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.06priv/ 



Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-25 Thread Peter Levart


Hi Alan,

On 02/25/2016 09:00 AM, Alan Bateman wrote:



On 25/02/2016 07:24, Peter Levart wrote:

:

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 think we need also need to look at the option where this is not 
public. I have concerns that it is exposing implementation to some 
extent and that may become an attractive nuisance in the future. This 
shouldn't be an issue for the NIO buffer usage, we can keep the usage 
via the shared secrets mechanism. I think this is what Mandy is 
suggesting too.


-Alan.


Sure, no problem. Here's a variant that keeps the 
Cleaner.cleanNextPending() method private and exposed via SharedSecrets 
to nio Bits but is otherwise equivalent to webrev.06:


http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.06priv/

Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-25 Thread Alan Bateman




On 25/02/2016 07:24, Peter Levart wrote:

:

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 think we need also need to look at the option where this is not 
public. I have concerns that it is exposing implementation to some 
extent and that may become an attractive nuisance in the future. This 
shouldn't be an issue for the NIO buffer usage, we can keep the usage 
via the shared secrets mechanism. I think this is what Mandy is 
suggesting too.


-Alan.

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-24 Thread Peter Levart


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  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

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-24 Thread Roger Riggs


Hi,

Moving cleanNextPending to the CleanerImpl is ok but exposes more of the 
CleanerImpl

to the rest of java.base internals; loosing a bit of encapsulation.

As for the calls to Reference.tryHandlePending(false), the existing code 
in Bits to reserveMemory

is called on arbitrary threads (whoever was creating a Direct-X-Buffer).

The additional call in the cleanNextPending is in the same thread 
context and helps

cleaning and reference processing occur sooner.

Roger


On 2/23/2016 9:28 PM, Mandy Chung wrote:

On Feb 23, 2016, at 3:22 PM, Kim Barrett  wrote:

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.

I suggest restore the JavaLangRefAccess used by direct byte buffer code and 
revisit such explicit mechanism until there is a need.

Another concern is that this method gets to run in any arbitrary thread rather 
than the dedicated reference handler thread / cleaner thread.

Mandy

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-23 Thread Mandy Chung

> On Feb 23, 2016, at 3:22 PM, Kim Barrett  wrote:
> 
> 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.

I suggest restore the JavaLangRefAccess used by direct byte buffer code and 
revisit such explicit mechanism until there is a need.

Another concern is that this method gets to run in any arbitrary thread rather 
than the dedicated reference handler thread / cleaner thread.

Mandy

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-23 Thread Kim Barrett

> On Feb 23, 2016, at 11:35 AM, Peter Levart  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.

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.

-- 
src/java.base/share/classes/java/nio/Direct-X-Buffer.java
  34 import jdk.internal.ref.CleanerFactory;

It might make sense to give Direct-X-Buffer its own dedicated Cleaner,
rather than sharing the jdk.internal Cleaner.

--

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-23 Thread Peter Levart


Hi Alan,

On 02/23/2016 04:33 PM, Alan Bateman wrote:



On 22/02/2016 21:41, Mandy Chung wrote:

:

I added new method to Cleaner:

public boolean helpClean() { }

I’m in two minds of making this a public method. An explicit way to 
enqueue pending references as well as invoke Cleanable::clean.  If it 
goes in, the method name needs to be renamed.


I'm also not sure that it's a good idea to have this be part of the 
Cleaner API. If feels like it's exposing too much of the internals. Is 
it really needed?


Without it, the DirectBufferAllocTest fails.

As John Cage (the character from Ally McBeal) would say: "I'm puzzled!". 
My previous iterations used similar method on the internal CleanerImpl 
class and there were suggestions to expose the functionality as public API.


I don't think this method exposes too much of internals. In the sense 
where it would prevent any possible future rearrangement of the 
internals or evolving of the Cleaner API. It doesn't talk about any 
queue(s) or ReferenceHandler thread. It keeps the Cleaner implementation 
details private. It just enables giving CPU time to assist with cleanup 
and if used as hinted in @implNote, provides back-pressure to allocating 
threads. I think this is a nice way of solving the problem of balancing 
allocation with deallocation. The same problem concurrent GC algorithms 
have, like G1 or CMS. If they can not keep-up with allocation threads, 
they pause them and do the cleanup while the world is stopped.


Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-23 Thread Peter Levart


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





Other than this new method, the change looks good to me.

Mandy

I think this form of method that just does one quantum of work and 
returns a boolean indicating whether there's more work waiting is a 
better fit for some clients that might want to do just a limited 
amount of work at once (like for example 
sun.java2d.Disposer.pollRemove that you mentioned). This method also 
deals with helping the ReferenceHandler thread, which is necessary 
to be able to "squeeze" out all outstanding work. As Cleaner is in 
the same package as Reference and helping ReferenceHandler thread is 
implicitly included in Cleaner.helpClean(), the JavaLangRefAccess 
interface and a field in SharedSecrets can be removed.


I also simplified the API in sun.nio.fs.NativeBuffer and replaced 
the accessor of Cleanable with a simple void free() method (called 
free because it deallocates memory).


I think this will have to be submitted to CCC for approval, right? 
Can you help me with it?



Regards, Peter

On 02/17/2016 11:34 PM, Kim Barrett wrote:
On Feb 17, 2016, at 4:06 AM, Peter Levart  
wrote:


Hi Kim,

Thanks for looking into this. Answers inline…

Peter,

Thanks for the explanations.


On 02/17/2016 01:20 AM, Kim Barrett wrote:

However, I recall Roger saying there were existing tests that

failed when certain uses of sun.misc.Cleaner were replaced with
java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
does.

If the failing test was the following:

java/nio/Buffer/DirectBufferAllocTest.java

...then it has been taken care of (see Bits.java).

That looks familiar. And yes, I see what you did there, and I don't
think Roger's initial prototype testing did anything similar, so
indeed this is likely the failure he encountered.

Though I'm still inclined to object to that form of drainQueue (see
below).


I don't understand why CleanerImpl needs to be changed to public in
order to provide access to the new drainQueue. Wouldn't it be better
to add Cleaner.drainQueue?
An interesting idea. But I don't know if such functionality is 
generally useful enough to commit to it in a public API.

java.desktop:sun.java2d.Disposer.pollRemove seems to me to be
addressing an essentially similar requirement, with
java.desktop:sun.font.StrikeCache being the user of that API.

Of course, that's already got all the scaffolding for using phantom
references, and there's no need to rewrite it to use
java.lang.ref.Cleaner.  But maybe there are others like this?

In any case, I really dislike the approach of exposing the CleanerImpl
object to get at this functionality.


Some of the other changes here don't seem related to the problem at
hand. ...
One thing that this change unfortunately requires is to get rid of 
lambdas and method references in the

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-23 Thread Alan Bateman




On 22/02/2016 21:41, Mandy Chung wrote:

:

I added new method to Cleaner:

public boolean helpClean() { }


I’m in two minds of making this a public method. An explicit way to enqueue 
pending references as well as invoke Cleanable::clean.  If it goes in, the 
method name needs to be renamed.

I'm also not sure that it's a good idea to have this be part of the 
Cleaner API. If feels like it's exposing too much of the internals. Is 
it really needed?


-Alan.

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-22 Thread Mandy Chung


> On Feb 22, 2016, at 1:56 PM, Roger Riggs  wrote:
> 
> Hi Mandy,
> 
> On 2/22/2016 4:41 PM, Mandy Chung wrote:
>>> On Feb 21, 2016, at 10:55 AM, Peter Levart  wrote:
>>> 
> 
>>> I added new method to Cleaner:
>>> 
>>>public boolean helpClean() { }
>>> 
>> I’m in two minds of making this a public method. An explicit way to enqueue 
>> pending references as well as invoke Cleanable::clean.  If it goes in, the 
>> method name needs to be renamed.
> I would suggest a name related to cleaning a single Cleanable.
>> 
>> 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.
> 

I agree the spec needs some work if it’s a public method.  This seems better to 
file a RFE to follow up.

Mandy

> Roger
> 
>> 
>> Other than this new method, the change looks good to me.
>> 
>> Mandy
>> 
>>> I think this form of method that just does one quantum of work and returns 
>>> a boolean indicating whether there's more work waiting is a better fit for 
>>> some clients that might want to do just a limited amount of work at once 
>>> (like for example sun.java2d.Disposer.pollRemove that you mentioned). This 
>>> method also deals with helping the ReferenceHandler thread, which is 
>>> necessary to be able to "squeeze" out all outstanding work. As Cleaner is 
>>> in the same package as Reference and helping ReferenceHandler thread is 
>>> implicitly included in Cleaner.helpClean(), the JavaLangRefAccess interface 
>>> and a field in SharedSecrets can be removed.
>>> 
>>> I also simplified the API in sun.nio.fs.NativeBuffer and replaced the 
>>> accessor of Cleanable with a simple void free() method (called free because 
>>> it deallocates memory).
>>> 
>>> I think this will have to be submitted to CCC for approval, right? Can you 
>>> help me with it?
>>> 
>>> 
>>> Regards, Peter
>>> 
>>> On 02/17/2016 11:34 PM, Kim Barrett wrote:
> On Feb 17, 2016, at 4:06 AM, Peter Levart  wrote:
> 
> Hi Kim,
> 
> Thanks for looking into this. Answers inline…
 Peter,
 
 Thanks for the explanations.
 
> On 02/17/2016 01:20 AM, Kim Barrett wrote:
>>> However, I recall Roger saying there were existing tests that
>> failed when certain uses of sun.misc.Cleaner were replaced with
>> java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
>> does.
> If the failing test was the following:
> 
> java/nio/Buffer/DirectBufferAllocTest.java
> 
> ...then it has been taken care of (see Bits.java).
 That looks familiar. And yes, I see what you did there, and I don't
 think Roger's initial prototype testing did anything similar, so
 indeed this is likely the failure he encountered.
 
 Though I'm still inclined to object to that form of drainQueue (see
 below).
 
>> I don't understand why CleanerImpl needs to be changed to public in
>> order to provide access to the new drainQueue. Wouldn't it be better
>> to add Cleaner.drainQueue?
> An interesting idea. But I don't know if such functionality is generally 
> useful enough to commit to it in a public API.
 java.desktop:sun.java2d.Disposer.pollRemove seems to me to be
 addressing an essentially similar requirement, with
 java.desktop:sun.font.StrikeCache being the user of that API.
 
 Of course, that's already got all the scaffolding for using phantom
 references, and there's no need to rewrite it to use
 java.lang.ref.Cleaner.  But maybe there are others like this?
 
 In any case, I really dislike the approach of exposing the CleanerImpl
 object to get at this functionality.
 
>> Some of the other changes here don't seem related to the problem at
>> hand. ...
> One thing that this change unfortunately requires is to get rid of 
> lambdas and method references in the implementation and dependencies of 
> java.land.ref.Cleaner API, because it gets used early in the boot-up 
> sequence when java.lang.invoke infrastructure is not ready yet.
 Oh, right!  Bootstrapping issues!
 
> The alternative to CleanerCleanable is a no-op Runnable implementation 
> passed to PhantomCleanableRef constructor. …
 OK.  Now I understand.
 
> Making CleanerImpl implement Runnable …

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-22 Thread Roger Riggs


Hi Mandy,

On 2/22/2016 4:41 PM, Mandy Chung wrote:

On Feb 21, 2016, at 10:55 AM, Peter Levart  wrote:




I added new method to Cleaner:

public boolean helpClean() { }


I’m in two minds of making this a public method. An explicit way to enqueue 
pending references as well as invoke Cleanable::clean.  If it goes in, the 
method name needs to be renamed.

I would suggest a name related to cleaning a single Cleanable.


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



Other than this new method, the change looks good to me.

Mandy


I think this form of method that just does one quantum of work and returns a boolean 
indicating whether there's more work waiting is a better fit for some clients that might 
want to do just a limited amount of work at once (like for example 
sun.java2d.Disposer.pollRemove that you mentioned). This method also deals with helping 
the ReferenceHandler thread, which is necessary to be able to "squeeze" out all 
outstanding work. As Cleaner is in the same package as Reference and helping 
ReferenceHandler thread is implicitly included in Cleaner.helpClean(), the 
JavaLangRefAccess interface and a field in SharedSecrets can be removed.

I also simplified the API in sun.nio.fs.NativeBuffer and replaced the accessor 
of Cleanable with a simple void free() method (called free because it 
deallocates memory).

I think this will have to be submitted to CCC for approval, right? Can you help 
me with it?


Regards, Peter

On 02/17/2016 11:34 PM, Kim Barrett wrote:

On Feb 17, 2016, at 4:06 AM, Peter Levart  wrote:

Hi Kim,

Thanks for looking into this. Answers inline…

Peter,

Thanks for the explanations.


On 02/17/2016 01:20 AM, Kim Barrett wrote:

However, I recall Roger saying there were existing tests that

failed when certain uses of sun.misc.Cleaner were replaced with
java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
does.

If the failing test was the following:

java/nio/Buffer/DirectBufferAllocTest.java

...then it has been taken care of (see Bits.java).

That looks familiar. And yes, I see what you did there, and I don't
think Roger's initial prototype testing did anything similar, so
indeed this is likely the failure he encountered.

Though I'm still inclined to object to that form of drainQueue (see
below).


I don't understand why CleanerImpl needs to be changed to public in
order to provide access to the new drainQueue. Wouldn't it be better
to add Cleaner.drainQueue?

An interesting idea. But I don't know if such functionality is generally useful 
enough to commit to it in a public API.

java.desktop:sun.java2d.Disposer.pollRemove seems to me to be
addressing an essentially similar requirement, with
java.desktop:sun.font.StrikeCache being the user of that API.

Of course, that's already got all the scaffolding for using phantom
references, and there's no need to rewrite it to use
java.lang.ref.Cleaner.  But maybe there are others like this?

In any case, I really dislike the approach of exposing the CleanerImpl
object to get at this functionality.


Some of the other changes here don't seem related to the problem at
hand. ...

One thing that this change unfortunately requires is to get rid of lambdas and 
method references in the implementation and dependencies of 
java.land.ref.Cleaner API, because it gets used early in the boot-up sequence 
when java.lang.invoke infrastructure is not ready yet.

Oh, right!  Bootstrapping issues!


The alternative to CleanerCleanable is a no-op Runnable implementation passed 
to PhantomCleanableRef constructor. …

OK.  Now I understand.


Making CleanerImpl implement Runnable …

I'd be fine with this if the CleanerImpl wasn't exposed as part of the
drainQueue functionality.


--
src/java.base/share/classes/sun/nio/fs/NativeBuffer.java
   76 Cleaner.Cleanable cleanable() {
   77 return cleanable;
   78 }

[pre-existing, but if we're changing things anyway...]

I'm kind of surprised by an accessor function (both here and in the
original code) rather than a cleanup function.  Is there a use case
for anything other than buffer.cleanable().clean()?

It can't be, since both old Cleaner and new Cleanable have only got a single 
method - clean().

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-22 Thread Mandy Chung


> On Feb 21, 2016, at 10:55 AM, Peter Levart  wrote:
> 
> Hi Kim, Roger,
> 
> I have prepared another webrev which addresses your outstanding concerns:
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.04/
> 

MethodHandleNatives.java
   line 71-74 Nit: can you break the long lines 

> I added new method to Cleaner:
> 
>public boolean helpClean() { }
> 

I’m in two minds of making this a public method. An explicit way to enqueue 
pending references as well as invoke Cleanable::clean.  If it goes in, the 
method name needs to be renamed.

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?

Other than this new method, the change looks good to me.

Mandy

> I think this form of method that just does one quantum of work and returns a 
> boolean indicating whether there's more work waiting is a better fit for some 
> clients that might want to do just a limited amount of work at once (like for 
> example sun.java2d.Disposer.pollRemove that you mentioned). This method also 
> deals with helping the ReferenceHandler thread, which is necessary to be able 
> to "squeeze" out all outstanding work. As Cleaner is in the same package as 
> Reference and helping ReferenceHandler thread is implicitly included in 
> Cleaner.helpClean(), the JavaLangRefAccess interface and a field in 
> SharedSecrets can be removed.
> 
> I also simplified the API in sun.nio.fs.NativeBuffer and replaced the 
> accessor of Cleanable with a simple void free() method (called free because 
> it deallocates memory).
> 
> I think this will have to be submitted to CCC for approval, right? Can you 
> help me with it?
> 
> 
> Regards, Peter
> 
> On 02/17/2016 11:34 PM, Kim Barrett wrote:
>>> On Feb 17, 2016, at 4:06 AM, Peter Levart  wrote:
>>> 
>>> Hi Kim,
>>> 
>>> Thanks for looking into this. Answers inline…
>> Peter,
>> 
>> Thanks for the explanations.
>> 
>>> On 02/17/2016 01:20 AM, Kim Barrett wrote:
> However, I recall Roger saying there were existing tests that
 failed when certain uses of sun.misc.Cleaner were replaced with
 java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
 does.
>>> If the failing test was the following:
>>> 
>>> java/nio/Buffer/DirectBufferAllocTest.java
>>> 
>>> ...then it has been taken care of (see Bits.java).
>> That looks familiar. And yes, I see what you did there, and I don't
>> think Roger's initial prototype testing did anything similar, so
>> indeed this is likely the failure he encountered.
>> 
>> Though I'm still inclined to object to that form of drainQueue (see
>> below).
>> 
 I don't understand why CleanerImpl needs to be changed to public in
 order to provide access to the new drainQueue. Wouldn't it be better
 to add Cleaner.drainQueue?
>>> An interesting idea. But I don't know if such functionality is generally 
>>> useful enough to commit to it in a public API.
>> java.desktop:sun.java2d.Disposer.pollRemove seems to me to be
>> addressing an essentially similar requirement, with
>> java.desktop:sun.font.StrikeCache being the user of that API.
>> 
>> Of course, that's already got all the scaffolding for using phantom
>> references, and there's no need to rewrite it to use
>> java.lang.ref.Cleaner.  But maybe there are others like this?
>> 
>> In any case, I really dislike the approach of exposing the CleanerImpl
>> object to get at this functionality.
>> 
 Some of the other changes here don't seem related to the problem at
 hand. ...
>>> One thing that this change unfortunately requires is to get rid of lambdas 
>>> and method references in the implementation and dependencies of 
>>> java.land.ref.Cleaner API, because it gets used early in the boot-up 
>>> sequence when java.lang.invoke infrastructure is not ready yet.
>> Oh, right!  Bootstrapping issues!
>> 
>>> The alternative to CleanerCleanable is a no-op Runnable implementation 
>>> passed to PhantomCleanableRef constructor. …
>> OK.  Now I understand.
>> 
>>> Making CleanerImpl implement Runnable …
>> I'd be fine with this if the CleanerImpl wasn't exposed as part of the
>> drainQueue functionality.
>> 
 --
 src/java.base/share/classes/sun/nio/fs/NativeBuffer.java
   76 Cleaner.Cleanable cleanable() {
   77 return cleanable;
   78 }
 
 [pre-existing, but if we're changing things anyway...]
 
 I'm kind of surprised by an accessor function (both here and in the
 original code) rather than a cleanup function.  Is there a use case
 for anything other than buffer.cleanable().clean()?
>>> It can't be, since both old Cleaner and new Cleanable have only got a 
>>> single method - clean().
>> So this could

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-21 Thread Peter Levart


Hi Kim, Roger,

I have prepared another webrev which addresses your outstanding concerns:

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.04/

I added new method to Cleaner:

public boolean helpClean() { ... }

I think this form of method that just does one quantum of work and 
returns a boolean indicating whether there's more work waiting is a 
better fit for some clients that might want to do just a limited amount 
of work at once (like for example sun.java2d.Disposer.pollRemove that 
you mentioned). This method also deals with helping the ReferenceHandler 
thread, which is necessary to be able to "squeeze" out all outstanding 
work. As Cleaner is in the same package as Reference and helping 
ReferenceHandler thread is implicitly included in Cleaner.helpClean(), 
the JavaLangRefAccess interface and a field in SharedSecrets can be removed.


I also simplified the API in sun.nio.fs.NativeBuffer and replaced the 
accessor of Cleanable with a simple void free() method (called free 
because it deallocates memory).


I think this will have to be submitted to CCC for approval, right? Can 
you help me with it?



Regards, Peter

On 02/17/2016 11:34 PM, Kim Barrett wrote:

On Feb 17, 2016, at 4:06 AM, Peter Levart  wrote:

Hi Kim,

Thanks for looking into this. Answers inline…

Peter,

Thanks for the explanations.


On 02/17/2016 01:20 AM, Kim Barrett wrote:

However, I recall Roger saying there were existing tests that

failed when certain uses of sun.misc.Cleaner were replaced with
java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
does.

If the failing test was the following:

java/nio/Buffer/DirectBufferAllocTest.java

...then it has been taken care of (see Bits.java).

That looks familiar. And yes, I see what you did there, and I don't
think Roger's initial prototype testing did anything similar, so
indeed this is likely the failure he encountered.

Though I'm still inclined to object to that form of drainQueue (see
below).


I don't understand why CleanerImpl needs to be changed to public in
order to provide access to the new drainQueue. Wouldn't it be better
to add Cleaner.drainQueue?

An interesting idea. But I don't know if such functionality is generally useful 
enough to commit to it in a public API.

java.desktop:sun.java2d.Disposer.pollRemove seems to me to be
addressing an essentially similar requirement, with
java.desktop:sun.font.StrikeCache being the user of that API.

Of course, that's already got all the scaffolding for using phantom
references, and there's no need to rewrite it to use
java.lang.ref.Cleaner.  But maybe there are others like this?

In any case, I really dislike the approach of exposing the CleanerImpl
object to get at this functionality.


Some of the other changes here don't seem related to the problem at
hand. ...

One thing that this change unfortunately requires is to get rid of lambdas and 
method references in the implementation and dependencies of 
java.land.ref.Cleaner API, because it gets used early in the boot-up sequence 
when java.lang.invoke infrastructure is not ready yet.

Oh, right!  Bootstrapping issues!


The alternative to CleanerCleanable is a no-op Runnable implementation passed 
to PhantomCleanableRef constructor. …

OK.  Now I understand.


Making CleanerImpl implement Runnable …

I'd be fine with this if the CleanerImpl wasn't exposed as part of the
drainQueue functionality.


--
src/java.base/share/classes/sun/nio/fs/NativeBuffer.java
   76 Cleaner.Cleanable cleanable() {
   77 return cleanable;
   78 }

[pre-existing, but if we're changing things anyway...]

I'm kind of surprised by an accessor function (both here and in the
original code) rather than a cleanup function.  Is there a use case
for anything other than buffer.cleanable().clean()?

It can't be, since both old Cleaner and new Cleanable have only got a single 
method - clean().

So this could be replaced with

   void clean() {
 cleanable.clean();
   }

To me, that seems better.


Similarly for the DirectBuffer interface.

This one is a critical method, used by various 3rd party softwares...

I want to cover my ears when people start talking about some of the
things that have done…  OK.

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-18 Thread Roger Riggs


Hi Peter,

Nice to see this improvement.

I would support adding the drainQueue method to java.lang.ref.Cleaner; 
it provides
a mechanism to share the cleanup load (as in the case of Direct buffers) 
that may be useful

to other use cases.  It is preferable to hacking in to the CleanerImpl.

Making CleanerImpl implement Runnable is ok as long as CleanerImpl is 
limited to *only*
be the implementation of Cleaner and is not expected to be used directly 
even internal to the base module.


The conversions of lambdas to classes is necessary, unfortunately.

The names of the Cleaner threads could use the threadId instead of 
introducing a counter,
but probably not a big difference either way.  The numbers are not going 
to be significant.


Roger



On 2/17/2016 4:06 AM, Peter Levart wrote:

Hi Kim,

Thanks for looking into this. Answers inline...

On 02/17/2016 01:20 AM, Kim Barrett wrote:
On Feb 16, 2016, at 11:15 AM, Peter Levart  
wrote:


Hello everybody,

Thanks for looking into this and for all your comments. Now that it 
seems we have a consensus that replacing internal Cleaner with 
public Cleaner is not a wrong idea, I created an issue for it [1] so 
that we may officially review the implementation. I also created an 
updated webrev [2] which fixes some comments in usages that were not 
correct any more after the change. I also took the liberty to modify 
the CleanerImpl.InnocuousThreadFactory to give names to threads in 
accordance to the style used in some other thread factories 
(Timer-0, Timer-1, ..., Cleaner-0, Cleaner-1, ..., etc.). This gives 
the thread of internal Cleaner instance, which is now constructed as 
part of the boot-up sequence, a stable name: "Cleaner-0".


If you feel that internal Cleaner instance should have a Thread with 
MAX_PRIORITY, I can incorporate that too.


[1] https://bugs.openjdk.java.net/browse/JDK-8149925
[2] 
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.03/


I re-ran the java/lang/ref and java/nio tests and all pass except 
two ignored tests:

I think some of the existing uses of sun.misc.Cleaner were really just
a matter of convenience and the previous lack of
java.lang.ref.Cleaner.  An example of this that I have personal
knowledge of the history for is the CallSite cleanup in
MethodHandleNatives.  It's good to see those converted.

As for the others, if nobody wants to defend the special handling by
the reference processing thread, I'm certainly happy to see it
removed. However, I recall Roger saying there were existing tests that
failed when certain uses of sun.misc.Cleaner were replaced with
java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
does.


If the failing test was the following:

java/nio/Buffer/DirectBufferAllocTest.java

...then it has been taken care of (see Bits.java). All java/lang/ref 
and java/nio tests pass on my PC. Are there any internal Oracle tests 
that fail?




-- 


src/java.base/share/classes/java/lang/ref/Reference.java

After removal of the special handling of removing Cleaner objects from
the pending list, do we still need to catch OOMEs from the pending
list?  If not sure, feel free to defer that in another RFE for cleanup.


As you have already noticed it is still possible for a lock.wait() to 
throw OOME because of not being able to allocate InterruptedException.


The other place where OOME could still be thrown (and has not been 
fixed yet) is from sun.misc.Cleaner.clean() special handling. I even 
constructed a reproducer for it (see the last comment in JDK-8066859). 
So removing special handling of sun.misc.Cleaner would finally put the 
JDK-8066859 to the rest.




-- 


src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java

I don't understand why CleanerImpl needs to be changed to public in
order to provide access to the new drainQueue. Wouldn't it be better
to add Cleaner.drainQueue?


An interesting idea. But I don't know if such functionality is 
generally useful enough to commit to it in a public API.


I have another idea where java.lang.ref.Cleaner would use an Executor 
instead of ThreadFactory. The default would be an instance of a 
single-threaded executor per Cleaner instance, but if user passed in a 
ForkJoinPool, he could use it's method ForkJoinPool.awaitQuiescence() 
to help the executor's thread(s) do the cleaning.




Some of the other changes here don't seem related to the problem at
hand. Are these proposed miscellaneous cleanups? I'm specifically
looking at the CleanerCleanable class. And I'm not sure why the
thread's constructor argument is being changed, requiring CleanerImpl
to implement Runnable.


One thing that this change unfortunately requires is to get rid of 
lambdas and method references in the implementation and dependencies 
of java.land.ref.Cleaner API,

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-17 Thread Kim Barrett

> On Feb 17, 2016, at 4:06 AM, Peter Levart  wrote:
> 
> Hi Kim,
> 
> Thanks for looking into this. Answers inline…

Peter,

Thanks for the explanations.

> On 02/17/2016 01:20 AM, Kim Barrett wrote:
>>> However, I recall Roger saying there were existing tests that
>> failed when certain uses of sun.misc.Cleaner were replaced with
>> java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
>> does.
> 
> If the failing test was the following:
> 
> java/nio/Buffer/DirectBufferAllocTest.java
> 
> ...then it has been taken care of (see Bits.java).

That looks familiar. And yes, I see what you did there, and I don't
think Roger's initial prototype testing did anything similar, so
indeed this is likely the failure he encountered.

Though I'm still inclined to object to that form of drainQueue (see
below).

>> I don't understand why CleanerImpl needs to be changed to public in
>> order to provide access to the new drainQueue. Wouldn't it be better
>> to add Cleaner.drainQueue?
> 
> An interesting idea. But I don't know if such functionality is generally 
> useful enough to commit to it in a public API.

java.desktop:sun.java2d.Disposer.pollRemove seems to me to be
addressing an essentially similar requirement, with
java.desktop:sun.font.StrikeCache being the user of that API.

Of course, that's already got all the scaffolding for using phantom
references, and there's no need to rewrite it to use
java.lang.ref.Cleaner.  But maybe there are others like this?

In any case, I really dislike the approach of exposing the CleanerImpl
object to get at this functionality.

>> Some of the other changes here don't seem related to the problem at
>> hand. ...
> 
> One thing that this change unfortunately requires is to get rid of lambdas 
> and method references in the implementation and dependencies of 
> java.land.ref.Cleaner API, because it gets used early in the boot-up sequence 
> when java.lang.invoke infrastructure is not ready yet.

Oh, right!  Bootstrapping issues!

> The alternative to CleanerCleanable is a no-op Runnable implementation passed 
> to PhantomCleanableRef constructor. …

OK.  Now I understand.

> Making CleanerImpl implement Runnable …

I'd be fine with this if the CleanerImpl wasn't exposed as part of the
drainQueue functionality.

>> --
>> src/java.base/share/classes/sun/nio/fs/NativeBuffer.java
>>   76 Cleaner.Cleanable cleanable() {
>>   77 return cleanable;
>>   78 }
>> 
>> [pre-existing, but if we're changing things anyway...]
>> 
>> I'm kind of surprised by an accessor function (both here and in the
>> original code) rather than a cleanup function.  Is there a use case
>> for anything other than buffer.cleanable().clean()?
> 
> It can't be, since both old Cleaner and new Cleanable have only got a single 
> method - clean().

So this could be replaced with

  void clean() {
cleanable.clean();
  }

To me, that seems better.

>> Similarly for the DirectBuffer interface.
> 
> This one is a critical method, used by various 3rd party softwares...

I want to cover my ears when people start talking about some of the
things that have done…  OK.

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-17 Thread Peter Levart




On 02/17/2016 10:06 AM, Peter Levart wrote:
On 02/17/2016 01:20 AM, Kim Barrett wrote:

src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java

I don't understand why CleanerImpl needs to be changed to public in
order to provide access to the new drainQueue. Wouldn't it be better
to add Cleaner.drainQueue?


An interesting idea. But I don't know if such functionality is 
generally useful enough to commit to it in a public API.


I have another idea where java.lang.ref.Cleaner would use an Executor 
instead of ThreadFactory. The default would be an instance of a 
single-threaded executor per Cleaner instance, but if user passed in a 
ForkJoinPool, he could use it's method ForkJoinPool.awaitQuiescence() 
to help the executor's thread(s) do the cleaning. 


...well, this would not be a good idea as ReferenceHandler thread would 
then have to feed the Executor (invoke Executor::execute(Runnable)) and 
a user-specified Executor could abuse that. There has to be a queue API 
between ReferenceHandler and custom user code.


Regards, Peter

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-17 Thread Alan Bateman



On 16/02/2016 16:15, Peter Levart wrote:

:

and the following two, which seem to not like my network config. or 
something like that:


java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java: Unit 
test for DatagramChannel's multicast support
java/nio/channels/DatagramChannel/Promiscuous.java: Test for 
interference when two sockets are bound to the same port but joined to 
different multicast groups
These two can be problematic when there is a firewall or VPN connection. 
I can't think how they could fail due to the changes that we are 
discussing here.


-Alan

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-17 Thread Peter Levart


Hi Kim,

Thanks for looking into this. Answers inline...

On 02/17/2016 01:20 AM, Kim Barrett wrote:

On Feb 16, 2016, at 11:15 AM, Peter Levart  wrote:

Hello everybody,

Thanks for looking into this and for all your comments. Now that it seems we have a 
consensus that replacing internal Cleaner with public Cleaner is not a wrong idea, I 
created an issue for it [1] so that we may officially review the implementation. I also 
created an updated webrev [2] which fixes some comments in usages that were not correct 
any more after the change. I also took the liberty to modify the 
CleanerImpl.InnocuousThreadFactory to give names to threads in accordance to the style 
used in some other thread factories (Timer-0, Timer-1, ..., Cleaner-0, Cleaner-1, ..., 
etc.). This gives the thread of internal Cleaner instance, which is now constructed as 
part of the boot-up sequence, a stable name: "Cleaner-0".

If you feel that internal Cleaner instance should have a Thread with 
MAX_PRIORITY, I can incorporate that too.

[1] https://bugs.openjdk.java.net/browse/JDK-8149925
[2] 
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.03/

I re-ran the java/lang/ref and java/nio tests and all pass except two ignored 
tests:

I think some of the existing uses of sun.misc.Cleaner were really just
a matter of convenience and the previous lack of
java.lang.ref.Cleaner.  An example of this that I have personal
knowledge of the history for is the CallSite cleanup in
MethodHandleNatives.  It's good to see those converted.

As for the others, if nobody wants to defend the special handling by
the reference processing thread, I'm certainly happy to see it
removed. However, I recall Roger saying there were existing tests that
failed when certain uses of sun.misc.Cleaner were replaced with
java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
does.


If the failing test was the following:

java/nio/Buffer/DirectBufferAllocTest.java

...then it has been taken care of (see Bits.java). All java/lang/ref and 
java/nio tests pass on my PC. Are there any internal Oracle tests that fail?




--
src/java.base/share/classes/java/lang/ref/Reference.java

After removal of the special handling of removing Cleaner objects from
the pending list, do we still need to catch OOMEs from the pending
list?  If not sure, feel free to defer that in another RFE for cleanup.


As you have already noticed it is still possible for a lock.wait() to 
throw OOME because of not being able to allocate InterruptedException.


The other place where OOME could still be thrown (and has not been fixed 
yet) is from sun.misc.Cleaner.clean() special handling. I even 
constructed a reproducer for it (see the last comment in JDK-8066859). 
So removing special handling of sun.misc.Cleaner would finally put the 
JDK-8066859 to the rest.




--
src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java

I don't understand why CleanerImpl needs to be changed to public in
order to provide access to the new drainQueue. Wouldn't it be better
to add Cleaner.drainQueue?


An interesting idea. But I don't know if such functionality is generally 
useful enough to commit to it in a public API.


I have another idea where java.lang.ref.Cleaner would use an Executor 
instead of ThreadFactory. The default would be an instance of a 
single-threaded executor per Cleaner instance, but if user passed in a 
ForkJoinPool, he could use it's method ForkJoinPool.awaitQuiescence() to 
help the executor's thread(s) do the cleaning.




Some of the other changes here don't seem related to the problem at
hand. Are these proposed miscellaneous cleanups? I'm specifically
looking at the CleanerCleanable class. And I'm not sure why the
thread's constructor argument is being changed, requiring CleanerImpl
to implement Runnable.


One thing that this change unfortunately requires is to get rid of 
lambdas and method references in the implementation and dependencies of 
java.land.ref.Cleaner API, because it gets used early in the boot-up 
sequence when java.lang.invoke infrastructure is not ready yet.


The alternative to CleanerCleanable is a no-op Runnable implementation 
passed to PhantomCleanableRef constructor. I opted for a subclass of 
abstract PhantomCleanable class with an empty performCleanup() method. 
Both approaches require a new class, but the CleanerCleanable is a more 
direct way to express the intent.


Making CleanerImpl implement Runnable and consequently making its run() 
method public is also just a way to avoid introducing another anonymous 
inner class as an adapter between Runnable.run() and CleanerImpl.run(). 
CleanerImpl is in an internal concealed package, so there's no danger of 
abuse. But I can revert it back to using an anonymous inner adapter 
class (as was done in

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-16 Thread Kim Barrett

> On Feb 16, 2016, at 7:20 PM, Kim Barrett  wrote:
> src/java.base/share/classes/java/lang/ref/Reference.java
> 
> After removal of the special handling of removing Cleaner objects from
> the pending list, do we still need to catch OOMEs from the pending
> list?  If not sure, feel free to defer that in another RFE for cleanup.

Never mind.  OOME could result from attempting to construct and throw 
InterruptedException.

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-16 Thread Kim Barrett

> On Feb 16, 2016, at 11:15 AM, Peter Levart  wrote:
> 
> Hello everybody,
> 
> Thanks for looking into this and for all your comments. Now that it seems we 
> have a consensus that replacing internal Cleaner with public Cleaner is not a 
> wrong idea, I created an issue for it [1] so that we may officially review 
> the implementation. I also created an updated webrev [2] which fixes some 
> comments in usages that were not correct any more after the change. I also 
> took the liberty to modify the CleanerImpl.InnocuousThreadFactory to give 
> names to threads in accordance to the style used in some other thread 
> factories (Timer-0, Timer-1, ..., Cleaner-0, Cleaner-1, ..., etc.). This 
> gives the thread of internal Cleaner instance, which is now constructed as 
> part of the boot-up sequence, a stable name: "Cleaner-0".
> 
> If you feel that internal Cleaner instance should have a Thread with 
> MAX_PRIORITY, I can incorporate that too.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8149925
> [2] 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.03/
> 
> I re-ran the java/lang/ref and java/nio tests and all pass except two ignored 
> tests:

I think some of the existing uses of sun.misc.Cleaner were really just
a matter of convenience and the previous lack of
java.lang.ref.Cleaner.  An example of this that I have personal
knowledge of the history for is the CallSite cleanup in
MethodHandleNatives.  It's good to see those converted.

As for the others, if nobody wants to defend the special handling by
the reference processing thread, I'm certainly happy to see it
removed. However, I recall Roger saying there were existing tests that
failed when certain uses of sun.misc.Cleaner were replaced with
java.lang.ref.Cleaner. I don't remember the details, but maybe Roger
does.

-- 
src/java.base/share/classes/java/lang/ref/Reference.java

After removal of the special handling of removing Cleaner objects from
the pending list, do we still need to catch OOMEs from the pending
list?  If not sure, feel free to defer that in another RFE for cleanup.

--
src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java

I don't understand why CleanerImpl needs to be changed to public in
order to provide access to the new drainQueue. Wouldn't it be better
to add Cleaner.drainQueue?

Some of the other changes here don't seem related to the problem at
hand. Are these proposed miscellaneous cleanups? I'm specifically
looking at the CleanerCleanable class. And I'm not sure why the
thread's constructor argument is being changed, requiring CleanerImpl
to implement Runnable.

-- 
src/java.base/share/classes/sun/nio/fs/NativeBuffer.java
  76 Cleaner.Cleanable cleanable() {
  77 return cleanable;
  78 }

[pre-existing, but if we're changing things anyway...]

I'm kind of surprised by an accessor function (both here and in the
original code) rather than a cleanup function.  Is there a use case
for anything other than buffer.cleanable().clean()?

Similarly for the DirectBuffer interface.

--

RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

2016-02-16 Thread Peter Levart


Hello everybody,

Thanks for looking into this and for all your comments. Now that it 
seems we have a consensus that replacing internal Cleaner with public 
Cleaner is not a wrong idea, I created an issue for it [1] so that we 
may officially review the implementation. I also created an updated 
webrev [2] which fixes some comments in usages that were not correct any 
more after the change. I also took the liberty to modify the 
CleanerImpl.InnocuousThreadFactory to give names to threads in 
accordance to the style used in some other thread factories (Timer-0, 
Timer-1, ..., Cleaner-0, Cleaner-1, ..., etc.). This gives the thread of 
internal Cleaner instance, which is now constructed as part of the 
boot-up sequence, a stable name: "Cleaner-0".


If you feel that internal Cleaner instance should have a Thread with 
MAX_PRIORITY, I can incorporate that too.


[1] https://bugs.openjdk.java.net/browse/JDK-8149925
[2] 
http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.03/


I re-ran the java/lang/ref and java/nio tests and all pass except two 
ignored tests:


java/nio/Buffer/LimitDirectMemory.sh: Test option to limit direct memory 
allocation
java/nio/file/spi/SetDefaultProvider.java: Unit test for 
java.nio.file.spi.FileSystemProvider


and the following two, which seem to not like my network config. or 
something like that:


java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java: Unit 
test for DatagramChannel's multicast support
java/nio/channels/DatagramChannel/Promiscuous.java: Test for 
interference when two sockets are bound to the same port but joined to 
different multicast groups



Regards, Peter


On 02/16/2016 03:02 PM, Chris Hegarty wrote:

On 15 Feb 2016, at 20:05, Mandy Chung  wrote:


On Feb 15, 2016, at 9:06 AM, Uwe Schindler  wrote:

Hi,


On 15/02/2016 14:57, Chris Hegarty wrote:

Peter,

http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.02/


This patch looks correct to me.  The innocuous thread created for common 
cleaner is of Thread.MAX_PRIORITY - 2 priority whereas the reference handler 
thread is of MAX_PRIORITY priority.

I see your point, but I don’t see this as an issue because threads failing to
allocate memory themselves get involved in cleaning. So pressure is
somewhat off the reference handler thread.

-Chris.

52 matches

Mail list logo