Quarkus: Caffeine error at runtime when using native build

[aimanph23]

Describe the bug

"causedBy": {
--
  | "exception": {
  | "refId": 5,
  | "exceptionType": "java.lang.ClassNotFoundException",
  | "message": "com.github.benmanes.caffeine.cache.SSMSW",
  | "frames": [
  | {
  | "class": "com.oracle.svm.core.hub.ClassForNameSupport",
  | "method": "forName",
  | "line": 60
  | },
  | {
  | "class": "java.lang.ClassLoader",
  | "method": "loadClass",
  | "line": 160
  | },
  | {
  | "class": "com.github.benmanes.caffeine.cache.LocalCacheFactory",
  | "method": "newBoundedLocalCache",
  | "line": 95
  | },

Expected behavior
Should be working in Native build

Actual behavior
Only works on JVM build and local dev.
Build native is successful, but at runtime it triggers an error

To Reproduce
Steps to reproduce the behavior:

1. Error when a code is hit with caffeine implementation at runtime
2. No problem with JVM build and local dev runtime

Environment (please complete the following information):
Quarkus 1.5.2.Final
GraalVM 20.1.0

[geoand]

cc @gwenneg

[geoand]

Can you give some more information on how to reproduce the problem?

[aimanph23]

Hi @geoand,

The problem was fix just a while ago by using a configuration file to register class for reflection since this is a 3rd party jar (caffeine-2.8.4.jar) and we cannot use @RegisterForReflection

Add resourcesreflection-config.json

[
  {
    "name" : "com.github.benmanes.caffeine.cache.SSMSW",
    "allDeclaredConstructors" : true,
    "allPublicConstructors" : true,
    "allDeclaredMethods" : true,
    "allPublicMethods" : true,
    "allDeclaredFields" : true,
    "allPublicFields" : true
  }
]

And in resources\application.properties

quarkus.native.additional-build-args =-H:ReflectionConfigurationFiles=reflection-config.json

[geoand]

That's good to know.

Just one point: You can use @RegisterForReflection to register a third party class for reclection, see the targets method of the annotarion.

@gwenneg I assume we should be registering this class for reflection in the cache extension?

[aimanph23]

Thanks @geoand for that option

Btw these are the classes that was imported

import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;

[gwenneg]

@geoand: com.github.benmanes.caffeine.cache.SSMSW should probably be registered for reflection in CaffeineProcessor.

[ben-manes]

A similar issue was raised in https://github.com/ben-manes/caffeine/issues/434. I am not familiar with GraalVM, so my question is naive:

Should libraries provide a manifest of for reflection, e.g. reflection-config.json, and would that be automatically picked up by Graal? If so, we could code generate this resource file in addition to the classes. Of course that would also mean including all of the generated classes, rather than the minimal subset an application is using. We use reflection because explicit switch/new was expensive to class load.

[geoand]

@ben-manes the question is very good! I can answer it, but I am sure @Sanne has a lot more points to add, so I'll pass it on to him :)

[Sanne]

Hi @ben-manes !

That is certainly an option, but I would recommend to not include such reflection instructions directly within Caffeine - store them in a separate artifact at least, so that they are opt-in as several users would prefer to control their own options, and these are purely "additive" today - which implies that if you include such directives directly in the Caffeine jar users would be unable to remove the ones they don't want to have.

As an example of how you could do such a thing, for Hibernate ORM I've created a separate jar - this is meant to help any other user of Hibernate ORM + GraalVM:

• https://github.com/hibernate/hibernate-orm/tree/master/hibernate-graalvm

One important thing to bear in mind: when you register a type X for reflection, then the GraalVM compiler is forced to consider this type as "included", even though the application is actually never using this thing (the compiler will not be able to _proof_ this type as unused and will have to assume that there's possibilities for this code to be needed).

That is a sub-optimal choice, especially if there are multiple implementations for the same core contract: as you well know from JIT optimisations, having a megamorphic call sites will result in less efficient code - for a combination of many reasons. Since there is no JIT in the native images, and since reflection is confusing the compiler, you risk paying the cost associated with a megamorphic call all the times.
Code size is also impacted of course, although in the case of Caffeine I suppose there is only minimal risk of some specific caching algorithm to "drag in" a significant dependency tree. But then also bear in mind that additional code could have an impact on the efficiency of the code actually being used, so code size amplifies the efficiency problem.

In general in Quarkus we try to do much better than the static list that I have in hibernate-graalvm by leveraging the build-time analysis: we can infer what is going to be used by looking at the code which is being built, at its configuration, its annotations, or even possibly its bytecode, and decide based on that input how to "guide" the GraalVM native-image compilation step; for example automatically generating the minimal set of Reflections to enable.

That's what a Quarkus extension is for: to help making such optimisations by plugging into the analysis phase and outputting the optimisations that a specific library is needing.

Having said that, simply enabling registration of "SSMSW" to be constructed reflectively for all applications is a viable solution, although not the ideal solution. We could go for this as a stop-gap solution, but in the longer term I'd like to see either:

• either Caffeine won't need reflection for this, so that the GraalVM compiler can do its magic automatically
• or we identify a way to have the Quarkus extension automatically infer which components of Caffeine are being used

Do you think either of these could be feasible?

N.B. the Hibernate ORM extension combines some highly optimised choices with a "static list" of reflective restrations: as in some cases it's not worth it, or on a long term wishlist, and we're being pragmatic about it.

[ben-manes]

Thank you for the thoughtful explanation! The example and why it should optional, if even added, is very helpful.

We currently generate ~400 classes to minimize the per-configuration memory overhead by reducing the number of cache / entry fields. This dropped from ~700 classes in 2.6.1, which was ~750kb of compressed bytecode bloat (https://github.com/ben-manes/caffeine/issues/110). Traditional server-side apps don't care about larger jars with unloaded classes and that still seems preferable compared to having dozens of unused fields on a map entry, which wastes memory for large caches.

either Caffeine won't need reflection for this, so that the GraalVM compiler can do its magic automatically or we identify a way to have the Quarkus extension automatically infer which components of Caffeine are being used

Originally we generated a string switch statement whose block returned the specialized type. The factory classes were oddly slow to class load due to bytecode verification. Possibly moving to a numeric offset to a Class[] array might have helped, but switching to reflection was the most straightforward way to cut the fat and prior to AOT becoming popular.

The generated classes are a private implementation detail, so a risk is that we'll generate a new naming pattern. For example an ask is to have pluggable key/value equivalence (Infinispan wanted this for byte[] keys, others for weak key interners). Casual development of this feature hints that it will replace entry variants for soft/weak references with factory parameters, which might reduce the number of entry classes with a few additional cache-level fields. That would be a semver minor release (new feature), but you might consider a major release due to breaking your static class list.

I'm not sure what the optimal strategy is:

1. Is codegen of unloaded classes still preferable to heavy weight objects? (I believe so)
2. Would reverting back to a static class listing defeat Graal's optimization, since the selection is still a runtime choice? (I believe so)
3. Does changing the names of internal classes cause downstream pain for Graal users? (I believe so)

For now I've been happy enough ignoring the problem and not burning cycles on it, given this is a weekend hobby project after all. If we can devise something that works better then I'd be happy to make some changes to improve developer QOL.

[Sanne]

Interesting puzzle, I'm not sure what to recommend as this is a new context; we'll probably need to do some experiments.

[ @galderz since you're now a GraalVM expert with both Infinispan and Caffeine experience, this might be of interest to you too? ]

I'll try to answer your points but I'm not sure, I hope to be able to do some experiments this summer to give you a better answer.

Is codegen of unloaded classes still preferable to heavy weight objects? (I believe so)

I believe you're right for the regular JIT, as the memory layout isn't optimised at runtime. Interestingly, the dead code elimination and constant folding of GraalVM in native image might be able to do a better job, as I believe it doesn't have to respect the same memory layout of the regular JVM, e.g. it could remove unused/constant fields.

Would reverting back to a static class listing defeat Graal's optimization, since the selection is still a runtime choice?

This could actually work, but only if the function mapping from the user's code to the actual implementation code is simple enough to analyse for the compiler to not have to give up. It would also need to be able to infer that the input of such a choice is a constant (or a subset of all possible options). I'm not sure what could qualify for being "simple enough" though, this is where I think it would be cool to do some experiments.

Does changing the names of internal classes cause downstream pain for Graal users?

It would if we had to rely on reflection, or obscure rules mapping to reflection. In the case of Quarkus this is ok though: we had to accept that such rules and optimisations can break even just because of internal changes: such compiler optimisations require coupling to internals so we have to accept that any micro-upgrade of a component might need to revisit such constants.

So if you have to change the naming structure occasionally that's not worrying me - we'll just adapt. Of course it would be more worrying if your classes were random-generated at each release, I hope that's not the plan :)

_suggestion_ :
what if Caffeine had an API we could invoke to "warmup" the kinds of caches that the application is going to need we make available?

Essentially a framework like Quarkus could benefit from being able to invoke something similar to the LocalCacheFactory, but which allows us to split the cache initialization in two phases, like in pseudo code:

 //to run during static initialization of a Quarkus application
 CacheFactory cf = LocalCacheCodePreparer.forconfigurations( Set<Caffeine> builders );

 //to run at runtime:
 LocalCache actualCache = cf.newBoundedLocalCache( Caffeine, cacheloader, async );

And this last method would throw an exception if and when the passed parameters don't match any of the configurations that have been passed to the first method.

N.B. we can transfer the cf instance with all the state it contains from what is generated in a static initialization block which we trigger during the build of the application into the actual produced binary. So if the CacheFactory had a map with methodhandles pointing to the constructors of each generated class it needs to be able to construct, the compiler would be able to convert such state.

I realize this is very rough; in particular I haven't thought about how the API should look like to use such a CacheFactory - would be great if this could be passed into the Caffeine instance so to set an alternative internal cache-builder strategy.

[ben-manes]

what if Caffeine had an API we could invoke to "warmup" the kinds of caches that the application is going to need we make available?

That's an interesting idea and seems like the most viable approach, as we won't rely on compiler magic working (and then breaking due to a seemingly innocent change). We'd need to iterate on the API to deemphasize so as to minimize conceptual weight for normal users who shouldn't need to learn about it. A top-level class sounds important, so I'd be curious if we could sneak it into something else.

Your idea has characteristics of CaffeineSpec which lacks configuring instance types, like a RemovalListener, that mostly don't matter for our specialized constructions. That's for simple parsing of an externalized configuration and used as Caffeine.from(spec)...build(loader), but custom formats (hocon, yaml) might just rebuild this utility as they see fit. Assuming they didn't, we could have it preload the class so that AOT's PGO observed it for runtime usage, as you said.

CaffeineSpec as-is might be too limiting, requiring that we either evolve it or add some pre-init method to the Caffeine builder to offer a more flexible configuration. However that does beg the question of whether modifying an externalized cache configuration could cause the app to fail because it didn't observe the right classes during static initialization? In your cache options, that might be when switching from expireAfterAccess to expireAfterWrite due to a misunderstanding of those settings without rebuilding.

[bnazare]

@geoand: com.github.benmanes.caffeine.cache.SSMSW should probably be registered for reflection in CaffeineProcessor.

That would probably work but only for @aimanph23's specific scenario. I'm facing a similar issue but using a simpler cache configuration. In my case I had to add two different classes to "reflect-config.json", like so:

{
  "name":"com.github.benmanes.caffeine.cache.SSW",
  "allDeclaredConstructors":true
},
{
  "name":"com.github.benmanes.caffeine.cache.PSW",
  "allDeclaredConstructors":true
}

Not only did I have to configure reflection by hand but I had to figure out the actual classes by trial and error. Both are things that I was not expecting to have to do when using an official Quarkus extension.

Btw, here's my cache configuration in case it's useful:

quarkus.cache.caffeine."my-cache".initial-capacity=100
quarkus.cache.caffeine."my-cache".expire-after-write=10M

[geoand]

All the more reason for the extension to be doing this work

[gaetancollaud]

My workaround so far is to use the @RegisterForReflection annotation like so:

@RegisterForReflection(
    classNames = {
      "com.github.benmanes.caffeine.cache.SSMSW",
      "com.github.benmanes.caffeine.cache.PSWMW",
      // add more here if you need to
    },

I've quickly looked at the CaffeineProcessor.java and I also looked a bit at what Caffeine generates.

Would it be a solution to lookup (via reflection) for all classes that extends com.github.benmanes.caffeine.cache.BoundedLocalCache ? Because it seems that all generated classes are children of BoundedLocalCache. @ben-manes can you confirm ?

If this would be a viable solution I don't mind taking a moment to make a PR to improve the CaffeineProcessor.

Edit: Hum, while doing a POC of searching for all the classes that extend BoundedLocalCache it seems like there is almost 400 classes. I'm not sure if we should add them all... So maybe my solution proposition is not that good. Let me know

[ben-manes]

@gaetancollaud The generated classes implement either BoundedLocalCache or Node (the cache entry).

[gsmet]

Yeah, I just got hit by this issue too. We need to fix it.

I would say we probably need to index Caffeine in Jandex and then get the implementations.

@ben-manes I suppose there's no way to get a simple list of class names?

[ben-manes]

See attachment for the list of generated classes. If there is a manifest format that can be included in the jar then we could add that, too.

classes.txt

[gsmet]

Yeah that's a ton of classes.

Having them in a service file or something could help but I'm wondering if
there would be a way to determine which would be useful depending on the
features we are using?

On Thu, Oct 8, 2020 at 8:00 PM Ben Manes [e-mail] wrote:

See attachment for the list of generated classes. If there is a manifest
format that can be included in the jar then we could add that, too.

classes.txt
https://github.com/quarkusio/quarkus/files/5350479/classes.txt

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/quarkusio/quarkus/issues/10420#issuecomment-705731790,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAJYOBNU3F2RECXSPSMOXTDSJX43BANCNFSM4OOR3F5A
.

[ben-manes]

atm the features are listed in the javadoc, e.g. PSAWRMS

/**
 * <em>WARNING: GENERATED CODE</em>
 *
 * <p>A cache entry that provides the following features:
 *
 * <ul>
 *   <li>MaximumSize
 *   <li>StrongKeys (inherited)
 *   <li>StrongValues (inherited)
 *   <li>ExpireAccess (inherited)
 *   <li>ExpireWrite (inherited)
 *   <li>RefreshWrite (inherited)
 * </ul>
 *
 * @author [email protected] (Ben Manes)
 */
@SuppressWarnings({"unchecked", "PMD.UnusedFormalParameter", "MissingOverride", "NullAway"})
final class PSAWRMS<K, V> extends PSAWR<K, V> {

[gaetancollaud]

Instead of having a manifest or looking for children of abstract class, what about having an annotation that we could lookup ?

Exactly like the @RegisterForReflection of quarkus.

This way we don't tamper too much with Caffeine for this. Each generated class should just have this annotation and we could then look it up by reflection.

[gsmet]

Having an annotation will require to index the jar anyway. So it makes no difference from a Quarkus POV. Once it's indexed, you can ask implementations to the index.

[ben-manes]

Also, fyi, if you don't index the entire jar then you could lose the ability to change the configuration externally without deploying a new build. Quoting from above,

However that does beg the question of whether modifying an externalized cache configuration could cause the app to fail because it didn't observe the right classes during static initialization? In your cache options, that might be when switching from expireAfterAccess to expireAfterWrite due to a misunderstanding of those settings without rebuilding.

A feature-only build would be more brittle in this respect. That might not be too bad in any modern CI pipeline, and might be the expectation anyway given other library interactions that could have similar challenges in an AOT world.

[Sanne]

Personally I'd prefer the API we discussed in previous comments - but I understand that's more work, and having it all "just work" is a good first step.

So let's index Caffeine to extract all implementations and register them; a little improvement could be for Caffeine to include the Jandex index within its jar: would you mind @ben-manes ?

Jandex supports build time indexing; I've never done this from a Gradle project but it seems there's a plugin: https://github.com/kordamp/jandex-gradle-plugin

Indexes are very compact, but also you can filter its content to only include what you need. Alternatively the manifest idea you suggested could also work, we just need to load the list and produce the matching reflection rules.

All in all the manifest idea might be simpler, but including a trimmed jandex index would speedup things for all the additional other cases in which a full classpath indexing is triggered (and not only by Quarkus).

[ben-manes]

I'm fine with adding a Jandex and open to iterating on API improvements to be AOT friendly.

The plugin has been added and a new snapshot is being built by CI. That takes ~1hr (millions of tests). If someone more knowledge than I am could verify this works as desired, or offer to include a sample project to include in my test suite (via /examples/, I'd certainly appreciate it.

[gsmet]

@ben-manes is there somewhere I can get the snapshots?

[ben-manes]

@gsmet The CI publishes to Sonatype's snapshot repository and any ad hoc commit via jitpack should work too.

[gsmet]

I will test things on Quarkus.

As for adding a unit test in Caffeine, a good example is: https://github.com/wildfly/jandex#loading-a-persisted-index .

Except you would look for index.getAllKnownSubclasses(DotName.createSimple("com.github.benmanes.caffeine.cache.BoundedLocalCache")) instead.

[gsmet]

@ben-manes I confirm it's all good from my side with your snapshot.

Draft PR is here: https://github.com/quarkusio/quarkus/pull/12621 .

[ben-manes]

@gsmet Thanks, added that test case. Let me know if you have any timeline for when you'd like a new release.

[gsmet]

@ben-manes The earlier, the better. I'm releasing 1.9.0.Final core artifacts on Wednesday morning early Paris time so having a release by then would be great so that this can be finally fixed. Thanks!

[ben-manes]

Sounds good, I'll release over the weekend (as late here, PST).

[gsmet]

Cool, thanks for the quick turnaround.

[ben-manes]

Released 2.8.6