Hi Kim, Thanks for taking a look at this code.
On 06/09/2015 04:03 AM, Kim Barrett wrote:
On May 31, 2015, at 3:32 PM, Peter Levart <[email protected]> wrote:So, for the curious, here's the improved prototype: http://cr.openjdk.java.net/~plevart/misc/JEP132/ReferenceHandling/webrev.02/ And here are the improved benchmarks (with some results inline): http://cr.openjdk.java.net/~plevart/misc/JEP132/ReferenceHandling/refproc/While perusing the offered code (webrev.02) (not a careful review yet), I think I've found a serious bug: In Reference.java in unhookPendingChunk 253 // assert r.next == r; 254 // move a chunk of pending/discovered references to a 255 // temporary local r/next chain ... 262 rd.next = r; I think that assertion suggested at line 253 does not hold, and line 262 may damage a queue. The problem is that the application could explicitly enqueue a reference while it is sitting in the pending list, waiting to be processed. So the enqueue places the reference on the associated queue's list, linked through the reference's next field. Then unhook comes along and clobbers the reference's next field. Oops! handlePendingChunk has similar problems with an application enqueue before the reference has been "handled". I haven't looked carefully at webrev.03, but at a quick glance it looks like it has the same problem. (Which is what I expected, given the description of the differences between webrev.02 and webrev.03.) I'm not sure why unhook is using the next field at all, rather than just continuing to use the discovered field. I've not studied this idea carefully, but I think it might work to leave the chunks linked through the discovered field until addChunk, with that being responsible for self-looping the discovered field and transferring linkage to the next field. That is, treat chunks as extensions of the pending list until addChunk-time. There might be some that makes using the discovered field that way a problem, but I'm not thinking of anything right now. Of course, this problem doesn't exist for FinalReference because application code doesn't have access to them to perform the problematic explicit enqueue.
Ops, you're right. Explicit enqueue-ing is what skipped my mind since I've been 1st working on finalizers and then generalized the solution... I'm afraid to use the 'discovered' field since it is in the domain of VM and I think Java side can only reset it to null while holding the lock. I also would not like to add another field to Reference just to support "chunking". Perhaps the chunk could be formed as a re-usable array of references. Let me think about this some more to figure out how to solve it most elegantly...
I'll follow-up when I have something. Regards, Peter
