Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance degradation in SPEL expression evaluation [SPR-16942] #21481

Open
spring-issuemaster opened this issue Jun 14, 2018 · 5 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Jun 14, 2018

Kanthi Vaidya opened SPR-16942 and commented

We are planning to move from Spring 4.3.6 to Spring 5. We noticed a considerable dip in performance. About 50% degradation.

#21421 talks about a specific hot spot. Please look at attached benchmarks in that JIRA.

 

 

Juergen, I saw your code changes for caching the SortedMethods. We don't benefit much if you don't make your ConcurrentHashMap of sorted methods  a static variable.

Even after making the ConcurrentHashMap a static variable, we still don't get back the original performance of the prior spring versions. It definitely helps of course.

This is just one hotspot. More benchmarking and profiling is needed to identify other hot spots. And it probably makes sense to add a test case to the CI infrastructure to catch any such degradations.

This performance degradation, is preventing us from migration to Spring 5.

 


Affects: 5.0.7

Issue Links:

  • #21130 Support for SimpleEvaluationContext in SpEL
  • #21421 ReflectivePropertyAccessor should cache sorted methods

0 votes, 5 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 21, 2018

Christoph Dreis commented

Hi, I noticed this as well recently. The caching solution in #21421 helps only in situations were the ReflectivePropertyAccessor and/or EvaluationContext is not created over and over again. Spring Security`s WebExpressionVoter or more specifically AbstractSecurityExpressionHandler is creating a StandardEvaluationContext with a fresh ReflectivePropertyAccessor every time for example and thus not benefitting much of the cache.

I therefore wonder if we should additionally look at the call-sites and enhance them if possible - similar to what the framework does in some of the annotation relevant code in order to prevent the costly checks in AnnotationUtils or AnnotatedElementUtils.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 31, 2018

Juergen Hoeller commented

Christoph Dreis, it is definitely recommended to reuse EvaluationContext instances as far as possible, preferring the use of Expression.getValue with a root object argument over a context-bound root object if necessary. SimpleEvaluationContext (#21130) strongly suggests this usage model on a recent note. And for our internal use of StandardEvaluationContext, we follow the same guideline: see e.g. the expression context caching in StandardBeanExpressionResolver.

So please report any such obvious cases to the corresponding projects if they are not aware of it already.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 31, 2018

Christoph Dreis commented

I figured already - hence the comment. See it as an addendum to the work that might be done in the Framework itself, which I think is still necessary. But I believe the projects that make use of it are able to optimize the calls themselves as well. I'll let you know once I created the issue(s) in the respective projects.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 1, 2018

@hradilf

This comment has been minimized.

Copy link

@hradilf hradilf commented Jun 21, 2019

Hi @spring-issuemaster,
please, is there any estimation when this issue will be solved?
Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.