Re: [jbosscache-dev] New collections
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.
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