On 09/19/2014 09:45 AM, David Holmes wrote:
On 19/09/2014 5:35 PM, Peter Levart wrote:
On 09/19/2014 09:06 AM, David Holmes wrote:
Hi Peter,
Two comments:
1. Doing the Object.wait() violates the thou-shalt-not-allocate rule,
due to the potential for creating the InterruptedException. But there
is no way to avoid that.
It depends on when the InterruptedException is allocated. While
Object.wait()-ing, the lock is not held. If, while waiting, the
interruption is detected, it could be that InterruptedException is
allocated before the lock is attempted to be re-gained. This can easily
be verified - stay tuned...
Allocation happens after the monitor is re-acquired.
You're right. I have verified this with a simple test (a modified
InterruptedException class). But this means that interrupting
ReferenceHandler thread is already a violation of
thou-shalt-not-allocate rule.
Do you think that a re-design of VM <-> Java interface is warranted?
Now that we have Unsafe, we could handle a queue of 'pending' references
using CAS. There is a single blocking consumer of that queue - a
ReferenceHandler thread. Other consumers (calling tryHandlePending()
from nio.Bits) are non-blocking, just polling the queue. So there could
be a single static:
private static ReferenceHandler waiter;
field in Reference class, holding the reference to ReferenceHandler
thread while it is Unsafe.park()-ed - no allocation necessary. The VM
could just unpark the waiter after CAS-ing new references on the pending
list...
But that's another issue. We're now asking ourselves if "instanceof"
call is in need of a clean-up, not wait().
Regards, Peter
2. I don't see how the instanceof can still result in OOME if we have
pre-loaded and initialized the relevant classes.
I don't see either. So you're more for a variant that we move instanceof
check outside the synchronized block to where it was before and wait for
stress tests to show if we're right? This might be a cleaner way to
final solution...
I think I see what you are saying now ...
So I'm not sure what it is that needs fixing now ?? Note that if the
OOME is thrown while holding the lock it means we haven't deadlocked
with the GC.
This might be an indication that there's not any allocation taking place
in instanceof check any more (wait is different, since it's releasing
lock temporarily). If it was taking place, the stress tests that
detected OOME before, would detect deadlock now, right?
There's only a potential for deadlock, it isn't inevitable. You'd need
to see when the GC needs to acquire this lock.
I understand that this is more of a clean-up attempt than fixing any
real issue.
I'm inclined to take the "if it ain't broke ..." position on this. But
I do see your point.
Cheers,
David
Regards, Peter
David
On 19/09/2014 4:43 PM, Peter Levart wrote:
Hi,
This story has a long tail. It started with:
https://bugs.openjdk.java.net/browse/JDK-7038914
Some stress tests triggered OOME in ReferenceHandler thread which
would
die. The first attempt at fixing this was the following discussion:
https://www.mail-archive.com/core-libs-dev%40openjdk.java.net/msg16250.html
Which resulted in patch:
http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/0b8dab7fec54
This assumed that ReferenceHandler thread doing Object.wait() could be
interrupted (by stress test - normal application don't do that) and
failed to allocate InterruptedException object. A jtreg test was
designed which triggered that situation and a fix would catch OOME and
ignore it.
But the stress tests (the same or some other, I don't know) apparently
were not entirely happy with this fix. The following discussion
describes this:
https://www.mail-archive.com/core-libs-dev%40openjdk.java.net/msg23596.html
The other "unprotected" point at which OOME could be thrown and was
later confirmed by debugging is (r instanceof Cleaner) test. The
assumption was that it could trigger Cleaner class loading at 1st
execution which would cause OOME to be thrown. The fix that finally
silenced stress tests was the following:
http://hg.openjdk.java.net/jdk9/dev/jdk/rev/d04102f69d46
This part of code (the j.l.r.Reference class and its members) has
undergone further changes afterwards for fixing another bug:
https://bugs.openjdk.java.net/browse/JDK-6857566
With following patch:
http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/9934d34ed3c0
But this did not change the code paths - just factored-out the content
of the loop into a separate method that could be used from outside.
All well until kim.barr...@oracle.com noticed that the 2nd fix
introduced a potentially illegal situation. There is a
j.l.r.Reference.Lock inner class and a singleton object assigned to
static field in j.l.Reference class with the following notice:
/* Object used to synchronize with the garbage collector. The
collector
* must acquire this lock at the beginning of each collection
cycle. It is
* therefore critical that any code holding this lock complete as
quickly
* as possible, allocate no new objects, and avoid calling user
code.
*/
static private class Lock { }
private static Lock lock = new Lock();
The conflicting part is "allocate no new objects". Catching OOME
inside
a synchronized block holding this lock implies that new objects
could be
allocated. I have a feeling that the 2nd fix prevented that by
pre-loading the Cleaner class at Reference class initialization time.
But because it was hard to reproduce the situation where OOME was
thrown
from (r instanceof Cleaner) check, we nevertheless handled this
hypothetical situation. Perhaps it would be better that we didn't and
just see if OOME returned after just adding the pre-loading of Cleaner
class...
So here we are, at an attempt to clean this up:
https://bugs.openjdk.java.net/browse/JDK-8055232
We can move (r instanceof Cleaner) check outside of synchronized block
to where it was before the 2nd fix and wait what stress tests will
show.
Another possibility is to move the instanceof check outside of
synchronized block, but handle the hypothetical OOME by re-linking the
unlinked reference back into the pending chain:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ReferenceHandlerExceptions/webrev.01/
What would you suggest?
Regards, Peter
