RFR: 7161229 - PriorityBlockingQueue keeps hard reference to last removed element

[David Holmes]

Hi Remi,

On 25/06/2012 7:01 PM, Rémi Forax wrote:
> Hi David,
> I think the test
>
> if (n > 0) {
>
> should not be in siftDown* but should be done at all callsites,
> by example in heapify, you can hoist the test outside of the loop.

I originally had it at the call-site (it already exists in a number of 
them) but Doug prefers it to be internalized. He wrote:

"... a better strategy is to move the n > 0 check to the siftDown code 
itself, so (existing and potential) callers don't need the check.
... an explicit n > 0 check acts to hoist array index checks in loop so 
turns out to be faster in general."

> In dequeue, it's a matter of style but the 'else' is not needed anymore.
>
> Otherwise the changes looks ok for me.

Thanks,
David

> Rémi
>
> On 06/25/2012 06:26 AM, David Holmes wrote:
>> webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/7161229/webrev/
>>
>> When removing the last element from the PBQ the "sift down" logic
>> would store it back in as array[0]. The simple fix is for the
>> sift-down to be a no-op if the queue size is zero.
>>
>> The fix has been contributed by Doug Lea. We are taking this
>> opportunity to synchronize PBQ with Doug's latest version at:
>>
>> http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/PriorityBlockingQueue.java?view=log
>>
>>
>> So in addition to the above functional fix there are a number of
>> miscellaneous code and comment cleanups. The only non-trivial, but
>> still very simple, change is to the drainTo method to make it more
>> robust if the add() on the destination collection throws an exception.
>>
>> I am of course a Reviewer for this.
>>
>> I have provided the update to the LastElement test (as this is
>> certainly related to the last element) but there are some reservations
>> about examining the PBS internals this way. Unfortunately there is no
>> good way to test for these kinds of retention issues. You either need
>> a whitebox test like this, or need to rely on non-guaranteed GC and
>> finalization behaviour.
>>
>> Thanks,
>> David Holmes
>
>