- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 966
 
First Step in optimizing GSP Performance on Grails 7. Safe small optimizations to start #15109
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
Conversation
…mizations to start
        
          
                grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPageWritable.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      …s, think the GrailsWebUnit helper has a behavior that is atypical
| 
           Isn't gsp currently exclusive commons logging? Not sure if adding org.slf4j dependencies is a good idea if that is the case. Spring Framework is 100% commons logging  | 
    
…s to the GroovyPage, ignore cache if using that
| 
           This isn't made for spring . It's made for grails  | 
    
          
 @davydotcom GSP should be independent of Spring and Grails. What is being gained by switching out the logger and requiring another dependency in the classpath?  | 
    
| 
           I've reformatted some of the files using IntelliJ to fix the codeStyle errors. It seems some of this reformat caused parts of the file that were unchanged to be formatted. This is probably ok, but trying to give a heads up for other users.  | 
    
          
 According to Spring, slf4j is a supported logging platform: https://docs.spring.io/spring-framework/reference/core/spring-jcl.html  | 
    
| class GroovyPageCompiler { | ||
| 
               | 
          ||
| private static final Log LOG = LogFactory.getLog(GroovyPageCompiler) | ||
| private static final Logger LOG = LoggerFactory.getLogger(GroovyPageCompiler) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just @Slf4j('LOG') and you get log statements wrapped in if(log.isDebugenabled()) "for free"
| gsptarget.flush() | ||
| // write static html parts to data file (read from classpath at runtime) | ||
| File htmlDataFile = new File(new File(targetDir, packageDir), className + GroovyPageMetaInfo.HTML_DATA_POSTFIX) | ||
| File htmlDataFile = new File(new File(targetDir, packageDir), className + GroovyPageMetaInfo.HTML_DATA_POSTFIX) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems redundant to create two new File(targetDir, packageDir) here and in line 231
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call out, I have a change local to fix this but given closeness to release its part of some larger changes. can likely wait
          
 @jdaugherty Of course it is. It is bundled with the Spring Boot starter, but why force another dependency on gsp which should be independent of both grails and spring? what value is being added by switching it other than personal preference?  | 
    
| 
           I would like to add, that this all looks good and very well thought through.  | 
    
| 
           Worth mentioning, this does not break existing GSP compiles. we left the old method names such that the old compiled GSP Files would still function.  | 
    
I have been asked to take a look at Grails GSP Performance. This is an older codebase and I was able to identify some low hanging fruit to start this journey that is pretty safe for release.
There are multiple parts that need to be considered with GSP performance. The differences in cold start performance in development mode as well as production mode rendering.
Cold start involves compiler overhead. This means any changes in groovy4 compiler performance @paulk-asert can have a significant impact in this area. Fortunately, from what I can tell, there does not seem to be a huge difference in performance here in my testing.
I am aware that the compiler (since groovy 3) runs slower on LARGE files. In an effort to reduce file size, I have shorthanded
printHtmlParts(Integer)toh(Integer)when the gsp is first being converted to agroovyfile. This should help with just scanning and size for the compilation process.I have also started swapping out commons logging with
SLF4J. Why you may ask? There are several spots where there are wrapped if statements ifdebugEnabled. This is no longer necessary onSLF4Jand simply reads better.I also have started adding javadoc headers to some of the GSP classes so it is easier to follow what is going on in the code. As this is not the easiest to read through.
Big Optimizations
Whenever a page or partial is rendered, a new instance of
GroovyPageis created. This is never cached even though 99% of the class is thread safe (except for the OutputStack). We have made changes in the already cachedGroovyPageMetaInfoclass that now instantiates aThreadLocalcache of the page. In addition, theConstructorpointer is also cached for concurrent access to reduce reflection overhead. There is an excessive amount of operations that are performed before a page is rendered in theGroovyPage#initRun(). I have moved some of this out intoinitCommonRun()that only needs to be run per instance, as opposed to per execution of the pagesrun(). This can be taken farther by a lot but did not want to create high risk issues to start. The other big move was to create thehtmlPartsSetin the meta info class instead of during construction of each groovy page instance.Making this change shows approximately 30% in overhead reduction.
Test Scenarios
First Scenario: Render 10,000 nested template renders in a
<g:each>of a row:Render Time after a few warmups BEFORE Changes: 200ms avg
Render Time after a few warmups AFTER Changes: 150ms avg
Second Scenario: Render 10,000 nested template renders using
<g:render collection=''>Render Time after a few warmups BEFORE Changes: 63ms avg
Render Time after a few warmups AFTER Changes: 40ms avg
Third Scenario: Render LARGE HTML Page (lots of html parts)
Render Time after a few warmups BEFORE Changes: 2ms avg
Render Time after a few warmups AFTER Changes: 1ms avg
These Tests were run on an M3 Max Macbook Pro . I would imagine even more gains on different hardware.
What's next.
We need to aim to further reduce the instantiation logic within
GroovyPageit's a short lived object that needs to be super quick to instantiate. There are common singleton reference classes that are associated at the start likeTagLibraryLookupthat should be removed.We need to shorten this pipeline overall. There are a lot more spots I can see that need optimized and will continue to dig farther. The TagLibraryLookup could also stand a rethink, but that is for another day.
I would appreciate any thoughts on this PR and look forward to hearing if this improves overall performance for others.