hk2166 commented on issue #16558: URL: https://github.com/apache/grails-core/issues/16558#issuecomment-6448603374
Thanks @jdaugherty, that makes sense. I dropped the static cache idea and looked at what GORM already keeps per entity. The constraints are already evaluated once per entity and kept. `mappingContext.getEntityValidator(entity)` creates a `PersistentEntityValidator` the first time, the validator registry holds it, and it holds the evaluated `constrainedProperties`. `DomainClass.getConstrainedProperties()` already reads them from there. Binding a domain instance doesn't use them today. The two hot spots in the report line up with: 1. `DataBindingUtils.getConstrainedProperties(Object)`: the `constraintsMap` and `getConstraintsMap()` lookups throw, and then `ValidationSupport.getConstrainedPropertiesForClass` evaluates the `constraints` closure again on every bind. 2. `BindingIncludeLists.bindableConstraintValue`: `ValidationSupport` wraps each property in a `ConstrainedDelegate`, which has neither `getMetaConstraintValue` nor `metaConstraints`. Two exceptions are thrown and caught for each property before it reaches the wrapped one. Revised proposal, with no new cache: - When the bound object's class (resolved through the proxy handler, as `bindingType` already does) is a persistent entity in `grailsApplication.mappingContext`, read the constraints from that entity's validator instead of evaluating the closure. The memory stays with the mapping context, is bounded by the number of domain classes, and goes away with the application context. - Check for the `constraintsMap` and `getConstraintsMap()` accessors with `hasProperty` / `respondsTo` instead of catching exceptions. The lookup order stays the same, so instance-level constraints still take precedence. - Read `bindable` without exceptions, unwrapping a `ConstrainedDelegate` first. - Keep computing the unbindable names on each bind from those constraints. I'd measure this with the reporter's benchmark first, and only suggest storing the result if it still shows up. Tests would cover `bindable: false` on domain instances, proxies and subclasses, that the constraints come from the entity rather than being evaluated again, and that command objects and `Validateable` classes behave as before. On overlap: #16058 changes `SimpleDataBinder` and `GrailsWebDataBinder` for index validation, so it doesn't touch this code. @borinquenkid's #16602 and #16683 convert `DataBindingUtils` and `BindingIncludeLists` to Groovy on side branches, so this change would conflict with them. I can target 8.0.x now or wait and apply it on top of the conversion, whichever is easier. @jamesfredley does this fit the intent of #15947? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
