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

Add support for org.ehcache:ehcache:3.10.8-jakarta #124

Merged
merged 9 commits into from
Jan 11, 2023

Conversation

linghengqian
Copy link
Contributor

@linghengqian linghengqian commented Nov 24, 2022

What does this PR do?

Checklist before merging

  • I have properly formatted metadata files (see CONTRIBUTING document)
  • I have added thorough tests. (see this)

Comment on lines 14 to 19
String libraryVersion = TestUtils.testedLibraryVersion.split("-")[0]

dependencies {
testImplementation("org.ehcache:ehcache:$libraryVersion") {
capabilities {
requireCapability('org.ehcache:ehcache-jakarta')
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testImplementation 'ch.qos.logback:logback-classic:1.4.5'
testImplementation 'ch.qos.logback:logback-core:1.4.5'
}

Copy link
Contributor Author

@linghengqian linghengqian Nov 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Now execute ./gradlew clean test -Pcoordinates=org.ehcache:ehcache:3.10.8-jakarta, we will see a Warning Log. Do I need to do something or ignore it? Refer to Java 11 support ehcache/ehcache3#2671 .
04:12:09.769 [main] WARN org.ehcache.sizeof.ObjectGraphWalker - The JVM is preventing Ehcache from accessing the subgraph beneath 'private final byte[] java.lang.String.value' - cache sizes may be underestimated as a result
java.lang.reflect.InaccessibleObjectException: Unable to make field private final byte[] java.lang.String.value accessible: module java.base does not "opens java.lang" to unnamed module @3e7dfd44
        at java.base@17.0.5/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
        at java.base@17.0.5/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
        at java.base@17.0.5/java.lang.reflect.Field.checkCanSetAccessible(Field.java:178)
        at java.base@17.0.5/java.lang.reflect.Field.setAccessible(Field.java:172)
        at org.ehcache.sizeof.ObjectGraphWalker.getAllFields(ObjectGraphWalker.java:245)
        at org.ehcache.sizeof.ObjectGraphWalker.getFilteredFields(ObjectGraphWalker.java:204)
        at org.ehcache.sizeof.ObjectGraphWalker.walk(ObjectGraphWalker.java:159)
        at org.ehcache.sizeof.SizeOf.deepSizeOf(SizeOf.java:70)
        at org.ehcache.impl.internal.sizeof.DefaultSizeOfEngine.sizeof(DefaultSizeOfEngine.java:51)
        at org.ehcache.impl.internal.store.heap.OnHeapStore.getSizeOfKeyValuePairs(OnHeapStore.java:981)
        at org.ehcache.impl.internal.store.heap.OnHeapStore.makeValue(OnHeapStore.java:1529)
        at org.ehcache.impl.internal.store.heap.OnHeapStore.makeValue(OnHeapStore.java:1515)
        at org.ehcache.impl.internal.store.heap.OnHeapStore.newCreateValueHolder(OnHeapStore.java:1464)
        at org.ehcache.impl.internal.store.heap.OnHeapStore.lambda$put$8(OnHeapStore.java:344)
        at org.ehcache.impl.internal.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1934)
        at org.ehcache.impl.internal.store.heap.SimpleBackend.compute(SimpleBackend.java:98)

@wilkinsona
Copy link
Contributor

Many of the entries in reflect-config.json are for classes that aren't part of EhCache. For example, there are numerous entries for classes beneath jakarta.xml.bind. Unless EhCache is doing something with JAX-B that is unique, I don't think such entries belong in its reachability metadata. We've seen previously (#108 and #113 for example) the problems that can be caused by things being too broad and I think this may have similar problems.

@linghengqian
Copy link
Contributor Author

linghengqian commented Nov 25, 2022

Many of the entries in reflect-config.json are for classes that aren't part of EhCache. For example, there are numerous entries for classes beneath jakarta.xml.bind. Unless EhCache is doing something with JAX-B that is unique, I don't think such entries belong in its reachability metadata. We've seen previously (#108 and #113 for example) the problems that can be caused by things being too broad and I think this may have similar problems.

  • @wilkinsona I have removed the GraalVM reachability metadata related to javax.management.**. But unfortunately, whether it is proxy-config.json or reflect-config.json, once I remove the entry related to jakarta.xml.bind.**, ./gradlew clean test -Pcoordinates=org.ehcache:ehcache:3.10.8-jakarta will fail. Ehcache is really using JAXB to parse xml files.

@wilkinsona
Copy link
Contributor

Ehcache is really using JAXB to parse xml files.

I believe anyone using JAXB may have this problem. As such, I am not sure that it's appropriate for EhCache to provide the reachability metadata that makes it work. It may be that there should be some reachability metadata for JAXB itself. Put another way, depending on Ehcache shouldn't magically make JAXB start working in other places.

@linghengqian
Copy link
Contributor Author

Ehcache is really using JAXB to parse xml files.

I believe anyone using JAXB may have this problem. As such, I am not sure that it's appropriate for EhCache to provide the reachability metadata that makes it work. It may be that there should be some reachability metadata for JAXB itself. Put another way, depending on Ehcache shouldn't magically make JAXB start working in other places.

  • I guess you want me to provide GraalVM reachability metadata for org.glassfish.jaxb:jaxb-runtime:3.0.2, but I honestly don't know how to write as many units as possible for this infrastructure for testing, I've never dealt with JAXB directly. 🧐

@wilkinsona
Copy link
Contributor

I suspect that someone should do that, but it doesn't have to be you of course. To proceed here, I think we need to understand how Ehcache is using JAXB. That would give us the information that's needed to determine where the metadata should go. Until that's been done, I think adding it under Ehcache would be the wrong thing to do.

@linghengqian
Copy link
Contributor Author

linghengqian commented Nov 25, 2022

I suspect that someone should do that, but it doesn't have to be you of course. To proceed here, I think we need to understand how Ehcache is using JAXB. That would give us the information that's needed to determine where the metadata should go. Until that's been done, I think adding it under Ehcache would be the wrong thing to do.

  • I would first generate GraalVM metadata for org.glassfish.jaxb:jaxb-runtime:3.0.2 and hang it in test/resources/META-INF/native-image/org.glassfish.jaxb/jaxb-runtime directory. This will take some time.

@linghengqian
Copy link
Contributor Author

linghengqian commented Nov 26, 2022

  • I have resolved the whole issue of this PR. I have temporarily hosted parts of GraalVM Reachability metadata with jakarta.xml.bind:jakarta.xml.bind-api:3.0.1 and org.glassfish.jaxb:jaxb-runtime:3.0.2 in the test/resources folder.
  • It just looks like GraalVM Trace Agent is not strong enough in Conditional Mode and needs me to handle it manually.
  • image

@linghengqian
Copy link
Contributor Author

@dnestoro I'm assuming I need to resolve merge conflicts first, right?

@dnestoro
Copy link
Member

@dnestoro I'm assuming I need to resolve merge conflicts first, right?

Yes. Sorry, I forgot to ask for that

# Conflicts:
#	metadata/index.json
#	tests/src/index.json
{
"name": "com.sun.org.apache.xerces.internal.impl.msg.SAXMessages",
"locales": [
"und",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please elaborate on why only those locales are needed?
I am also wondering if that belongs in ehcache hints or not.

@dnestoro @gradinac Do we support conditions on bundles?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This is simply generated by Conditional Agent Mode. Do I need to remove it?

@sdeleuze
Copy link
Collaborator

I will test it tomorrow with https://github.com/spring-projects/spring-aot-smoke-tests.

sdeleuze added a commit to sdeleuze/spring-aot-smoke-tests that referenced this pull request Dec 16, 2022
@sdeleuze
Copy link
Collaborator

@linghengqian I have tested with spring-projects/spring-aot-smoke-tests#160 and had the following error:

Caused by: java.lang.IllegalArgumentException: The serializer: org.ehcache.impl.serialization.PlainJavaSerializer does not have a constructor that takes in a ClassLoader.
	at org.ehcache.impl.config.serializer.DefaultSerializationProviderConfiguration.addSerializerFor(DefaultSerializationProviderConfiguration.java:99) ~[cache-ehcache:3.10.8]
	at org.ehcache.impl.config.serializer.DefaultSerializationProviderConfiguration.addSerializerFor(DefaultSerializationProviderConfiguration.java:75) ~[cache-ehcache:3.10.8]
	at org.ehcache.jsr107.EhcacheCachingProvider.createCacheManager(EhcacheCachingProvider.java:148) ~[cache-ehcache:3.10.8]
	at org.ehcache.jsr107.EhcacheCachingProvider.getCacheManager(EhcacheCachingProvider.java:134) ~[cache-ehcache:3.10.8]
	at org.ehcache.jsr107.EhcacheCachingProvider.getCacheManager(EhcacheCachingProvider.java:85) ~[cache-ehcache:3.10.8]

Could you please add the constructor hints for serializer + related tests and test with the sample from the PR I linked above:

  • By customizing reachabilityMetadataUrl property in gradle.properties, for example on my computer I used reachabilityMetadataUrl=file:///home/seb/workspace/graalvm-reachability-metadata/metadata
  • Running ./gradlew clean framework:cache-ehcache:nativeAppTest

@linghengqian
Copy link
Contributor Author

@sdeleuze

  • Because I'm on a business trip these days, the reply is late. To be honest, I don't quite understand the operating mechanism of https://github.com/spring-projects/spring-aot-smoke-tests, so I added some related tests in the new commit based on my guess, and it passed New tests pull more relevant metadata.
{
     "condition": {
       "typeReachable": "org.ehcache.impl.config.serializer.DefaultSerializationProviderConfiguration"
     },
     "name": "org.ehcache.impl.serialization.PlainJavaSerializer",
     "methods": [
       {
         "name": "<init>",
         "parameterTypes": [
           "java.lang.ClassLoader"
         ]
       }
     ]
   }

@sdeleuze
Copy link
Collaborator

sdeleuze commented Jan 2, 2023

Ok I will test again to check how it goes now.

@sdeleuze
Copy link
Collaborator

sdeleuze commented Jan 4, 2023

Works as expected on Spring side.

@dnestoro You can merge this PR.

@vjovanov vjovanov merged commit 378bcfe into oracle:master Jan 11, 2023
@linghengqian linghengqian deleted the ehcache-jakarta branch January 11, 2023 08:34
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

Successfully merging this pull request may close these issues.

Add support for org.ehcache:ehcache-jakarta
5 participants