Lazily recompute Inet4Address, Inet6Address and CidrAddress fields at runtime #5353

gwenneg · 2019-11-09T19:57:27Z

This PR is related to #4218

It can be merged into master before GraalVM 19.3.0 is available.

machi1990 · 2019-11-10T09:13:07Z

For info, PR #5336 contains the same fix and more I guess.

gwenneg · 2019-11-10T09:36:29Z

Thanks for pointing this out @machi1990, I'll take a look at #5336.

gwenneg · 2019-11-10T11:19:01Z

So, it seems @galderz started testing GraalVM 19.3.0 with JDK 11 and faced an issue that is being fixed in #5336.

This PR is only about applying patches suggested by @cstancu and see if the current Quarkus master build is OK with it, as mentioned in #4218. We've got the answer about the build since all checks passed.

Now, I don't know if this PR should be merged or closed as a duplicate of #5336.

gsmet · 2019-11-10T11:45:55Z

I think we should include all the changes required by @cstansu ASAP. But IIRC there were more suggestions than that in the original issue ?

gwenneg · 2019-11-10T12:25:59Z

@gsmet Yes, there are other suggestions, some are breaking changes (see #5358) and I still have to test others. I submitted this PR because of @cstancu's comment.

gwenneg · 2019-11-10T18:01:12Z

I added the command-line option suggested by @cstancu as it did not cause any issue locally. Let's see if CI is happy with it.

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

gwenneg · 2019-11-10T21:06:32Z

I added the command-line option suggested by @cstancu as it did not cause any issue locally. Let's see if CI is happy with it.

I may have to retract the no local issue part of my message, I'm actually dealing with a GraalVM error right now. I'm putting the coding-in-progress label back until it's fixed.

gwenneg · 2019-11-11T09:54:43Z

Here's the GraalVM error:

Error: Classes that should be initialized at run time got initialized during image building:
 org.wildfly.common.net.Inet the class was requested to be initialized at build time (from the command line). org.wildfly.common.net.CidrAddress caused initialization of this class with the following trace: 
org.wildfly.common.net.CidrAddress the class was requested to be initialized at build time (from the command line). org.wildfly.common.net.CidrAddress caused initialization of this class with the following trace: 
	at io.quarkus.runner.AutoFeature.beforeAnalysis(AutoFeature.zig:856)
	at com.oracle.svm.hosted.NativeImageGenerator.lambda$runPointsToAnalysis$7(NativeImageGenerator.java:670)
	at com.oracle.svm.hosted.FeatureHandler.forEachFeature(FeatureHandler.java:63)
	at com.oracle.svm.hosted.NativeImageGenerator.runPointsToAnalysis(NativeImageGenerator.java:670)
	at com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:526)
	at com.oracle.svm.hosted.NativeImageGenerator.lambda$run$0(NativeImageGenerator.java:444)
	at java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1386)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

com.oracle.svm.core.util.UserError$UserException: Classes that should be initialized at run time got initialized during image building:
 org.wildfly.common.net.Inet the class was requested to be initialized at build time (from the command line). org.wildfly.common.net.CidrAddress caused initialization of this class with the following trace: 
org.wildfly.common.net.CidrAddress the class was requested to be initialized at build time (from the command line). org.wildfly.common.net.CidrAddress caused initialization of this class with the following trace: 
	at io.quarkus.runner.AutoFeature.beforeAnalysis(AutoFeature.zig:856)
	at com.oracle.svm.hosted.NativeImageGenerator.lambda$runPointsToAnalysis$7(NativeImageGenerator.java:670)
	at com.oracle.svm.hosted.FeatureHandler.forEachFeature(FeatureHandler.java:63)
	at com.oracle.svm.hosted.NativeImageGenerator.runPointsToAnalysis(NativeImageGenerator.java:670)
	at com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:526)
	at com.oracle.svm.hosted.NativeImageGenerator.lambda$run$0(NativeImageGenerator.java:444)
	at java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1386)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

	at com.oracle.svm.core.util.UserError.abort(UserError.java:65)
	at com.oracle.svm.hosted.classinitialization.ConfigurableClassInitialization.checkDelayedInitialization(ConfigurableClassInitialization.java:494)
	at com.oracle.svm.hosted.classinitialization.ClassInitializationFeature.duringAnalysis(ClassInitializationFeature.java:188)
	at com.oracle.svm.hosted.NativeImageGenerator.lambda$runPointsToAnalysis$8(NativeImageGenerator.java:711)
	at com.oracle.svm.hosted.FeatureHandler.forEachFeature(FeatureHandler.java:63)
	at com.oracle.svm.hosted.NativeImageGenerator.runPointsToAnalysis(NativeImageGenerator.java:711)
	at com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:526)
	at com.oracle.svm.hosted.NativeImageGenerator.lambda$run$0(NativeImageGenerator.java:444)
	at java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1386)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

If I understand the GraalVM classes initialization logic correctly, there's a conflict between the --initialize-at-run-time=org.wildfly.common.net.CidrAddress,org.wildfly.common.net.Inet command-line option (the same thing happens with RuntimeInitializedClassBuildItem) and the fact that CidrAddress is initialized at build time because it is part of a recursive initialization of another class.

Even if the error message is not mentioning it, I suspected CidrAddressConverter was the class triggering the CidrAddress initialization, so I tried to initialize CidrAddressConverter at runtime and things only got worse (I didn't really expect it to work anyway!).

I'm pretty new to this subject so I might miss something obvious to solve this issue. Is there a way to initialize CidrAddress and Inet at runtime without the initialization conflict mentioned above?

cc @dmlloyd

Sanne · 2019-11-11T10:12:08Z

It should be fine to use a class at build time, even though we request it to be initialized at runtime - as long as we don't keep references to such instances around.

The error message seems to say something different; are you sure we don't have another feature / BuildItem somewhere which is requesting a different option for the same class?

gwenneg · 2019-11-11T10:20:43Z

The error message seems to say something different; are you sure we don't have another feature / BuildItem somewhere which is requesting a different option for the same class?

I also suspected this, but I didn't find anything. I'll take another look :)

Sanne · 2019-11-11T10:21:33Z

BTW I don't think the issue relates with when the class is initialized, it's probably about it keeping an instance in the heap.

The solution likely requires to defer the conversion from String to address objects, so to not have any such object in the heap yet.

dmlloyd

Initializing Inet during run time will cause some problems in the not-too-distant future.

gwenneg · 2019-11-11T17:57:28Z

Initializing Inet during run time will cause some problems in the not-too-distant future.

@dmlloyd: Is it ok to do it for the GraalVM 19.3.0 integration into Quarkus, as it seems to be a requirement according to @cstancu?

dmlloyd · 2019-11-11T19:44:58Z

Initializing Inet during run time will cause some problems in the not-too-distant future.

@dmlloyd: Is it ok to do it for the GraalVM 19.3.0 integration into Quarkus, as it seems to be a requirement according to @cstancu?

I don't think it is a requirement. I didn't find any comments justifying it at any rate.

gwenneg · 2019-11-11T19:49:13Z

@cstancu wrote:

Additional command-line options --initialize-at-run-time=org.wildfly.common.net.CidrAddress,org.wildfly.common.net.Inet. This is needed due to a new restriction that prohibits the presence of java.net.Inet4Address in the image heap.

You can find the original ZulipChat conversation here.

dmlloyd · 2019-11-11T19:58:57Z

OK the correct fix here would be to recompute the field values rather than initialize at run time. The fields in question are:

public final class Inet {
    // ...
    public static final Inet4Address INET4_ANY = getInet4Address(0, 0, 0, 0);
    // ...
    public static final Inet4Address INET4_LOOPBACK = getInet4Address(127, 0, 0, 1);
    // ...
    public static final Inet4Address INET4_BROADCAST = getInet4Address(255, 255, 255, 255);
    // ...
    public static final Inet6Address INET6_ANY = getInet6Address(0, 0, 0, 0, 0, 0, 0, 0);
    // ...
    public static final Inet6Address INET6_LOOPBACK = getInet6Address(0, 0, 0, 0, 0, 0, 0, 1);

They can each be recomputed using the FromAlias strategy, with values of the same expression that was given above.

gwenneg · 2019-11-11T20:24:45Z

Thanks for the pointers! I'll remove the runtime initialization and see if I can get the field recomputation to work.

gwenneg · 2019-11-12T22:22:05Z

I pushed some changes yesterday which didn't break the Quarkus master build and I am now trying to test these changes with the GraalVM master branch (or should I say I'm trying to figure out how to build and then use the branch for now).

gwenneg · 2019-11-14T23:19:29Z

I removed the URLClassPath changes from this PR since @galderz took care of it in #5336. This PR now only focuses on the java.net.Inet4Address issue mentioned in #4218 and discussed above.

I'm still not done with the GraalVM tests from the master branch.

cstancu · 2019-11-20T00:05:42Z

Yes, I mean those objects being reachable from a static field and thus being serialized in the native image heap. If possible I would recommend splitting the original class and extracting the part that needs to be recomputed to another class but being from wildfly that may not be possible. Alternatively, you can use @InjectAccessors to recompute the value at run time on first access. Here is an example of using @InjectAccessors: https://github.com/oracle/graal/blob/master/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SecuritySubstitutions.java#L297

dmlloyd · 2019-11-20T00:09:57Z

Yes, I mean those objects being reachable from a static field and thus being serialized in the native image heap. If possible I would recommend splitting the original class and extracting the part that needs to be recomputed to another class but being from wildfly that may not be possible.

You mean, extract it for the purpose of marking the extracted class as runtime-initialized? Or are you saying that analysis treats class boundaries specially?

Alternatively, you can use @InjectAccessors to recompute the value at run time on first access. Here is an example of using @InjectAccessors: https://github.com/oracle/graal/blob/master/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SecuritySubstitutions.java#L297

Thanks for the info. How would @InjectAccessors differ from recomputing the field value? Wouldn't recomputing the field value cause the original value to not be recorded? This is what I have always assumed was the case.

cstancu · 2019-11-20T00:18:36Z

extract it for the purpose of marking the extracted class as runtime-initialized

Yes.

How would @InjectAccessors differ from recomputing the field value?

Recomputation via @RecomputeFieldValue is a build time recomputation and the resulting value is recorded in the image heap; the original field value is indeed discarded. @InjectAccessors allows a run time recomputation.

gwenneg · 2019-11-20T00:19:30Z

Thanks for your answers @cstancu! I just ran a quick test and it seems @InjectAccessors solved my problem.

dmlloyd · 2019-11-20T00:54:53Z

How would @InjectAccessors differ from recomputing the field value?

Recomputation via @RecomputeFieldValue is a build time recomputation and the value is recorded in the image heap. @InjectAccessors allows a run time recomputation.

Uh oh. This means that we have some code making some majorly wrong assumptions. Unfortunately the documentation of @RecomputeFieldValue doesn't explicitly state when the value is taken; I assumed that it would have been recomputed at run time (in a similar process to run time static initialization) because, I suppose, that's the way I hoped it would work.

gwenneg · 2019-11-20T01:07:20Z

I'm pushing an updated version of the PR to see if the Quarkus CI is happy with it, but I'll do another round of tests tomorrow.

gwenneg · 2019-11-20T01:08:54Z

core/runtime/src/main/java/io/quarkus/runtime/graal/CidrAddressSubstitutions.java

+        try {
+            Constructor<CidrAddress> ctor = CidrAddress.class.getDeclaredConstructor(InetAddress.class, int.class);
+            ctor.setAccessible(true);
+            result = ctor.newInstance(Inet.INET6_ANY, 0);
+        } catch (NoSuchMethodException | SecurityException | InstantiationException | IllegalAccessException
+                | IllegalArgumentException | InvocationTargetException e) {
+            throw new RuntimeException("CidrAddress instanciation failed", e);
+        }


@dmlloyd Is this acceptable? The CidrAddress constructor is private so I need a way to access it.

Use an alias for the constructor instead.

I tried the alias way last night and it didn't work, but it was super late for me so I didn't really try to analyze the reason and decided to test this alternative that I knew would work. I'll replace this with an alias again tonight and see if I can make it work. Thanks.

dmlloyd · 2019-11-20T20:25:28Z

core/runtime/src/main/java/io/quarkus/runtime/graal/InetSubstitutions.java

+    public static Inet6Address INET6_LOOPBACK;
+}
+
+class Inet4AnyAccessor {


I think these accessors could be substantially simplified by introducing a new class, which is a run-time initialized class that includes the fields like this:

static final class Inet_RunTime { public static final Inet4Address INET4_ANY = Inet.getInet4Address(0, 0, 0, 0); public static final Inet4Address INET4_LOOPBACK = Inet.getInet4Address(127, 0, 0, 1); public static final Inet4Address INET4_BROADCAST = Inet.getInet4Address(255, 255, 255, 255); public static final Inet6Address INET6_ANY = Inet.getInet6Address(0, 0, 0, 0, 0, 0, 0, 0); public static final Inet6Address INET6_LOOPBACK = Inet.getInet6Address(0, 0, 0, 0, 0, 0, 0, 1); }

Then each accessor only has to look like this (for example):

static Inet4Address get() { return Inet_RunTime.INET4_LOOPBACK; }

and that would allow the accessor to be inlined, effectively performing a safe(r) partial class reinitialization at run time of that class.

Simplifying the code is exactly what I was thinking about now that the reflection code is gone :) I'll try your suggestion right away, thanks!

It seems GraalVM doesn't like this:

static final class Inet_RunTime { public static final Inet4Address INET4_ANY = Inet.getInet4Address(0, 0, 0, 0); public static final Inet4Address INET4_LOOPBACK = Inet.getInet4Address(127, 0, 0, 1); public static final Inet4Address INET4_BROADCAST = Inet.getInet4Address(255, 255, 255, 255); public static final Inet6Address INET6_ANY = Inet.getInet6Address(0, 0, 0, 0, 0, 0, 0, 0); public static final Inet6Address INET6_LOOPBACK = Inet.getInet6Address(0, 0, 0, 0, 0, 0, 0, 1); }

It is causing many errors locally including:

Error: com.oracle.graal.pointsto.constraints.UnsupportedFeatureException: No instances of java.net.Inet4Address are allowed in the image heap as this class should be initialized at image runtime. Object has been initialized without the native-image initialization instrumentation and the stack trace can't be tracked. Trace: at parsing io.quarkus.runtime.graal.Inet4AnyAccessor.get(InetSubstitutions.java:51)

Oh wait, I didn't do anything about initializing the class at runtime. Trying again...

Suggestion applied and working.

gwenneg · 2019-11-20T22:21:02Z

@dmlloyd Would you please review this PR again? Everything's working fine locally so I think it's ready to be merged into master.

dmlloyd · 2019-11-20T22:39:21Z

Sure, seems better. @gsmet ?

Gwenneg has tested and validated the changes.

gsmet · 2019-11-20T22:46:30Z

I dismissed my review. Looks like we just have to wait for CI? Adding the tag.

machi1990 · 2019-11-21T06:32:41Z

CI is green, merging. Thanks @gwenneg @gsmet @dmlloyd @cstancu

gwenneg · 2019-11-21T06:45:25Z

Thanks you all for your help with this one!

gwenneg added the coding-in-progress label Nov 9, 2019

gwenneg changed the title ~~Prepare for GraalVM and JDK 11~~ Prepare for GraalVM 19.3.0 and JDK 11 Nov 9, 2019

gwenneg removed the coding-in-progress label Nov 9, 2019

gwenneg marked this pull request as ready for review November 9, 2019 23:11

geoand mentioned this pull request Nov 10, 2019

Upgrade GraalVM to 19.3.0 #5358

Merged

gwenneg force-pushed the issue-4218-graalvm-19.3.0 branch from f301b62 to 4107b19 Compare November 10, 2019 17:58

Sanne reviewed Nov 10, 2019

View reviewed changes

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java Outdated Show resolved Hide resolved

gwenneg added the coding-in-progress label Nov 10, 2019

dmlloyd requested changes Nov 11, 2019

View reviewed changes

gwenneg force-pushed the issue-4218-graalvm-19.3.0 branch from 4107b19 to e4aa525 Compare November 11, 2019 22:24

cescoffier removed the coding-in-progress label Nov 13, 2019

gwenneg force-pushed the issue-4218-graalvm-19.3.0 branch from e4aa525 to 3746489 Compare November 14, 2019 23:15

gwenneg force-pushed the issue-4218-graalvm-19.3.0 branch from 3746489 to 213186b Compare November 20, 2019 01:06

gwenneg commented Nov 20, 2019

View reviewed changes

gwenneg changed the title ~~Recompute org.wildfly.common.net.Inet fields~~ Lazily recompute Inet4Address, Inet6Address and CidrAddress fields at runtime Nov 20, 2019

gwenneg force-pushed the issue-4218-graalvm-19.3.0 branch 3 times, most recently from e558604 to 93a2acc Compare November 20, 2019 20:19

dmlloyd reviewed Nov 20, 2019

View reviewed changes

Lazily recompute InetAddress subclasses fields at runtime

e2c6932

gwenneg force-pushed the issue-4218-graalvm-19.3.0 branch from 93a2acc to e2c6932 Compare November 20, 2019 22:11

gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 20, 2019

machi1990 merged commit afde1e8 into quarkusio:master Nov 21, 2019

machi1990 added this to the 1.1.0 milestone Nov 21, 2019

machi1990 added the backport? label Nov 21, 2019

gwenneg deleted the issue-4218-graalvm-19.3.0 branch November 21, 2019 06:45

gwenneg mentioned this pull request Nov 21, 2019

Upgrade to GraalVM 19.3.0 / Java 8 edition #4218

Closed

jaikiran mentioned this pull request Nov 22, 2019

Fix resource registration in native-image for 19.3 of Graal #5676

Closed

gsmet removed the backport? label Nov 22, 2019

gwenneg mentioned this pull request Nov 28, 2019

cannot build a vertx native image with 19.3.0 oracle/graal#1902

Closed

Lazily recompute Inet4Address, Inet6Address and CidrAddress fields at runtime #5353

Lazily recompute Inet4Address, Inet6Address and CidrAddress fields at runtime #5353

Conversation

gwenneg commented Nov 9, 2019 • edited Loading

machi1990 commented Nov 10, 2019

gwenneg commented Nov 10, 2019

gwenneg commented Nov 10, 2019 • edited Loading

gsmet commented Nov 10, 2019

gwenneg commented Nov 10, 2019

gwenneg commented Nov 10, 2019

gwenneg commented Nov 10, 2019 • edited Loading

gwenneg commented Nov 11, 2019

Sanne commented Nov 11, 2019

gwenneg commented Nov 11, 2019

Sanne commented Nov 11, 2019

dmlloyd left a comment

Choose a reason for hiding this comment

gwenneg commented Nov 11, 2019

dmlloyd commented Nov 11, 2019

gwenneg commented Nov 11, 2019 • edited Loading

dmlloyd commented Nov 11, 2019

gwenneg commented Nov 11, 2019

gwenneg commented Nov 12, 2019

gwenneg commented Nov 14, 2019

cstancu commented Nov 20, 2019

dmlloyd commented Nov 20, 2019

cstancu commented Nov 20, 2019 • edited Loading

gwenneg commented Nov 20, 2019

dmlloyd commented Nov 20, 2019

gwenneg commented Nov 20, 2019 • edited Loading

gwenneg Nov 20, 2019

Choose a reason for hiding this comment

dmlloyd Nov 20, 2019

Choose a reason for hiding this comment

gwenneg Nov 20, 2019

Choose a reason for hiding this comment

dmlloyd Nov 20, 2019

Choose a reason for hiding this comment

gwenneg Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

gwenneg Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

gwenneg Nov 20, 2019

Choose a reason for hiding this comment

gwenneg Nov 20, 2019

Choose a reason for hiding this comment

gwenneg commented Nov 20, 2019

dmlloyd commented Nov 20, 2019

gsmet commented Nov 20, 2019

machi1990 commented Nov 21, 2019 • edited Loading

gwenneg commented Nov 21, 2019

gwenneg commented Nov 9, 2019 •

edited

Loading

gwenneg commented Nov 10, 2019 •

edited

Loading

gwenneg commented Nov 10, 2019 •

edited

Loading

gwenneg commented Nov 11, 2019 •

edited

Loading

cstancu commented Nov 20, 2019 •

edited

Loading

gwenneg commented Nov 20, 2019 •

edited

Loading

gwenneg Nov 20, 2019 •

edited

Loading

gwenneg Nov 20, 2019 •

edited

Loading

machi1990 commented Nov 21, 2019 •

edited

Loading