On 19 Jul 2017 18:00, "Guillaume Smet" <guillaume.s...@gmail.com> wrote:
Hi, Stuart Douglas (in CC) opened an interesting issue here: https://hibernate.atlassian.net/browse/HV-1437 I already made some good progress here: https://github.com/hibernate/hibernate-validator/pull/814 but I would appreciate some feedback on a few additional things. ## AnnotationMetaDataProvider#configuredBeans So, in AnnotationMetaDataProvider, we keep a cache of the annotation metadata found while building the aggregated BeanMetaData. It might be useful if you have a class hierarchy for instance as you would only build the annotation of the parent class once. But... as we initialize the bean metadata lazily, we need to keep this cache during the whole lifecycle of the application. I'm not familiar enough with the whole picture but I strongly suspect you should explore ways to get out of this lazy initialization strategy. Maybe keep building it lazily during bootstrap(s) but then add a "drop cached metadata" hook which containers could invoke explicitly at the end of bootstrap of an app? Worst case you'll have to rebuild the metadata on demand. So, here, we have to find a compromise: 1/ either favor the memory footprint but the annotation of a given class could be analyzed several times in the case of a class hierarchy 2/ or favor the startup cost (it's not a runtime cost, it's paid only once when validating the first bean of a given class) and have a higher memory footprint I guess my proposal above is 3/, trying to have both benefits. Usually, in HV, we take the 1/ route so I was a bit surprised 2/ was chosen here. Thoughts? ## Collection resizing So, interestingly, enough, the small CollectionHelper#toImmutable* utilities I introduced make quite a difference if there are a lot of empty/one element collections - which is the general case in Stuart's example. The idea of these utilities is quite simple: instead of blindly wrapping a collection using Collections#unmodifiable* to make a collection immutable, we test the size of the collection and we return a static empty collection or a singleton collection if the size is 0 or 1 respectively. So this is nice when the size is 0 or 1 but considering that HV metadata structures are mostly immutable once they are initialized, I would like to go a step further and create a collection of the right size when making it immutable in all the cases. We don't use that many collection types and we could default to the current wrapping method if the collection is not ArrayList, HashSet, LinkedHashSet, HashMap and maybe LinkedHashMap. Basically, for sets, we would have: public static <T> Set<T> toResizedImmutableSet(Set<? extends T> set) { switch ( set.size() ) { case 0: return Collections.emptySet(); case 1: return Collections.singleton( set.iterator().next() ); default: if ( set instanceof LinkedHashSet ) { LinkedHashSet<T> copy = new LinkedHashSet<T>( set.size(), 1 ); copy.addAll( set ); return Collections.unmodifiableSet( copy ); } else if ( set instanceof HashSet ) { HashSet<T> copy = new HashSet<T>( set.size(), 1 ); copy.addAll( set ); return Collections.unmodifiableSet( copy ); } else { return Collections.unmodifiableSet( set ); } } } It's one more memory allocation but I think reducing the runtime memory footprint would be worth it. Especially for data structures that are very common (think of the executable metadata, the parameter metadata and so on). I'm not sure I would generalize it to all our projects but I think it makes sense for HV where nearly everything is immutable and we have quite a lot of Maps and Sets in the metadata. Usually, Yoann and I are on the "Let's do it" side and the others on the "I'm not sure it's worth it" when it comes to CollectionHelper, but considering how well the first round has paid, I think we should at least give it a try. I'm also quite sure it's worth applying such optimisations. I'm only skeptical about the value of sharing such code via shared libraries. I'd even go a step further : try avoiding wrapping into immutable when those collections are exposed exclusively to code we directly control. The JIT can do much magic but it won't avoid allocating the wrappers so that's not cheap at all, but that's of course a maintenance / clean code / performance tradeoff. Would be great to validate automatically that we treat them as effectively immutable, maybe that's possible via annotations and some code validation tools? Thoughts? ## Metadata, metadata, metadata So, we have a lot of metadata. A. Lot. Especially, we have metadata even when there are no HV metadata at all. Think of Keycloak's RealmEntity ( https://github.com/keycloak/keycloak/blob/master/model/ jpa/src/main/java/org/keycloak/models/jpa/entities/RealmEntity.java), it makes a lot of metadata for no useful information. It's especially the method metadata that are heavy, even if I tried to reduce them to the minimum in this case. I once thought about completely removing the method metadata if the method wasn't annotated at all but then I thought that the overriding rules would prevent us to do that. Gunnar, am I right on this? So basically, I think we can't really do anything about this. Drop it as soon as we figure it's not useful? I also thought that it was useless to look for annotations on java.lang.Object but then again I think the overriding rules force us to do so. I'm not following here. Why is it not safe to skip annotations on Object? That's it for today. Comments welcome. -- Guillaume _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
