diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/ConstraintD.java b/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/ConstraintD.java index 124bafdf11..43c19e8698 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/ConstraintD.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/ConstraintD.java @@ -82,6 +82,8 @@ private static Set set(Supplier array) { private final List>> constraintValidatorClasses; // racy single-check memoization: the annotation is immutable, so concurrent initializations agree private String messageTemplate; + /** Memo of this constraint's entry in the factory's validator cache. */ + private volatile ConstraintValidator validator; private final Lazy toString = new Lazy<>(() -> String.format("%s: %s", ConstraintD.class.getSimpleName(), getAnnotation())); @@ -171,6 +173,24 @@ public U unwrap(Class type) throws ValidationException { } } + /** + * Get the initialized {@link ConstraintValidator} previously recorded with {@link #setValidator}. + * + * @return {@link ConstraintValidator} or {@code null} + */ + public ConstraintValidator getValidator() { + return validator; + } + + /** + * Record the initialized {@link ConstraintValidator} the factory caches for this constraint. + * + * @param validator + */ + public void setValidator(ConstraintValidator validator) { + this.validator = validator; + } + public Scope getScope() { return scope; } diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/DescriptorManager.java b/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/DescriptorManager.java index 2d3081ea9e..fbf132d294 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/DescriptorManager.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/DescriptorManager.java @@ -38,6 +38,7 @@ import org.apache.bval.jsr.metadata.HierarchyBuilder; import org.apache.bval.jsr.metadata.MetadataBuilder; import org.apache.bval.jsr.metadata.ReflectionBuilder; +import org.apache.bval.jsr.util.Proxies; import org.apache.bval.util.Validate; public class DescriptorManager { @@ -59,8 +60,8 @@ public static boolean isConstrained(E descripto private final ApacheValidatorFactory validatorFactory; private final ConcurrentMap, BeanD> beanDescriptors = new ConcurrentHashMap<>(); - /** Same meaning as {@link BeanDescriptor#isBeanConstrained()} — avoids loading metadata on repeat {@code hasWork} checks. */ - private final ConcurrentMap, Boolean> beanConstrainedByType = new ConcurrentHashMap<>(); + /** Keyed by the runtime class of validated objects, which may be a proxy of the described class. */ + private final ConcurrentMap, BeanD> beanDescriptorsByRuntimeClass = new ConcurrentHashMap<>(); // synchronization unnecessary private final ReflectionBuilder reflectionBuilder; @@ -70,32 +71,40 @@ public DescriptorManager(ApacheValidatorFactory validatorFactory) { this.reflectionBuilder = new ReflectionBuilder(validatorFactory); } - /** - * @return cached result of {@link BeanDescriptor#isBeanConstrained()} for this class, or {@code null} if unknown - */ - public Boolean getCachedBeanConstrained(Class beanClass) { - return beanConstrainedByType.get(beanClass); - } - public BeanDescriptor getBeanDescriptor(Class beanClass) { Validate.notNull(beanClass, IllegalArgumentException::new, "beanClass"); // cannot use computeIfAbsent due to recursion being the usual case: final BeanD existing = beanDescriptors.get(beanClass); if (existing != null) { - beanConstrainedByType.put(beanClass, existing.isBeanConstrained()); return existing; } final BeanD value = new BeanD<>(new MetadataReader(validatorFactory, beanClass).forBean(builder(beanClass))); final BeanD previous = beanDescriptors.putIfAbsent(beanClass, value); - final BeanD result = previous == null ? value : previous; - beanConstrainedByType.put(beanClass, result.isBeanConstrained()); + return previous == null ? value : previous; + } + + /** + * Get the descriptor for an object of runtime type {@code runtimeClass}, which is first unwrapped if it is a + * proxy class. + * + * @param runtimeClass + * @return {@link BeanD} + * @see Proxies#classFor(Class) + */ + public BeanD getBeanDescriptorForRuntimeClass(Class runtimeClass) { + final BeanD existing = beanDescriptorsByRuntimeClass.get(runtimeClass); + if (existing != null) { + return existing; + } + final BeanD result = (BeanD) getBeanDescriptor(Proxies.classFor(runtimeClass)); + beanDescriptorsByRuntimeClass.putIfAbsent(runtimeClass, result); return result; } public void clear() { beanDescriptors.clear(); - beanConstrainedByType.clear(); + beanDescriptorsByRuntimeClass.clear(); } private MetadataBuilder.ForBean builder(Class beanClass) { diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/PropertyD.java b/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/PropertyD.java index 3a89c6cc1c..2fdd70f22f 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/PropertyD.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/PropertyD.java @@ -16,6 +16,11 @@ */ package org.apache.bval.jsr.descriptor; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodHandles.Lookup; +import java.lang.invoke.MethodType; +import java.lang.reflect.AccessibleObject; import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; @@ -48,6 +53,16 @@ public String getPropertyName() { @Override public Object getValue(Object parent) throws Exception { + final MethodHandle getter = getter(); + if (getter != null) { + try { + return (Object) getter.invokeExact(parent); + } catch (Exception | Error e) { + throw e; + } catch (Throwable t) { + throw new IllegalArgumentException(t); + } + } Reflection.makeAccessible(getTarget()); try { return getTarget().get(parent); @@ -55,6 +70,11 @@ public Object getValue(Object parent) throws Exception { throw new IllegalArgumentException(e); } } + + @Override + MethodHandle unreflect(Lookup lookup) throws IllegalAccessException { + return lookup.unreflectGetter(getTarget()); + } } static class ForMethod extends PropertyD { @@ -70,6 +90,15 @@ public String getPropertyName() { @Override public Object getValue(Object parent) throws Exception { + final MethodHandle getter = getter(); + if (getter != null) { + try { + return (Object) getter.invokeExact(parent); + } catch (Throwable t) { + // same wrapping as Method#invoke followed by the catch below + throw new IllegalArgumentException(new InvocationTargetException(t)); + } + } Reflection.makeAccessible(getTarget()); try { return getTarget().invoke(parent); @@ -77,8 +106,24 @@ public Object getValue(Object parent) throws Exception { throw new IllegalArgumentException(e); } } + + @Override + MethodHandle unreflect(Lookup lookup) throws IllegalAccessException { + return lookup.unreflect(getTarget()); + } } + private static final MethodType GETTER_TYPE = MethodType.methodType(Object.class, Object.class); + + /** + * Reading through a cached {@link MethodHandle} avoids the slow path that {@link Field#get(Object)} and + * {@link Method#invoke(Object, Object...)} take when the reflective object is not a JIT constant. Holds + * {@link #NO_GETTER} when no handle can be created, in which case reflection is used. + */ + private volatile MethodHandle getter; + + private static final MethodHandle NO_GETTER = MethodHandles.constant(Object.class, null); + protected PropertyD(MetadataReader.ForContainer reader, BeanD parent) { super(reader, parent); } @@ -96,4 +141,23 @@ public final Stream read(GraphContext context) { } public abstract Object getValue(Object parent) throws Exception; + + abstract MethodHandle unreflect(Lookup lookup) throws IllegalAccessException; + + /** + * Get a {@code (Object)Object} handle reading this property, or {@code null} if none could be created. + */ + final MethodHandle getter() { + MethodHandle result = getter; + if (result == null) { + try { + Reflection.makeAccessible((AccessibleObject) getTarget()); + result = unreflect(MethodHandles.lookup()).asType(GETTER_TYPE); + } catch (RuntimeException | IllegalAccessException e) { + result = NO_GETTER; + } + getter = result; + } + return result == NO_GETTER ? null : result; + } } diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/groups/GroupStrategy.java b/bval-jsr/src/main/java/org/apache/bval/jsr/groups/GroupStrategy.java index 343283c8fc..4f2cfc7399 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/groups/GroupStrategy.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/groups/GroupStrategy.java @@ -36,6 +36,7 @@ public interface GroupStrategy { public static class Simple implements GroupStrategy { private final Set groups; + private int hashCode; private Simple(Set groups) { this.groups = groups; @@ -57,7 +58,12 @@ public GroupStrategy redefining(Map redefinition @Override public int hashCode() { - return groups.hashCode(); + int result = hashCode; + if (result == 0) { + result = groups.hashCode(); + hashCode = result; + } + return result; } @Override @@ -75,6 +81,7 @@ public String toString() { public static class Composite implements GroupStrategy { private final Set components; protected final boolean ordered; + private int hashCode; public Composite(Collection components, boolean ordered) { this.components = new LinkedHashSet<>(components); @@ -117,7 +124,12 @@ public boolean applyTo(Predicate operation) { @Override public int hashCode() { - return Objects.hash(components, ordered); + int result = hashCode; + if (result == 0) { + result = Objects.hash(components, ordered); + hashCode = result; + } + return result; } @Override diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/groups/Groups.java b/bval-jsr/src/main/java/org/apache/bval/jsr/groups/Groups.java index 92eefb40b7..7df749cd8d 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/groups/Groups.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/groups/Groups.java @@ -38,6 +38,7 @@ public class Groups { private final Set groups = new LinkedHashSet<>(); private final Set sequences = new LinkedHashSet<>(); + private volatile GroupStrategy strategy; /** * Get the Groups. @@ -65,6 +66,7 @@ public Collection getSequences() { * @return success */ boolean insertGroup(Group group) { + strategy = null; return groups.add(group); } @@ -76,6 +78,7 @@ boolean insertGroup(Group group) { * @return success */ boolean insertSequence(Collection groups) { + strategy = null; return !(groups == null || groups.isEmpty()) && sequences.add(Group.sequence(groups)); } @@ -120,9 +123,14 @@ private void ensureExpandable(List groupList, List defaultGroupLis } public GroupStrategy asStrategy() { - final List components = new ArrayList<>(); - components.addAll(groups); - components.addAll(sequences); - return GroupStrategy.composite(components); + GroupStrategy result = strategy; + if (result == null) { + final List components = new ArrayList<>(); + components.addAll(groups); + components.addAll(sequences); + result = GroupStrategy.composite(components); + strategy = result; + } + return result; } } \ No newline at end of file diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/groups/GroupsComputer.java b/bval-jsr/src/main/java/org/apache/bval/jsr/groups/GroupsComputer.java index 5a5e4e6a9d..aca11d7e2c 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/groups/GroupsComputer.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/groups/GroupsComputer.java @@ -61,6 +61,9 @@ public class GroupsComputer { /** caching resolved groups in a thread-safe map. */ private final Map, List> resolvedSequences = new ConcurrentHashMap<>(); + /** Results of {@link #computeGroups(Class...)} for a single group, the common case for explicit groups. */ + private final Map, Groups> singleGroups = new ConcurrentHashMap<>(); + /** * Compute groups from an array of group classes. * @@ -74,6 +77,15 @@ public final Groups computeGroups(Class... groups) { if (groups.length == 0 || (groups.length == 1 && groups[0] == Default.class)) { return DEFAULT_GROUPS; } + if (groups.length == 1 && groups[0] != null) { + final Groups cached = singleGroups.get(groups[0]); + if (cached != null) { + return cached; + } + final Groups result = computeGroups(Arrays.asList(groups)); + singleGroups.putIfAbsent(groups[0], result); + return result; + } return computeGroups(Arrays.asList(groups)); } diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/job/ConstraintValidatorContextImpl.java b/bval-jsr/src/main/java/org/apache/bval/jsr/job/ConstraintValidatorContextImpl.java index 2f88423f95..39bd290f2a 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/job/ConstraintValidatorContextImpl.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/job/ConstraintValidatorContextImpl.java @@ -18,8 +18,8 @@ */ package org.apache.bval.jsr.job; -import java.util.HashSet; -import java.util.Set; +import java.util.ArrayList; +import java.util.List; import jakarta.validation.ClockProvider; import jakarta.validation.ConstraintValidatorContext; @@ -40,7 +40,6 @@ import org.apache.bval.jsr.util.NodeImpl; import org.apache.bval.jsr.util.PathImpl; import org.apache.bval.util.Exceptions; -import org.apache.bval.util.Lazy; import org.apache.bval.util.Validate; public class ConstraintValidatorContextImpl implements ConstraintValidatorContext, ApacheMessageContext { @@ -136,7 +135,8 @@ private PathImpl extensiblePath() { private final ValidationJob.Frame frame; private final ConstraintD constraint; - private final Lazy>> violations = new Lazy<>(HashSet::new); + // typically holds a single violation; a list with a containment check keeps set semantics cheaply + private List> violations; private boolean defaultConstraintViolationDisabled; ConstraintValidatorContextImpl(ValidationJob.Frame frame, ConstraintD constraint) { @@ -178,14 +178,14 @@ ValidationJob.Frame getFrame() { return frame; } - Set> getRequiredViolations() { - if (!violations.optional().isPresent()) { + List> getRequiredViolations() { + if (violations == null) { if (defaultConstraintViolationDisabled) { Exceptions.raise(ValidationException::new, "Expected custom constraint violation(s)"); } addError(getDefaultConstraintMessageTemplate(), frame.context.getPath()); } - return violations.get(); + return violations; } @Override @@ -200,7 +200,14 @@ public Object getValidatedValue() { @SuppressWarnings({ "unchecked", "rawtypes" }) private void addError(String messageTemplate, PathImpl propertyPath) { - violations.get().add(((ValidationJob) frame.getJob()).createViolation(messageTemplate, this, propertyPath)); + if (violations == null) { + violations = new ArrayList<>(2); + } + final ConstraintViolation violation = + ((ValidationJob) frame.getJob()).createViolation(messageTemplate, this, propertyPath); + if (!violations.contains(violation)) { + violations.add(violation); + } } @Override diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/job/ValidateBean.java b/bval-jsr/src/main/java/org/apache/bval/jsr/job/ValidateBean.java index ee22b180fd..381604c4d6 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/job/ValidateBean.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/job/ValidateBean.java @@ -23,16 +23,13 @@ import org.apache.bval.jsr.GraphContext; import org.apache.bval.jsr.descriptor.BeanD; import org.apache.bval.jsr.descriptor.ConstraintD; -import org.apache.bval.jsr.descriptor.DescriptorManager; import org.apache.bval.jsr.util.PathImpl; -import org.apache.bval.jsr.util.Proxies; import org.apache.bval.util.Validate; -import java.util.Map; - public final class ValidateBean extends ValidationJob { private final T bean; + private BeanD descriptor; ValidateBean(ApacheFactoryContext validatorContext, T bean, Class[] groups) { super(validatorContext, groups); @@ -41,24 +38,16 @@ public final class ValidateBean extends ValidationJob { @Override protected boolean hasWork() { - final Class beanClass = bean.getClass(); - final Map, Class> classCache = validatorContext.getFactory().getUnwrappedClassCache(); - Class unwrappedClass = classCache.get(beanClass); - if (unwrappedClass == null) { - unwrappedClass = Proxies.classFor(beanClass); - classCache.putIfAbsent(beanClass, unwrappedClass); - } - final DescriptorManager dm = validatorContext.getFactory().getDescriptorManager(); - final Boolean cached = dm.getCachedBeanConstrained(unwrappedClass); - if (cached != null) { - return cached; - } - return dm.getBeanDescriptor(unwrappedClass).isBeanConstrained(); + @SuppressWarnings("unchecked") + final BeanD d = + (BeanD) validatorContext.getDescriptorManager().getBeanDescriptorForRuntimeClass(bean.getClass()); + descriptor = d; + return d.isBeanConstrained(); } @Override protected Frame> computeBaseFrame() { - return new BeanFrame(new GraphContext(validatorContext, PathImpl.create(), bean)); + return new BeanFrame(null, new GraphContext(validatorContext, PathImpl.create(), bean), descriptor); } @SuppressWarnings("unchecked") diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/job/ValidationJob.java b/bval-jsr/src/main/java/org/apache/bval/jsr/job/ValidationJob.java index 3c33c555aa..6f469f4584 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/job/ValidationJob.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/job/ValidationJob.java @@ -69,7 +69,6 @@ import org.apache.bval.jsr.metadata.ContainerElementKey; import org.apache.bval.jsr.util.NodeImpl; import org.apache.bval.jsr.util.PathImpl; -import org.apache.bval.jsr.util.Proxies; import org.apache.bval.jsr.valueextraction.ExtractValues; import org.apache.bval.jsr.valueextraction.ValueExtractors; import org.apache.bval.util.Exceptions; @@ -199,12 +198,17 @@ private boolean validateComposed(ConstraintD constraint, Consumer constraint) { // Fast path: the validator is cached after first use, so avoid building the (capturing) supplier // lambda and going through computeIfAbsent on the common cache-hit path. + final ConstraintValidator memo = constraint.getValidator(); + if (memo != null) { + return memo; + } final ConstraintValidator existing = validatorContext.getConstraintsCache().getValidators().get(constraint); if (existing != null) { + constraint.setValidator(existing); return existing; } - return validatorContext.getOrComputeConstraintValidator(constraint, () -> { + final ConstraintValidator computed = validatorContext.getOrComputeConstraintValidator(constraint, () -> { final Class constraintValidatorClass = new ComputeConstraintValidatorClass<>(validatorContext.getConstraintsCache(), constraint, getValidationTarget(), computeValidatedType(constraint)).get(); @@ -230,6 +234,8 @@ private ConstraintValidator getConstraintValidator(ConstraintD constraint) { } return constraintValidator; }); + constraint.setValidator(computed); + return computed; } private Class computeValidatedType(ConstraintD constraint) { @@ -254,8 +260,11 @@ public class BeanFrame extends Frame> { } BeanFrame(Frame parent, GraphContext context) { - super(parent, getBeanDescriptor(context.getValue()), - context.child(PathImpl::addBean, context.getValue())); + this(parent, context, getBeanDescriptor(context.getValue())); + } + + BeanFrame(Frame parent, GraphContext context, BeanD descriptor) { + super(parent, descriptor, context.child(PathImpl::addBean, context.getValue())); this.realContext = context; } @@ -589,7 +598,10 @@ private static boolean matchesGroups(ConstraintD constraint, Set targe protected final ApacheFactoryContext validatorContext; protected final Groups groups; - private final Lazy>> results = new Lazy<>(LinkedHashSet::new); + /** Collected violations; {@code null} until the first one is reported. */ + private Set> results; + /** What {@link #getResults()} returns once computed. */ + private Set> resultsView; ValidationJob(ApacheFactoryContext validatorContext, Class[] groups) { super(); @@ -598,33 +610,32 @@ private static boolean matchesGroups(ConstraintD constraint, Set targe } public final Set> getResults() { - if (results.optional().isPresent()) { - return results.get(); + if (resultsView != null) { + return resultsView; } if (hasWork()) { final Frame baseFrame = computeBaseFrame(); Validate.validState(baseFrame != null, "%s computed null baseFrame", getClass().getName()); - final Consumer> sink = results.consumer(Set::add); - - baseFrame.process(groups.asStrategy(), sink); - if (results.optional().isPresent()) { - return Collections.unmodifiableSet(results.get()); + baseFrame.process(groups.asStrategy(), this::addResult); + if (results != null) { + return resultsView = Collections.unmodifiableSet(results); } } - return results.reset(Collections::emptySet).get(); + return resultsView = Collections.emptySet(); + } + + private void addResult(ConstraintViolation violation) { + if (results == null) { + results = new LinkedHashSet<>(); + } + results.add(violation); } @SuppressWarnings("unchecked") private BeanD getBeanDescriptor(Object bean) { - final Class beanClass = Validate.notNull(bean, "bean").getClass(); - final Map, Class> classCache = validatorContext.getFactory().getUnwrappedClassCache(); - Class unwrappedClass = classCache.get(beanClass); - if (unwrappedClass == null) { - unwrappedClass = Proxies.classFor(beanClass); - classCache.putIfAbsent(beanClass, unwrappedClass); - } - return (BeanD) validatorContext.getDescriptorManager().getBeanDescriptor(unwrappedClass); + return (BeanD) validatorContext.getDescriptorManager() + .getBeanDescriptorForRuntimeClass(Validate.notNull(bean, "bean").getClass()); } final ConstraintViolationImpl createViolation(String messageTemplate, ConstraintValidatorContextImpl context, @@ -658,8 +669,7 @@ protected Predicate noViolations(Consumer consumer) { } private int violationCount() { - final Optional>> maybeResults = results.optional(); - return maybeResults.isPresent() ? maybeResults.get().size() : 0; + return results == null ? 0 : results.size(); } private String interpolate(String messageTemplate, MessageInterpolator.Context context) { diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/valueextraction/ValueExtractors.java b/bval-jsr/src/main/java/org/apache/bval/jsr/valueextraction/ValueExtractors.java index d6522e4893..fe3a54b786 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/valueextraction/ValueExtractors.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/valueextraction/ValueExtractors.java @@ -321,6 +321,10 @@ public Optional findUnwrappingInfo(Class containerClass, valueUnwrapping == ValidateUnwrappedValue.UNWRAP ? unwrapForcedCache : unwrapDefaultCache; // computeIfAbsent does not cache a thrown exception, so the UNWRAP "not found" error below is // re-evaluated on each call for that (rare, misconfigured) case rather than being memoized. + final Optional cached = cache.get(containerClass); + if (cached != null) { + return cached; + } return cache.computeIfAbsent(containerClass, k -> computeUnwrappingInfo(k, valueUnwrapping)); }