Should these migrate to jboss-common so they can come from there in future releases? Or if not come from there for JBC use, be available to projects outside JBC? Manik Surtani wrote: > Guys, > I have created 3 new collection impls in trunk - ImmutableListCopy, > ImmutableSetCopy and ImmutableMapCopy. I have also ported the > first 2 to branch 2.2.x (the reason why I didnt port the third is > because the MapCopy which it replaces can get serialized and I > don't want to break binary compat). > See the Javadoc of these classes for more details, but in a > nutshell these are very efficient alternatives to the "immutable > and defensively copied" idiom. I.e., if you ever find yourself > doing: > return Collections.unmodifiableList(new ArrayList(internalList)) > you should consider ImmutableListCopy instead. Constructon time is > about a tenth of the combined time it takes to create an ArrayList > and then wrap it in an UnmodifiableList. Also, iteration is very > quick since the internal array is immutable. Please keep this in > mind when working on JBC code. > Cheers, > -- > Manik Surtani > Lead, JBoss Cache > manik(a)jboss.org > _______________________________________________ > jbosscache-dev mailing list > jbosscache-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/jbosscache-dev -- Brian Stansberry Lead, AS Clustering JBoss, a division of Red Hat brian.stansberry(a)redhat.com

Manik Surtani wrote: > Guys, > I have created 3 new collection impls in trunk - ImmutableListCopy, > ImmutableSetCopy and ImmutableMapCopy. I have also ported the > first 2 to branch 2.2.x (the reason why I didnt port the third is > because the MapCopy which it replaces can get serialized and I > don't want to break binary compat). I took a look at adjusting these to be more efficient with the FastCopyHashMap. After doing some analysis and sanity benchmarks, I have determined that we should switch to unmodifiableXXX(obj.clone() or copy constructor) in all cases, for the following reasons: 1. There is no performance advantage with lists.

2. While the construction time in the Map and Set variants is faster, since it doesn't use hashing, it sacrifices way too much (O(n) lookup times). I did verify that the performance difference will be the same with just about every map implementation, since it's mostly allocation that is being measured here.

3. It prevents the copy optimization in FastCopyHashMap, so removing usage of ImmutableMapCopy will improve copy performance of anything using FastCopyHashMap.

4. The construction of the immutable wrapper is barely noticeable, and it's done per call, so the value is likely temporary.

Also I noticed that we have some duplicate copying going on. For example , CacheInvocationDelegate copies a set that is constructed by the command, per call.

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.

>> >> 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.

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

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.

Jason T. Greene wrote: >>> 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. :-) > Ok, I'll take a look at those. I remember them passing for me before. This is fixed now. I never saw the failure because the failing test because I was executing the wrong test profile. I thought it finished a little too early :)

>> 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? > Yeah I was originally planning on doing a instanceof check on known > types, I just forgot to commit that update. It is using clone though. This is done now. It uses clone because the copy-ctor of almost all the maps increases the size of the table.

>> 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. > Yeah I agree it's a screwed up design. The other day I was thinking this would be so easy to fix in the JDK. Add a new interface called ReallyCloneable that extends Cloneable and requires the clone method. Then introduce a System.clone(Object object) which is smart enough to try ReallyCloneble first, and then barring that use reflection.