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 native-image and LocalCacheFactory/NodeFactory #552

Closed
carterkozak opened this issue May 20, 2021 · 12 comments
Closed

GraalVM native-image and LocalCacheFactory/NodeFactory #552

carterkozak opened this issue May 20, 2021 · 12 comments

Comments

@carterkozak
Copy link

Hi, I'm sorry if this has been discussed elsewhere, but I've recently run into some foot-guns using native-images with caffeine caches.

I've been able to work around the reflection issues by adding reflection metadata for constructors of all implementations of LocalCacheFactory and NodeFactory, however that adds a lot of data to my image (5-10 mb). I see a similar issue on quarkus here, where only a subset of configurations are supported:
https://github.com/quarkusio/quarkus/blob/2cbd177416f92b9f45abb34b6f01acb8cb0f4eb5/extensions/caffeine/deployment/src/main/java/io/quarkus/caffeine/deployment/CaffeineProcessor.java#L19-L33

Would it be reasonable to generate references to constructors inline instead of using Class.forName and varhandles? The factory method would contain more bytecode to enumerate constructors, but I wouldn't expect it to be terribly large, equivalent to a few hundred lines of java code. I can put together a proof of concept if that's helpful.

Thanks!

carterkozak added a commit to carterkozak/caffeine that referenced this issue May 20, 2021
carterkozak added a commit to carterkozak/caffeine that referenced this issue May 20, 2021
@ben-manes
Copy link
Owner

Your change is similar to what we had a long time ago. It does cause a lot more bytecode and slower classloading, if I recall correctly. See this commit and the issue linked to it.

The Quarkus team suggested a factory like CaffeineSpec to warm in AOT, but that description was too vague for me to understand how to follow up on it. I'd be open to doing something, but it's unclear what that should be.

They previous had asked me to add a jandex index for them to leverage, but then that brought in all classes and resulted in a larger binary. I'd be afraid that your change might return to that.

@carterkozak
Copy link
Author

Thanks for the info, I can try a few implementations to gauge the impact on jar size as well as native-image size.

The Quarkus team suggested a factory like CaffeineSpec to warm in AOT, but that description was too vague for me to understand how to follow up on it. I'd be open to doing something, but it's unclear what that should be.

I think the quarkus design gives them a lot more control up front than I have in my target framework, so I'm not confident that approach would be helpful for me.

They previous had asked me to add a jandex index for them to leverage, but then that brought in all classes and resulted in a larger binary. I'd be afraid that your change might return to that.

I think jandex allows quarkus to quickly locate all implementations of a given interface/class, for example all LocalCacheFactory and NodeFactory implementations, so they could include constructor metadata. If the constructors are called directly without reflection, the AOT compiler should be able to eliminate implementations that aren't called by the program, however I can take some time in the next day or so to build a few sample native projects to verify this is correct -- it's possible the stringbuilder -> string switch implementation isn't easy enough to optimize, it's possible a bitset of features may allow better elimination.

Given that native-image is becoming more common, the original jar-size tradeoff might make sense to re-evaluate in order to avoid potential runtime failures.

@ben-manes
Copy link
Owner

ben-manes commented May 21, 2021

I believe a lot of the bytecode bloat that caused a long class load were the Strings themselves, since they go into the constant pool. A bitmapped integer might solve that problem.

@carterkozak
Copy link
Author

In testing, it doesn't look like I can avoid including unused types entirely from the native image, however the image size is smaller using direct constructor references as opposed to all reflective constructor data.
The 14mb estimate from the linked issue isn't something I can reproduce, I have to imagine that has something to do with possible interactions between other reflection which can take dynamic input, and caffeine caches? It's difficult to tell without a reproducer though. In my testing, the quarkus minimal reflection list results in a 14mb binary, and a bitmapped integer results in a 15mb binary. Still a 20k jar size difference between the current reflective implementation and a bit-mapped integer.

@ben-manes
Copy link
Owner

hmm.. then I'm not sure what we can or should do? Maybe @Sanne or @gsmet can give us hints on how to avoid including the unused types in native image?

@carterkozak
Copy link
Author

A potential idea is to code-gen the cache builder using a staged builder structure. A thought experiment at this point, not sure it's at all feasible to implement.

Something like this:

// exploded form rather than fluent to better describe individual stages:
CaffeineKeyStrengthStage stage0 = Caffeine.builder();
CaffeineValueStrengthStageWithStrongKeys stage1 = stage0.strongKeys();
Cache<?, ?> cache = stage1.build();

This way by the time build() is called, we've eliminated paths in a way that's obvious to the type system. It may not be perfect if specific values are applied which can be simplified out at runtime to produce a simpler implementation, but I'd expect the vast majority of implementations could be eliminated (unless a caffeine cachespec is used, in that case all implementations must be loaded).
Unfortunately it would be a serious public api break, and result in more total classes generated, which increases the jar size.

@ben-manes
Copy link
Owner

Technically it is possible if Caffeine.class is made a abstract as the constructor is private. It would certainly look messy.

@janknobloch
Copy link

have you considered using the java-lib agent approach of graal (running in a jvm) generating a reflection.json containing all actual needed classes ?

In order to make preparing these configuration files easier and more convenient, GraalVM provides an agent that tracks all usages of dynamic features of an execution on a regular Java VM.

https://www.graalvm.org/reference-manual/native-image/BuildConfiguration/

I used a native image configuring the following classes using a "ReflectionRegistry" by registering a

@AutomaticFeature
public class ReflectionRegistry implements Feature {}

And registering the following package "com.github.benmanes.caffeine.cache", which is what you're trying to avoid - i guess.

@carterkozak
Copy link
Author

At the moment I'm using such an AutomaticFeature in my projects to provide all constructor metadata for implementations of LocalCacheFactory and NodeFactory. The agent approach isn't ideal since I can't be confident that all configurations are tested, and at the end of the day I'm okay with an extra couple megabytes in my distribution package if it removes the risk of runtime failure.

@janknobloch
Copy link

Agree, just wanted to mention it as I wasn't sure you aware of it and you where initially arguing about the 5-10mb which seemed to be critical for you.

@ben-manes
Copy link
Owner

I think that I need the Quarkus team (@Sanne, @gsmet) to get involved in these discussions because this is outside of my expertise.

I don't want to reintroduce the non-reflective factory if that causes AOT frustrations, but also want to reduce the AOT pains of reflectively loaded classes. In a traditional JIT'ed world, the current api and implementations make sense and are more optimal for user experience and runtime performance. AOT changes the base design assumptions for the JVM, but is still rare enough that most of us ecosystem developers lack experience with it. For me to move forward requires having multiple stakeholders involved that can agree on the right path and help on the code changes.

@ben-manes
Copy link
Owner

Closing since there is no clarity on what, if anything, should be done on my side. I am fine making changes if we can minimize regressions for everyone involved. Please feel welcome to reopen or create a new issue if more direction can be provided.

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