JAVA-3045 Fix GraalVM native image support for GraalVM 22.2 #1612
Conversation
|
I tested a Java driver built with these changes against a sample native image built using GraalVM 22.1 and 22.2. The 22.2 version failed with the errors indicated in JAVA-3045; these changes corrected those issues and allowed for the creation of a functioning native image. GraalVM 22.1 was fine without these changes (which was consistent with our earlier testing). That version also built a working native image with these changes so backwards compatibility appears to not be an issue. |
…endency to resolve at build-time
|
The more I thought about this the more something just didn't sit right. I don't love the idea of forcing dependency to be a build-time initialization via CLI params; that feels like a symptom that something else is off. So I did some testing and I'm pretty sure I broke this with my fixes for JAVA-2950. It appears that the introduction of shaded Guava classes in Dependency wound up messing with its initialization logic in 22.2. I've confirmed that the current code cleanly builds a native image (and that that native image does what you'd expect it to) against both 22.1 and 22.2. @sdeleuze can you confirm that this addresses the problem in your test case? |
|
|
||
| Dependency(String... classNames) { | ||
| clzs = ImmutableList.copyOf(classNames); | ||
| clzs = Collections.unmodifiableList(Arrays.asList(classNames)); |
There was a problem hiding this comment.
A proxy for immutable lists without the type information. Probably not strictly necessary (there's no obvious way for these lists to change) but it provides information that these values shouldn't change.
| @@ -15,7 +15,9 @@ | |||
| */ | |||
| package com.datastax.oss.driver.internal.core.util; | |||
|
|
|||
| import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableList; | |||
There was a problem hiding this comment.
Shouldn't we add a comment somewhere to warn future maintainers that this class should be extra careful when importing anything outside of the standard library?
There was a problem hiding this comment.
So... I'm a little torn on this, frankly.
I'd feel better about saying something like this if I understood more cleanly why the change in question resulted in the behaviour we saw. But it seems to be the case that introducing a dependency upon a shaded class somehow:
- caused Graal to eval this class as a runtime class
- only did so for Graal 22.2; the original code has always worked fine on 22.1
I... just don't know if I have enough there to extract some kind of meaningful guidance. We've already seen that changes between Graal versions can have odd impacts on what was perfectly valid code. This seems like another one of those, but I just don't see a way to draw a lesson from that which can be clearly stated and apply to future use.
I'm open to suggestions if anybody has any.
There was a problem hiding this comment.
Yeah pretty hard to anticipate this kind of behavior change, I would vote for no specific comment but up to you.
core/src/main/java/com/datastax/oss/driver/internal/core/protocol/CompressorSubstitutions.java
Show resolved
Hide resolved
|
@absurdfarce I confirms this refined PR that avoid to use |
absurdfarce
requested review from
sdeleuze and
adutra
and removed request for
sdeleuze
Native tests will still fail till apache/cassandra-java-driver#1612 is resolved, still causing java.lang.NoClassDefFoundError: net/jpountz/lz4/LZ4Compressor.
|
A quick note on this: I believe all the dev work is done here and this is ready to be merged. I haven't done so yet because we're making some infrastructure changes internally and I'd like to get those sorted out so that I can get a good clean Jenkins run of the code base with this change. I don't anticipate any problem with that; it's really just a matter of finishing the infrastructure changes, and unfortunately that's one of quite a few issues demanding my attention. Just wanted to keep you updated @sdeleuze |
|
The changes mentioned in my previous commit have been delayed somewhat so I'm going to merge this as it stands in order to avoid having approved PRs lingering. I don't expect there to be any fallout from these changes but if there is it should be easy to clean up. |
|
I was trying not to ask this question on this closed issue and looked around for the answer before posting this but was unable to find the answer... When will Thanks |
|
Hey @onobc , thanks for checking in! I'm hoping to have 4.16.0 released next week, ideally early next week. That can always change of course but at the moment it looks like we're on track to make that happen. |
|
Hi @absurdfarce , thanks for the reply. Are snapshots published anywhere currently? |
|
We don't have snapshots published anywhere @onobc , however I'm pleased to announce that 4.16.0 should now be available from Maven Central! |
|
Thanks for the release @absurdfarce I attempted to use the fix in (referenced in spring-projects/spring-data-cassandra#1340). |
|
I checked |
|
@sdeleuze Unless I'm mistaken (always possible, some might say likely!) this was discussed when we originally worked on the PR. At the time it looked like removing the Guava classes solved the problem. That was always "black magic" to a certain degree, though, so it's possible there's more going on than just that. I'll try and re-test against 4.16.0 locally in order to see if I can repro what you're seeing. |
|
I tried to repro this locally but haven't had any luck yet. I added the following code snippet to my local app to very explicitly reference the dependency ops in the original report: MutableCodecRegistry registry = new DefaultCodecRegistry("foo");
DseTypeCodecsRegistrar.registerDseCodecs(registry);I then tried a matrix of all four variations of [GraalVM 22.2.0, GraalVM 22.3.1] x [ESRI dependency present, ESRI dependency missing]. In all four cases I was able to build my test app without any complaints about build-time initialization of Dependency. Note that most of the above was redundant. The driver is already executing the relevant code when it performs it's initialization and any issue with the Dependency class should stem from the checking process (and thus would apply whether any one dependency is present or not). But I wanted to be sure. I also tried out the sample app included in the original report to spring-data-cassandra. This also led to something of a dead end. The user didn't specify versions for the various dependencies in his pom.xml (so I'm sure what I'm getting now is different from what he got then) but at the moment I can't build anything at all. It looks like one of the Spring deps has been bumped up to Java17 in the interim; if I try to build with GraalVM 22.2.0 or 22.3.1 + Java11 I get the following: If I try to bump my Java version up to Java17 I (quite unsurprisingly) get complaints from the Graal compiler: @onobc Do you have a more current self-contained repro case that might bring the issue to the surface? |
|
Also getting the same error using The simple example can be found in this test: https://github.com/msupic/datastax-java-driver-native/blob/master/src/test/java/org/example/JavaDriverTest.java I can reproduce the issue using the following graalvm version:
|
|
Can you please check if |
|
I just tried the example from @msupic locally and had no issue: Tests actually failed to run but that was due to an issue with testcontainers understanding the config syntax from my Docker instance. It's very clear that a functioning native image was built for me locally. All of that said, there is a critical difference between my setup and what @msupic reported: All of that said, I don't think it matters very much. It's very clear that Dependency has to be a build-time dependency; it's used in the logic that determines when certain substitutions are employed and that only makes sense at build-time. It's also pretty clear that at least some configurations (incorrectly) determine that Dependency should be a run-time dependency and error out accordingly. The logic governing how Graal determines whether something should be a build-time or run-time dependency are extremely opaque to me so I'm not sure there's a reliable way to make that logic behave. We tried that once and clearly it didn't work. So without quite a bit of additional information I'm skeptical that we can come up with a code-oriented solution that'll really fix this. There's one more additional data point of relevance here; there's no obvious downside to including the arg specifying Dependency as a build-time dependency. It shouldn't be necessary but clearly in some cases it is... and it doesn't hurt anything if we use it all the time. @hhughes and I discussed this earlier this morning and the unanimous verdict was to add the args to the properties file. I'll be creating a PR to do exactly that shortly. Apologies for all the confusion everyone; clearly there's more variation here than one might have expected. |
|
Note: @hhughes has opened JAVA-3085 to address this follow-up work. All further activity on this issue should migrate over to the relevant PR for that ticket. |
|
@sdeleuze Adding Dependency to the --initialize-at-build-time fixes the issue. |
This pull request contains content originally provided by @sdeleuze in PR 1611