Review for CR 6728865 : Improved heuristics for Collections.disjoint() [updated]

[Chris Hegarty]

On 12/21/10 02:24 AM, David Holmes wrote:
> Functionality looks okay to me.
>
> I think those spec/doc clarifications may need to go through CCC though.

I agree with David, a CCC request should be filed for the spec changes. 
We should agree the spec changes on this mailing list before proposing them.

I understand where you're coming from with this spec change, but I think 
the additional text may be too restricting.

   "@throws NullPointerException ......... or if one collection
    contains {@code null} and the other collection does not permit
    {@code null} values."

For example, the following would be required to throw NPE ( but I don't 
believe your impl does):

   Set set = new HashSet();
   set.add(null);
   PriorityQueue pq = new PriorityQueue();
   Collections.disjoint(set, pq);

I think we may have to be a little more relaxed here, maybe just a 
cautionary note, "it may happen"???

-Chris.

>
> David
>
> Mike Duigou said the following on 12/21/10 11:48:
>> I've updated the webrev with Ulf's feedback.
>> http://cr.openjdk.java.net/~mduigou/6728865.2/webrev/
>>
>> The old heuristics:
>>
>> 1. If c1 is a Set then iterate over c2.
>>
>> 2. Iterate over the smaller Collection.
>>
>>
>> I believe that the || in the original should have been a &&
>>
>> I've rearranged it as branches in my revision.
>>
>>
>> The new heuristics:
>>
>> 1. If c1 is a Set then iterate over c2.
>>
>> 2. If c2 is a Set then iterate over c1.
>>
>> 3. If either collection is empty then result is always true.
>>
>> 4. Iterate over the smaller Collection.
>>
>> Mike
>>
>>
>>
>> On Dec 19 2010, at 16:42 , David Holmes wrote:
>>
>>> Hi Mike,
>>>
>>> Mike Duigou said the following on 12/20/10 10:29:
>>>> I have updated the webrev for CR 6728865 with Rémi's feedback. The
>>>> new webrev is at:
>>>> http://cr.openjdk.java.net/~mduigou/6728865.1/webrev/
>>>> The size() comparisons are now done only when both c1 and c2 are not
>>>> sets and I have removed the isEmpty() micro-optimization.
>>> So to summarise this change:
>>>
>>> 1. The original code checked for c1 being a set and not c2, but not
>>> vice-versa - this fixes that
>>>
>>> 2. This code adds an optimization when they are both not sets but at
>>> least one is empty
>>>
>>> Did I miss anything?
>>>
>>> Seems functionally sound to me, but I can't attest to any performance
>>> benefits.
>>>
>>> Cheers,
>>> David Holmes
>>