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

GraalVM compatibility #138

Open
jameskleeh opened this issue Apr 19, 2020 · 4 comments
Open

GraalVM compatibility #138

jameskleeh opened this issue Apr 19, 2020 · 4 comments

Comments

@jameskleeh
Copy link
Contributor

Currently generated source classes for rocker templates include something like

        static {
            PlainTextUnloadedClassLoader loader = PlainTextUnloadedClassLoader.tryLoad(gradleProperties.class.getClassLoader(), gradleProperties.class.getName() + "$PlainText", "UTF-8");
            PLAIN_TEXT_0_0 = loader.tryGet("PLAIN_TEXT_0_0");
            PLAIN_TEXT_1_0 = loader.tryGet("PLAIN_TEXT_1_0");
            PLAIN_TEXT_2_0 = loader.tryGet("PLAIN_TEXT_2_0");
        }

The PlainText class is available in the source code and thus dynamic classloading is not necessary. This breaks compatibility with GraalVM and requires a lot of manual work to get past it. Can you describe the reason for this behavior? Is there a flag or are you willing to support a flag to allow for direct class access?

@jjlauer
Copy link
Member

jjlauer commented Apr 20, 2020

First, I'll say I'm a fan of how the GraalVM is trying to move the ball forward on performance and compilation, so I like your question. What i'll write below is simply my research on the JVM, not idea how GraalVM may change the "constant" pool I mention below.

As you may or may not know, the JVM does not unload classes (and Strings they reference as part of their definition) as soon as you load them into the ClassLoader, until the ClassLoader used to load them is garbage collected. So unless you use a custom ClassLoader, you'll never have the ability to garbage collect the memory used for the Class itself.

I did a lot of early testing with why this is the case and if you're interested in reading more I have some notes here:

https://github.com/fizzed/rocker/blob/master/docs/README_CONSTANT_POOL.md

Templates are incredibly heavy with Strings (e.g. an HTML template) and those strings are all part of the class definitions for each template. Rocker's performance relies heavily on pre-converting strings into UTF-8 bytes for significant amounts of memory efficiency, speed, and other performance improvements. The class that holds these Strings is temporarily loaded, then unreferenced, and the JVM can get garbage collect the loaded class itself and remove the Strings from the "constant" pool. Its simply a way of easily bootstrapping the bytes required by using a 1-time use ClassLoader to load the plain text.

That being said, Rocker was designed from the start to support other strategies for loading the bytes it needs. There already is a 2nd strategy one can configure on Rocker to use simple static strings vs. static strings via an unload class.

https://github.com/fizzed/rocker/blob/master/rocker-compiler/src/main/java/com/fizzed/rocker/compiler/PlainTextStrategy.java

Perhaps that could work or you could look at other strategies that help target the GraalVM.

@jameskleeh
Copy link
Contributor Author

@jjlauer I appreciate the reply. After creating the issue I did find that option, however unfortunately there is no way to set it from the gradle plugin. I ended up copying the gradle plugin into my projects buildSrc and modifying it to achieve that result. The strings staying in memory is not a problem for my use case, so the tradeoff for GraalVM compatibility is worth it.

@jjlauer
Copy link
Member

jjlauer commented Apr 20, 2020

Cool, glad you saw that. If you were interested in helping build a simple PR to expose that config property in the Gradle and Maven plugins, possibly others may find that useful like you have. If you have any notes you wanted to share in the main README on using Rocker in GraalVM, that would be welcomed as well!

@ppedregal
Copy link

PR to fix #180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants