hk2166 opened a new pull request, #16781:
URL: https://github.com/apache/grails-core/pull/16781
## Description
Fixes the 8.0.0 data binding slowdown reported in #16558. On 8.0.x every
bind onto a domain instance (`new Book(map)`, `bindData`, `obj.properties =
params`) built its list of `bindable: false` properties from scratch:
1. `DataBindingUtils.getConstrainedProperties` asked the instance for
`constraintsMap` and `getConstraintsMap()`. A domain class has neither, so it
threw and caught a missing property exception and a missing method exception.
It then evaluated the class's `constraints` closure again through
`ValidationSupport`.
2. `BindingIncludeLists.bindableConstraintValue` then called
`getMetaConstraintValue` and read `metaConstraints` on each constrained
property. `ValidationSupport` and `Validateable` wrap each property in a
`ConstrainedDelegate`, which has neither, so it threw and caught two more
exceptions for every property before it reached the wrapped one. Command
objects pay this part too, which answers the question about commands on the
issue.
### Changes
There is no new cache:
- **Domain instances and proxies:** for an instance, or a proxy, of a mapped
entity, binding reads the constraints from the entity's validator
(`mappingContext.getEntityValidator(entity)`). The mapping context evaluates
them once, when it creates the validator, and keeps them for as long as it
holds the entity. With GORM for Hibernate or MongoDB,
`grailsApplication.mappingContext` is the datastore's mapping context, so this
is the validator `validate()` uses. Proxies are resolved through the mapping
context's proxy handler, as `bindingType` already does.
- **Binder lookup:** `GrailsWebDataBinder` passes its own
`GrailsApplication`, so the lookup doesn't resolve the application again on
every bind.
- **No exceptions:** the accessors are looked up through the metaclass
(`hasProperty`, `getMetaMethod`, `getStaticMetaMethod`) before they are called,
so a missing one no longer throws. The lookup order is unchanged.
- **Everything else:** an object that isn't a mapped entity, or whose
validator doesn't expose its constraints, takes the existing path.
### Before and after
These are µs per call. Each figure is the median of the last 3 of 5 rounds;
each round ran 5,000 warm-up calls, then timed 20,000 calls.
| | 8.0.x | This PR |
|---|---:|---:|
| Domain: `new Book(a: 1.0, b: 1.0, c: 1.0, d: 1.0)` | 76.1 | 12.4 |
| Domain: `DataBindingUtils.bindObjectToInstance` | 82.6 | 13.2 |
| Command (`Validateable`): `DataBindingUtils.bindObjectToInstance` | 32.2 |
14.3 |
| Domain: `new Book()` + 4 setters (no binding, for reference) | 2.8 | 3.2 |
How these were measured:
- A temporary Spock spec in `grails-test-suite-persistence`, run with
`DataTest` (the in-memory datastore, not Hibernate), on JDK 21 on an Apple
Silicon Mac. The spec is not part of this PR.
- The domain class and the command each have 24 constrained properties, 2 of
them `bindable: false`, and the bind sets 4 `BigDecimal` properties.
- The "after" times include the entity lookup that was a concern on the
issue: a proxy check, a lookup of the entity by class name and a lookup of its
validator.
- The absolute numbers are lower than in the report, which was measured in a
`bootRun` app with Hibernate. The improvement from 8.0.x is the point of the
comparison.
- The report also mentions
`GrailsWebDataBinder.getNestedBindingIncludeList`, which runs for each bound
property. This PR doesn't change it.
### Tests
The tests use the public binding APIs: the binder, `DataBindingUtils`, the
map constructor and `bindData`.
- `GrailsWebDataBinderSpec` (`grails-test-suite-persistence`):
- A domain class whose `constraints` closure counts how often it runs is
bound through the binder, `DataBindingUtils` and the map constructor. The
`bindable: false` property is not bound, and the closure does not run again. On
8.0.x this test fails, because the closure runs on every bind.
- A domain subclass doesn't bind the properties that it or its superclass
constrain `bindable: false`.
- `DomainProxyBindingSpec` (`grails-test-suite-uber`): a proxy of a domain
class doesn't bind a `bindable: false` property, through `bindData` or
`DataBindingUtils`.
- `DataBindingUtilsSpec` (`grails-web-databinding`):
- An instance of a mapped entity, and a proxy of one, are bound with the
constraints their validator holds. Both tests fail on 8.0.x.
- An entity whose validator doesn't expose its constraints falls back to
the constraints its class declares.
- A constrained property that wraps another is read through the one it
wraps.
Verification:
- All tests in `grails-web-databinding` (68),
`grails-test-suite-persistence` (112), `grails-test-suite-web` (444, 1 skipped)
and `grails-test-suite-uber` (626, 4 skipped) pass. The skips also occur on
8.0.x.
- `./gradlew cleanViolationReports aggregateViolations --continue` reports
no Checkstyle, CodeNarc, PMD or repository convention violations. SpotBugs is
disabled.
- I didn't run the repository-wide `clean` or `:grails-test-report:check`,
which would run every module's tests.
Fixes #16558
Generative AI tooling (Claude Code) was used to research the cause, write
the change and its tests, and run the benchmark. I reviewed the change.
## Contributor Checklist
Please review the following checklist before submitting your pull request.
Pull requests that do not meet these requirements may be closed without review.
### Issue and Scope
- [x] This PR is linked to an existing issue that has been **acknowledged or
approved** by the project team. If no approved issue exists, please give
background on why this change is necessary. Tickets are preferred for release
change log history.
- [x] This PR addresses the **complete scope** of the linked issue. Partial
implementations or unfinished work should not be submitted for review. *(The
constraints are no longer evaluated on each bind, which is what the issue
reports. The smaller cost the report attributes to
`getNestedBindingIncludeList` is unchanged.)*
- [x] This PR contains a **single, focused change**. Unrelated changes
should be submitted as separate pull requests.
- [x] This PR targets the **correct branch** for the type of change:
- **Patch release branches** (e.g., `7.0.x`): Bug fixes only. No new
features or API changes.
- **Minor release branches** (e.g., `7.1.x`): New features are welcome,
but breaking existing APIs must be avoided.
- **Major release branches** (e.g., `8.0.x`): Reserved for major
changes. Breaking API changes are permitted.
### Code Quality
- [x] I have **added or updated tests** that cover the changes introduced in
this PR. All code contributions are expected to include appropriate test
coverage.
- [ ] I have verified that all existing tests pass by running `./gradlew
build --rerun-tasks`. *(Not the full build; all tests in the four affected
modules pass, see above.)*
- [x] My code follows the project's **code style** guidelines. I have run
`./gradlew codeStyle` and resolved any violations. See [Code
Style](../CONTRIBUTING.md#code-style) for details.
- [x] This PR does **not** include mass reformatting, style-only changes, or
large-scale refactoring unless it was **explicitly approved** in the linked
issue. Unsolicited reformatting will not be accepted.
- [x] If generative AI tooling was used in preparing this contribution, a
quality model was used to ensure contributions are **consistent with the
project's quality standards**.
### Licensing and Attribution
- [x] All contributed code is provided under the [Apache License
2.0](https://www.apache.org/licenses/LICENSE-2.0), and new source files include
the appropriate **Apache license header**.
- [ ] I have the necessary rights to submit this contribution and confirm it
is my own original work (see [Legal
Notice](../CONTRIBUTING.md#i-want-to-contribute)).
- [x] If generative AI tooling was used in preparing this contribution, I
have followed the [Apache Software Foundation's policy on generative
tooling](https://www.apache.org/legal/generative-tooling.html) and have
properly attributed its use.
### Documentation
- [x] If this PR introduces user-facing changes, I have included or updated
the relevant documentation. *(No user-facing change: binding behaves as before,
only faster.)*
- [x] If this PR adds a new feature, I have updated the **What's New**
section of the Grails Guide.
- [x] If this PR introduces breaking changes or changes that require user
action during an upgrade, I have updated the **Upgrade Notes** for the
corresponding version in the Grails Guide.
- [x] The PR description clearly explains **what** was changed and **why**.
---
> **First-time contributors:** Please read our [Contributing
Guide](../CONTRIBUTING.md) before submitting.
> Pull requests that appear to be auto-generated, incomplete, or unrelated
to an approved issue may be
> closed to help maintainers focus on reviewed and planned work. We
appreciate your understanding.
--
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]
