On 19 Jul 2017 18:00, "Guillaume Smet" <guillaume.smet(a)gmail.com&gt; 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(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

Hi all, One general challenge for HV is that we cannot easily determine the set of validated classes upfront. Neither is there means of listing all validated types explicitly (akin to JPA's <class> in persistence.xml), nor is it sufficient to check classes for a limited set of well-known annotations to identify the relevant classes (as on of BV's premises is support for custom constraint types). Building up the meta-data eagerly would avoid retaining memory for the "raw model", on the other hand it means we build up instances in the "aggregated model" for classes which potentially are never validated. So I'm not really sure whether that's better. But if we decide to build up the metadata eagerly, this should be based on Jandex IMO, otherwise we'd end up with re-implementing lots of that ourselves. On specific question would be how to limit the scope of such scan, as there isn't the notion of something similar to "persistence archives" in JPA. But then we wouldn't want to scan all the JARs on the classpath. Limiting it to the classes of the current deployment seems reasonable in WildFly. But in SE (say when using HV with Spring on Tomcat) we'd need a way to express which JARs to scan. +1, caching instances of the raw model seems not too useful, e.g. typically people won't derive dozens of sub-types from one base class in their model (which is the case where the current caching would be beneficial, as the metadata for the base class would only be retrieved once). A variation of the current scheme could be to process all types of a hierarchy upon first validation of any type in that hierarchy. Say we have a class A with sub-classes B and C. When validating B for the first time, we'd build up the metadata for A, B and C, caching the raw metadata for A just in the context of this call. This requires though to reliably identify all sub-classes of A, which cannot be done using standard reflection. Jandex could help (though we cannot keep the index at runtime, so we'd have to retrieve and store just the inheritance relationships from the index). The overriding rules are checked when merging the raw metadata into the aggregated model, so indeed I'd think we could omit that method metadata from the aggregated model in this case. Definitely worth exploring. Same for property metadata. I've filed https://hibernate.atlassian.net/browse/HV-1447 for looking into this. Calls to BeanMetaData#getMetaDataFor() could return an empty placeholder implementation then. Another thing I've been pondering on is the implementation of the public metamodel (defined by BV API). Currently that's independent from our own (aggregated) metamodel and thus allocates another bit of memory. We could try and implement the BV metamodel interfaces directly with our own metamodel model types. E.g. BeanMetaData#getBeanDescriptor() would then simply return itself. This should save some memory currently allocated for the nodes of the external model. --Gunnar 2017-07-23 22:23 GMT+02:00 Guillaume Smet <guillaume.smet(a)gmail.com&gt;:

You would need to hardcode the definition of Object in HV's code. It's unclear to me that it would not be detrimental. I'd keep the model generic. And favor metadata recomputation over caching.