[jbosscache-dev] New collections

[Manik Surtani]

On 7 Aug 2008, at 05:35, Jason T. Greene wrote:

> Manik Surtani wrote:
>> On 5 Aug 2008, at 14:42, Jason T. Greene wrote:
>>>>>
>>>>> What I would like to do, provided everyone is ok with this, is  
>>>>> switch this code to static factory methods like so:
>>>>>
>>>>> Immutables.immutableCopy(Map map);
>>>>> Immutables.immutableCopy(List list);
>>>>> Immutables.immutableCopy(Set set);
>>>>>
>>>>> These will then just call clone and Collections.unmodifiableXXX  
>>>>> as appropriate. We can then add optimizations later if they  
>>>>> become available.
>>>> Would these be more generic, to suit any collection  
>>>> implementation?  Remember that not all Maps, for example,  
>>>> implement Cloneable or have a copy constructor.  And wrapping any  
>>>> Map passed in as a FastCopyHashMap may break correctness, for  
>>>> example, if a SortedMap were passed in.
>>>
>>> Right, they would use instanceof to check all the known map types  
>>> as a fast-path optimization. After that it would check for  
>>> cloneable, and use reflection to invoke clone. It would then look  
>>> for a copy constructor, and failing that it would then create a  
>>> different collection type. We could for example, in the case of a  
>>> non-copyable SortedMap, use a TreeMap, and order is maintained.
>> Right yes, you would need the logic in place to do this.
>> -- 
>
> Ok, today I went ahead and made these changed (commit 6536). So in  
> case anything breaks, you know why :) I did run through the unit  
> tests locally though, with no failures.

I think it did break a few things - see the tests in  
o.j.c.notifications.  :-)

Also, looking at Immutables.immutableMapCopy(), isn't it inefficient  
that you attempt to call clone() using reflection, catch exceptions on  
failure, then try a copy constructor, catch exception on failure, and  
finally attempt a new HashMap()?  Given that this would most likely be  
used with either a FastCopyHashMap or a HashMap, shouldn't we just  
check for instanceof of these types and use the copy constructor  
directly, especially given that there is hardly any performance diff  
between FCHM.clone() and it's copy constructor?

Cloneable is such an arse interface - they really should have exposed  
clone() as a public method there.  Solves a lot of problems, no need  
for reflection, etc.


> There is now a Immutables factory class that does copying,  
> conversion, and wrapping of list, set, collection, and map. So as  
> long as everything uses this, it should be easy to swap out  
> implementations.
>
> As suggested, I went ahead and left ImmutableListCopy, however the  
> other types use a wrapped clone/copy. Since the marshalling code  
> needs to detect these types, I ended up having to reimplement the  
> Collections.unmodifiableXXX types (not a lot of code though).
>
> These changes saved another copy in the unmarshalling code, since it  
> needs to construct a new HashMap anyway. In this scenario, the  
> Immutables.immutableMapWrap() is used, which does not copy.
>
> Also the duplicate copy in CacheInvocationDelegate.getChildrenNames  
> was also removed using the set variant of the wrap method.
>
> -- 
> Jason T. Greene
> JBoss, a division of Red Hat

--
Manik Surtani
Lead, JBoss Cache
manik at jboss.org