Hi Kim,
On 2016-08-09 03:25, Kim Barrett wrote:
On Aug 8, 2016, at 8:16 AM, Per Liden <per.li...@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.
