Skip to content

Avoid exceptions when evaluating validation hints#26787

Closed
dreis2211 wants to merge 4 commits intospring-projects:masterfrom
dreis2211:performance-validation-hints-exceptions
Closed

Avoid exceptions when evaluating validation hints#26787
dreis2211 wants to merge 4 commits intospring-projects:masterfrom
dreis2211:performance-validation-hints-exceptions

Conversation

@dreis2211
Copy link
Copy Markdown
Contributor

@dreis2211 dreis2211 commented Apr 11, 2021

Hi,

we noticed in one of our loadtests that a substantial amount of "hidden" NoSuchMethodExceptions is being thrown when using @javax.validation.Valid on controller methods.

@PostMapping("test")
public void test(@RequestBody @Valid @NotNull TestRequest request) {
     // Some code
}

This is caused by calling AnnotationUtils.getValue() on this annotation, which has no such property. This PR bypasses the annotation utility call. I also took the freedom to refactor this logic into a common place in ValidationUtils to avoid copy-paste throughout the different modules.

An isolated JMH benchmark shows the following results when dealing with @Valid:

Benchmark                                         Mode  Cnt     Score     Error   Units
MyBenchmark.testNew                               avgt   10     6,253 ±   0,312   ns/op
MyBenchmark.testOld                               avgt   10  1761,995 ±  92,262   ns/op

As a workaround you can use org.springframework.validation.annotation.Validated of course, but I would prefer not to have such a drawback when using javax.validation.Valid.

Let me know what you think.

Cheers,
Christoph

@dreis2211
Avoid exceptions when evaluating validation hints
56eadac 
Prior to this commit, evaluating validation hints for @javax.validation.Valid
caused exceptions being raised when getting the value of this annotation, which
does not exist. Bypassing AnnotationUtils.getValue() in those cases can improve
performance by avoiding the cost incurred by raising exceptions.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 11, 2021
dreis2211
dreis2211 commented Apr 13, 2021
View reviewed changes
Comment thread spring-context/src/main/java/org/springframework/validation/ValidationUtils.java Outdated
* @param ann the annotation (potentially a validation annotation)
* @return the validation hints to apply (possibly an empty array),
* or {@code null} if this annotation does not trigger any validation
* @since 5.3.6
Copy link
Copy Markdown
Contributor Author

@dreis2211 dreis2211 Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be 5.3.7 now - let me know if I should add that.

@jhoeller jhoeller added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 21, 2021
@jhoeller jhoeller added this to the 5.3.7 milestone Apr 21, 2021
@jhoeller
Copy link
Copy Markdown
Contributor

jhoeller commented Apr 21, 2021 •
edited
Loading

Hey @dreis2211 it's indeed worth handling the common javax.annotation.Valid case explicitly there, and also worth extracting the evaluation logic. However, it'll have to be a new org.springframework.validation.annotation.ValidationAnnotationUtils class in order to avoid a package cycle, since it refers to the Validated annotation in the subpackage there. If you could refactor it that way with a since marker for 5.3.7, we'd be happy to roll it in...

@dreis2211
Copy link
Copy Markdown
Contributor Author

dreis2211 commented Apr 21, 2021 •
edited
Loading

@jhoeller Thanks for the review. I just committed the requested changes.

snicoll
snicoll approved these changes Apr 21, 2021
View reviewed changes
Comment thread spring-context/src/main/java/org/springframework/validation/ValidationUtils.java Outdated
@snicoll snicoll self-assigned this Apr 22, 2021
@snicoll
Copy link
Copy Markdown
Member

snicoll commented Apr 22, 2021

@dreis2211 I can see ModelAttributeMethodProcessorTests is failing. Can you please have a look?

@dreis2211
Copy link
Copy Markdown
Contributor Author

dreis2211 commented Apr 22, 2021

@snicoll Should be fixed - I was a bit overly ambitious and wanted to optimize the custom @Valid annotations case as well, which broke when the annotation was defined as an inner class.

snicoll pushed a commit that referenced this pull request Apr 22, 2021
@dreis2211 @snicoll
Avoid exceptions when evaluating validation hints
e7cbe23 
Prior to this commit, evaluating validation hints for
@javax.validation.Valid caused exceptions being raised when getting the
value of this annotation, which does not exist. Bypassing
AnnotationUtils.getValue() in those cases can improve performance by
avoiding the cost incurred by raising exceptions.

See gh-26787
@snicoll snicoll closed this in e4c0ff5 Apr 22, 2021
@snicoll
Copy link
Copy Markdown
Member

snicoll commented Apr 22, 2021

Thanks for the follow-up Christoph.

@sync-by-unito sync-by-unito Bot mentioned this pull request May 12, 2021
Merged
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this pull request Mar 26, 2022
@dreis2211 @lxbzmy
Avoid exceptions when evaluating validation hints
d7410fd 
Prior to this commit, evaluating validation hints for
@javax.validation.Valid caused exceptions being raised when getting the
value of this annotation, which does not exist. Bypassing
AnnotationUtils.getValue() in those cases can improve performance by
avoiding the cost incurred by raising exceptions.

See spring-projectsgh-26787
@jhoeller jhoeller mentioned this pull request Aug 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@snicoll snicoll snicoll approved these changes

Assignees

@snicoll snicoll

Labels

in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement

Projects

None yet

Milestone

5.3.7

Development

Successfully merging this pull request may close these issues.

4 participants

@dreis2211 @jhoeller @snicoll @spring-projects-issues