RFR: 8156500: deadlock provoked by new stress test com/sun/jdi/OomDebugTest.java

[Per Liden]

Hi Kim,

On 2016-08-09 03:25, Kim Barrett wrote:
>> On Aug 8, 2016, at 8:16 AM, Per Liden <per.liden at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> On 2016-08-01 20:47, Kim Barrett wrote:
>>> This updated webrev addresses concerns about the performance of the VM
>>> API used by the reference processor thread in the original webrev.
>>>
>>> New webrevs:
>>> full: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03/
>>>      http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03/
>>> incr: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.03.incr/
>>>      http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.03.incr/
>>
>> Overall I think you managed to hit a good balance here, keeping the VM API straight forward without loosing flexibility. Having the ReferenceHandler doing the actual work seems sound and avoids some complexity, which I like.
>
> Thanks for looking at this.
>
>> I have one suggestion though, regarding CheckReferencePendingList(). While reviewing I found that I had to check several times what its return value actually meant, the "check" part of the name doesn't quite reveal that.
>> Further, it seems to me that the waiting path of this function has fairly little in common with the non-waiting path, e.g. it always returns true. So, to make both the naming and implementation more clear I'd like to suggest that we split this into two separate functions, HasReferencePendingList() and WaitForReferencePendingList(), like this:
>
> I was thinking about splitting things way, and ended up not doing so
> for no good reason I can think of. And it does seem clearer that way,
> so...
>
> New webrevs:
> full: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.04/
>       http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.04/
> incr: http://cr.openjdk.java.net/~kbarrett/8156500/jdk.04.inc/
>       http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.04.inc/

Thanks for fixing, looks good.

cheers,
Per

>
>> Other than this I think the patch looks good.
>>
>>>
>>> The originally offered approach was an attempt to minimize changes.  I
>>> was trying to be as similar to the existing code as possible, while
>>> moving part of it into the VM, where we have the necessary control
>>> over suspension and the like.  We already know we want to make changes
>>> in this area, in order to eliminate the use of
>>> jdk.internal.ref.Cleaner (JDK-8149925).  But we've also agreed that
>>> JDK-8149925 wasn't urgent and to defer it to JDK 10.  I don't think we
>>> should revisit that.
>>
>> Is JDK-8149925 the correct bug id? Looks like that bug has already been fixed in 9.
>
> That CR was originally for removing all the uses.  During review the
> nio.Bits / direct buffer use was separated, and is the last one.
> There was a bunch of discussion about removing that final one in that
> review (especially later parts)
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/038663.html
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039315.html
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/039862.html
>
> and also here:
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039561.html.
>
> However, I don't see a followup CR for removing the nio.Bits use.
> Maybe that didn't get created when we split the issue, or maybe I'm
> just failing to find it.  Peter?
>
> Note that the consensus back in March/April was to defer it, along
> with the dependent actual removal of the internal Cleaner, to JDK 10.
>
>