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

Switch to Apache EL implementation by default #24744

Closed
thomasschuerger opened this issue Jan 11, 2021 · 15 comments
Closed

Switch to Apache EL implementation by default #24744

thomasschuerger opened this issue Jan 11, 2021 · 15 comments
Assignees
Labels
status: noteworthy A noteworthy issue to call out in the release notes type: enhancement A general enhancement
Milestone

Comments

@thomasschuerger
Copy link

thomasschuerger commented Jan 11, 2021

We have noticed that JSP EL expression evaluation has become significantly slower when non-existing attributes are accessed, starting with Spring Boot 2.3.0. This problem seems to affect all current versions of Spring Boot 2.3.x (2.3.0 to 2.3.7) and 2.4.x (2.4.0 and 2.4.1), but not 2.2.x.

Starting with Spring Boot 2.3.0, the following happens: For each non-existing attribute accessed during Spring EL expression evaluation on a JSP (for example, when using ${myProperty} or <c:if test="${myProperty}">...</c:if> and no such attribute is defined in the view model), there are class loader calls to find the classes "java.lang.<attribute>", "java.servlet.<attribute>", "java.servlet.http.<attribute>" and "java.servlet.jsp.<attribute>".

This happens once per unique attribute on each request (or per JSP/tag context); accessing the same attribute again in the same context does not lead to additional class lookups. This significantly slows down the EL expression evaluation, since the 4 lookups for each attribute take a few milliseconds on an idle machine. When properties that do exist are accessed, there is no performance difference between the different Spring Boot versions. At least in our HTML frontend it is common that we only set model attributes in certain cases, and the JSPs/tags then check whether they are set, so accesses to non-existing attributes happen very frequently, which leads to noticeable performance problems.

Root cause seems to be that a different ExpressionFactory is used (com.sun.el.ExpressionFactoryImpl in 2.3.0+ instead of org.apache.el.ExpressionFactoryImpl in 2.2.x). The latter sets a certain context attribute to true (see below), but the former does not.

The ScopedAttributeELResolver contains this code:

   boolean resolveClass = true;
    // Performance short-cut available when running on Tomcat
    if (AST_IDENTIFIER_KEY != null) {
        // Tomcat will set this key to Boolean.TRUE if the
        // identifier is a stand-alone identifier (i.e.
        // identifier) rather than part of an AstValue (i.e.
        // identifier.something). Imports do not need to be
        // checked if this is a stand-alone identifier
        Boolean value = (Boolean) context.getContext(AST_IDENTIFIER_KEY);
        if (value != null && value.booleanValue()) {
            resolveClass = false;
        }
    }
    // This might be the name of an imported class
    ImportHandler importHandler = context.getImportHandler();
    if (importHandler != null) {
        Class<?> clazz = null;
        if (resolveClass) {
            clazz = importHandler.resolveClass(key);
        }
      ... 

AST_IDENTIFIER_KEY is the class org.apache.el.parser.AstIdentifier.

In Spring Boot 2.2.x, resolveClass is set to false since the context boolean is true, in 2.3.0 and beyond it stays at true. The performance problem occurs when importHandler.resolveClass(key) is called, which does the four class lookups.

The class that should normally set this attribute is org.apache.el.parser.AstIdentifier (which is not used in Spring Boot 2.3.0+, there com.sun.el.parser.AstIdentifier is used), using this code:

  /* Putting the Boolean into the ELContext is part of a performance
   * optimisation for ScopedAttributeELResolver. When looking up "foo",
   * the resolver can't differentiate between ${ foo } and ${ foo.bar }.
   * This is important because the expensive class lookup only needs to
   * be performed in the later case. This flag tells the resolver if the
   * lookup can be skipped.
   */
  if (parent instanceof AstValue) {
      ctx.putContext(this.getClass(), Boolean.FALSE);
  } else {
      ctx.putContext(this.getClass(), Boolean.TRUE);
  }

The class loading becomes visible when setting logging.level.org.apache.catalina.loader=DEBUG

In Spring Boot 2.3.0 and beyond, the following is logged for each request when accessing ${myProperty}:

o.a.c.loader.WebappClassLoaderBase       :     findClass(java.lang.myProperty)
o.a.c.loader.WebappClassLoaderBase       :     --> Returning ClassNotFoundException
o.a.c.loader.WebappClassLoaderBase       :     findClass(javax.servlet.myProperty)
o.a.c.loader.WebappClassLoaderBase       :     --> Returning ClassNotFoundException
o.a.c.loader.WebappClassLoaderBase       :     findClass(javax.servlet.http.myProperty)
o.a.c.loader.WebappClassLoaderBase       :     --> Returning ClassNotFoundException
o.a.c.loader.WebappClassLoaderBase       :     findClass(javax.servlet.jsp.myProperty)
o.a.c.loader.WebappClassLoaderBase       :     --> Returning ClassNotFoundException

This is easily reproducible in a minimalistic Spring Boot application. Locally, with classloader logging disabled, I get around 4000 requests per second when hammering a simple JSP that accesses 30 different non-existing model attributes in Spring Boot 2.2.12. In all later Spring Boot versions, I get around 33 requests per second only.

You can find the test application here: http://www.schuerger.com/spring-boot/test-app.tar.gz

Run it with

mvn clean compile spring-boot:run

Then use ApacheBench to URL-hammer it:

ab -n 800 -c 8 http://localhost:8080/

Afterwards, edit pom.xml to switch to version 2.2.12.RELEASE, repeat and compare. Disable the class loading logging for more realistic results.

Compare this to when changing the TestController to set all of the 30 attributes to a dummy value instead of leaving them undefined. Afterwards, the performance between the Spring Boot versions is similar.

Note that this is most likely not a Tomcat issue. I have tested this locally with different Tomcat versions (9.0.34, 9.0.35 and 9.0.41) with different combinations of Spring Boot. Note that Spring Boot 2.2.7 by default uses version 9.0.34, 2.2.12 uses 9.0.41 and 2.3.0 uses 9.0.35. The problem occurs in 2.3.x and 2.4.x regardless of the Tomcat version used and does not occur in 2.2.x regardless of the Tomcat version used.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 11, 2021
@wilkinsona
Copy link
Member

wilkinsona commented Jan 12, 2021

Thanks very much for the detailed analysis @TranceTip. In 2.3.0 we made a change meant that we use the same EL implementation (the RI) with all the embedded containers that we support (#19550). This is why the expression factory has changed.

It may be that we can configure the RI to behave as the Apache EL implementation did, we'll need to look into that possibility. In the meantime, you should be able to exclude org.glassfish:jakarta.el and add a dependency on org.apache.tomcat.embed:tomcat-embed-el instead to restore the behaviour you were seeing in 2.2.x. Could you please give that a try and let us know how it goes?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jan 12, 2021
@thomasschuerger
Copy link
Author

thomasschuerger commented Jan 12, 2021

Confirmed, when adding

                   <exclusions>
                            <exclusion>
                                    <groupId>org.glassfish</groupId>
                                    <artifactId>jakarta.el</artifactId>
                            </exclusion>
                   </exclusions>

to the spring-boot-starter-tomcat dependency, I get the same performance as before. Thanks for that!

Note, however, that org.apache.tomcat.embed:tomcat-embed-el is already included by the org.apache.tomcat.embed:tomcat-embed-jasper dependency, regardless of whether jakarta.el is excluded or not. Are tomcat-embed-el and jakarta.el supposed/allowed to co-exist or are they mutually-exclusive?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 12, 2021
@thomasschuerger
Copy link
Author

thomasschuerger commented Jan 12, 2021

Both tomcat-embed-el and jakarta.el bring their own different version of javax.el.ExpressionFactory. That doesn't sound like they should be allowed to co-exist. The jakarta version always returns com.sun.el.ExpressionFactoryImpl for newInstance(), the Tomcat version has a way more complicated way to determine the ExpressionFactory implementation to use.

@wilkinsona
Copy link
Member

Thanks for trying the Apache EL implementation.

The EL spec requires that javax.el.ExpressionFactory load an implementation using the following algorithm:

  • Use the Services API (as detailed in the JAR specification). If a resource with the name of META-INF/services/javax.el.ExpressionFactory exists, then its first line, if present, is used as the UTF-8 encoded name of the implementation class.
  • Use the properties file "lib/el.properties" in the JRE directory. If this file exists and it is readable by the java.util.Properties.load(InputStream) method, and it contains an entry whose key is "javax.el.ExpressionFactory", then the value of that entry is used as the name of the implementation class.
  • Use the javax.el.ExpressionFactory system property. If a system property with this name is defined, then its value is used as the name of the implementation class.
  • Use a platform default implementation.

In this case, with two implementations on the classpath (each of which has a META-INF/services/javax.el.ExpressionFactory file) the one that is first on the classpath will win. Generally speaking, to avoid classpath ordering determining which implementation will be used, it is recommend that only one implementation is present. In your case, excluding the Glassfish reference implementation is the right thing to do as you prefer the performance characteristics of the Apache implementation.

@thomasschuerger
Copy link
Author

thomasschuerger commented Jan 12, 2021

Thanks, I will then go with the exclusion for now.

In any case, the dependency conflict regarding javax.el.ExpressionFactory needs to be fixed in Spring Boot. Having both EL implementations included when spring-boot-starter-tomcat and org.apache.tomcat.embed:tomcat-embed-jasper are used without explicit exclusions screams for problems to occur.

@wilkinsona
Copy link
Member

See https://bz.apache.org/bugzilla/show_bug.cgi?id=57583 for some background on the performance optimisation made in Tomcat's EL implementation.

Having both EL implementations included when spring-boot-starter-tomcat and org.apache.tomcat.embed:tomcat-embed-jasper are used without explicit exclusions screams for problems to occur

We can consider excluding tomcat-embed-el from tomcat-embed-jasper in our dependency management, but that runs the risk of breaking anyone who is not using our starters. I'll flag this so that the rest of the team can express an opinion. Generally speaking, when you add a third-party dependency directly rather than via a Spring Boot starter you become responsible for ensuring that there are not duplicate classes on the classpath. As you're using Maven, you may want to consider using the duplicate finder plugin to help with this.

@wilkinsona
Copy link
Member

wilkinsona commented Jan 12, 2021

There are a couple of things to consider for this issue:

  1. Should we switch to the Apache EL implementation across the board rather than the RI as it appears to be faster
  2. Should we put exclusions in place in our dependency management to reduce the chance of two EL implementations being on the classpath

The need for 2 may change depending on what we decide to do for 1.

@wilkinsona wilkinsona added for: team-attention An issue we'd like other members of the team to review and removed status: feedback-provided Feedback has been provided labels Jan 12, 2021
@thomasschuerger
Copy link
Author

thomasschuerger commented Jan 12, 2021

FYI: In our web application on a test environment, the landing page now has a throughput which is 6 times faster than before with the exclusion in place, so we have our previous 2.2.x performance back.

@philwebb philwebb changed the title Performance problem during EL expression evaluation for non-existing attributes Switch to Apache EL implementation by default Jan 13, 2021
@philwebb philwebb added type: enhancement A general enhancement status: noteworthy A noteworthy issue to call out in the release notes and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Jan 13, 2021
@philwebb philwebb added this to the 2.5.x milestone Jan 13, 2021
@wilkinsona wilkinsona self-assigned this Jan 15, 2021
@wilkinsona wilkinsona modified the milestones: 2.5.x, 2.5.0-M1 Jan 15, 2021
@thomasschuerger
Copy link
Author

Will this fix be backported to 2.3.x and 2.4.x as well?

@snicoll
Copy link
Member

snicoll commented Jan 15, 2021

@TranceTip this is flagged as an enhancement so, no it won't.

@maciejtoporowicz
Copy link

My team had exactly the same issue after we migrated from spring boot 1.5.x to 2.3.6 and looks like the exclusion of jakarta.el helped us too. Thanks a lot for the investigation and for the solution!

I was googling for "spring boot jsp render slow" (and similar) quite often this month, but this thread never popped up. Writing this so that hopefully more people are able to find it.

@vivekvara
Copy link

We had also faced the same problem and the exclusion of jakarta.el helped us too.
For a long-term solution, we are decided to go by defining each variable by adding respective scopes(as suggested by tomcat 8 migration release).

This leads us to more improved performance and moves away from any specific implementation.

@drahkrub
Copy link

drahkrub commented Feb 24, 2021

Ran into this problem yesterday and spent hours debugging. The slowdown in a (large) packaged spring boot app (with jsps utilizing jstl) is huge because the jars inside the war file are scanned when the class loader searches for the classes java.lang.<attribute>, ... as described - if glassfish EL is used.

For a long-term solution, we are decided to go by defining each variable by adding respective scopes(as suggested by tomcat 8 migration release).

At first glance I didn't understand this statement, but here's the long version: https://tomcat.apache.org/migration-8.html#JavaServer_Pages_2.3

@thomasschuerger
Copy link
Author

This fix is in Spring Boot 2.5.0, right? Shouldn't it be listed on https://github.com/spring-projects/spring-boot/releases?

@bclozel
Copy link
Member

bclozel commented Jun 18, 2021

@TranceTip it is listed in the 2.5.0-M1 release as well as the official release notes.

agrancaric added a commit to croz-ltd/nrich that referenced this issue Mar 25, 2022
…of org.glassfish:jakarta.el to be inline with spring (see spring-projects/spring-boot#24744 for details)
agrancaric added a commit to croz-ltd/nrich that referenced this issue Mar 28, 2022
…of org.glassfish:jakarta.el to be inline with spring (see spring-projects/spring-boot#24744 for details)
agrancaric added a commit to croz-ltd/nrich that referenced this issue Apr 1, 2022
…of org.glassfish:jakarta.el to be inline with spring (see spring-projects/spring-boot#24744 for details)
agrancaric added a commit to croz-ltd/nrich that referenced this issue Apr 6, 2022
…of org.glassfish:jakarta.el to be inline with spring (see spring-projects/spring-boot#24744 for details)
jzrilic pushed a commit to croz-ltd/nrich that referenced this issue Apr 20, 2022
…of org.glassfish:jakarta.el to be inline with spring (see spring-projects/spring-boot#24744 for details)
matrei added a commit to matrei/grails-forge that referenced this issue Feb 26, 2024
There is no need for this dependency.
An el-impl is already provided by Spring Boot/tomcat-embed-el.
tomcat-embed-el is also used for other servlet containers.

Related: spring-projects/spring-boot#24744
matrei added a commit to matrei/grails-forge that referenced this issue Mar 11, 2024
There is no need for this dependency.
An el-impl is already provided by Spring Boot/tomcat-embed-el.
tomcat-embed-el is also used for other servlet containers.

Related: spring-projects/spring-boot#24744
matrei added a commit to matrei/grails-forge that referenced this issue Mar 11, 2024
There is no need for this dependency.
An el-impl is already provided by Spring Boot/tomcat-embed-el.
tomcat-embed-el is also used for other servlet containers.

Related: spring-projects/spring-boot#24744
puneetbehl pushed a commit to grails/grails-forge that referenced this issue Mar 14, 2024
There is no need for this dependency.
An el-impl is already provided by Spring Boot/tomcat-embed-el.
tomcat-embed-el is also used for other servlet containers.

Related: spring-projects/spring-boot#24744
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: noteworthy A noteworthy issue to call out in the release notes type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

9 participants