eclipse-vertx / vert.x

Vert.x is a tool-kit for building reactive applications on the JVM
http://vertx.io
Other
14.31k stars 2.08k forks source link

Add small native image build project to the repo to catch regressions #4147

Open jvican opened 3 years ago

jvican commented 3 years ago

Describe the feature

The latest vert.x release didn't compile against GraalVM 21.1.0, 21.2.0 and 21.3.0 (the three latest releases) because of some internal changes in the project. In particular, I think changes in VertxInternal are causing static initializers that require netty's PooledByteBufAllocator to be initialized at build time.

I fixed this issue in https://github.com/vertx-howtos/graal-native-image-howto/pull/11 but I was wondering that to avoid future regressions it would be wise to:

  1. Add native-image.properties to the vertx project so that GraalVM automatically picks up the native image settings and downstream consumers don't need to specify them themselves.
  2. Add a small native-image project that uses those settings to compile an http server to native and runs a small workload to force most of vert.x to be initialized and guarantee that it works.

As a result, if a new change that alters how static initializers work was to happen and compiling to native failed, then the CI would also fail and it would be on the vert.x contributor to fix it before merging.

Use cases

These two changes should hopefully prevent any future regressions from happening again and would allow downstream consumers to compile to native automatically without any configuration.

Contribution

I don't currently have the time to implement this but can provide guidance if needed.

vietj commented 3 years ago

we should consider having a CI build for this.

@cescoffier noticted it and the PooledByteBufAllocator has been moved to another class instead

vietj commented 3 years ago

can you try with latest master @jvican ?

cescoffier commented 3 years ago

There are other adjustments to do:

I would not add a native-image.properties as most frameworks using Vert.x would need to exclude it to tune the native compilation

cescoffier commented 3 years ago

If you are using Vert.x Web, depending on your app, you may need to delete some classes using code from optional dependencies not available during the build.

vietj commented 3 years ago

I think we should define a way to handle optional dependencies in a way that it always work too.

On Tue, Oct 26, 2021 at 8:30 AM Clement Escoffier @.***> wrote:

If you are using Vert.x Web, depending on your app, you may need to delete some classes using code from optional dependencies not available during the build.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/eclipse-vertx/vert.x/issues/4147#issuecomment-951603746, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCSAGE3CXR5PYPM37NDUIZKI5ANCNFSM5GWLVQHA .

jvican commented 3 years ago

noticted it and the PooledByteBufAllocator has been moved to another class instead

Yes, indeed, I also made the change to PooledByteBufAllocator locally but didn't include it in the PR. It's not clear if it's necessary because netty configures ByteBufAllocator as a class that must be initialized at runtime and any subclasses of it are also force likewise.

move some classes to runtime initialization (but not VertxInternal, as the consequence could be massive)

I tried pretty much all the callers for PooledByteBufAllocator in the codebase and only VertxInternal worked to prevent the initialization of ByteBufAllocator. Unfortunately, using --trace-class-initialization didn't work at all in my project so it's very disappointing to know who was the direct caller. Maybe we should try to remove the PooledByteBufAllocator references in VertxInternal to avoid needing to add it to runtime initialization.

I would not add a native-image.properties as most frameworks using Vert.x would need to exclude it to tune the native compilation

I don't think this is a good argument and it goes against what most libraries are doing. It's rare that you know better than the library maintainers and want to override the native-image.properties settings which need to be evolved with the code. Netty (and other project such as reactor, spring, micronaut, etc) include one native-image properties per project and that strategy works very well for them. When a class is truly configured to be initialized at runtime, it's because initializing it at build time (the default) isn't safe.

Note that the fact that we'd add these files wouldn't meant that users don't need to provide their native-image.properties. They still do.

can you try with latest master @jvican ?

Just to make it clear, my PR above is (I think) all you need to make current master compile to native. We can add the PooledByteBufAllocator change as well (as I did locally in my build) but I suspect it's not necessary.

cescoffier commented 3 years ago

The native-images properties are not a great approach to say the less. You can enable any flag in them, which will be global and NOT limited to the library. For us, it's a terrible pattern and we had to workaround that many times. If you want a native-images.properties provide it in a separate artifact. So, consumers can decideto use it or not. I would stay away from them as much as possible because it's not only knowing the library, it's also knowing native compilation and what your application/framework is trying to do.

About your PR, there are already changes in the master branch that avoid having VertxInternal being initialized at runtime. Typically, to go back to the previous topic, having a native-image.properties with that configuration would break Quarkus, as we need to initialize VertxInternal at build time (we inline a lot).

jvican commented 3 years ago

You can enable any flag in them, which will be global and NOT limited to the library.

Right that's why I suggest in the comment above that we provide the only vertx-related bits in native-image.properties, which is just the --initialize-at-run-time configurations. This is what many libraries such as Netty do. These settings are true regardless of what users do in application land.

For us, it's a terrible pattern and we had to workaround that many times.

If we constrain ourselves to runtime initialization settings, we will only block downstream users that want to initialize something that is marked as runtime at build time. If that's the case, either the library maintainers in vert.x forgot to do that change (which is a bug, in this case) or the downstream user is wrong and that can't be done. Totally worth paying this price, because just being able to use the defaults covers more than 80% of the common use cases.

About your PR, there are already changes in the master branch that avoid having VertxInternal being initialized at runtime.

Great, that wasn't clear — happy that we can inline as much as before. Will use the latest release and see how it goes.

jvican commented 2 years ago

I've just tried to upgrade to 4.2.1 and remove io.vertx.core.impl.VertxInternal,\ from my changes in https://github.com/vertx-howtos/graal-native-image-howto/pull/11 and I still get the same error as before:

Error: Classes that should be initialized at run time got initialized during image building:
 io.netty.buffer.PooledByteBufAllocator the class was requested to be initialized at run time (from jar:file:///Users/jvican/Code/nflx-notebooks-graphql-2.0/vertx-native/build/libs/vertx-native.jar!/META-INF/native-image/nflxnotebooks/bootloader/native-image.properties with 'io.netty.buffer.PooledByteBufAllocator' and from jar:file:///Users/jvican/.gradle/caches/modules-2/files-2.1/io.netty/netty-buffer/4.1.69.Final/c87da90e422b331ecd1e157ca77e5300348b6d0d/netty-buffer-4.1.69.Final.jar!/META-INF/native-image/io.netty/buffer/native-image.properties with 'io.netty.buffer.PooledByteBufAllocator'). io.netty.buffer.PooledByteBufAllocator has been initialized without the native-image initialization instrumentation and the stack trace can't be tracked. Try avoiding to initialize the class that caused initialization of io.netty.buffer.PooledByteBufAllocator

com.oracle.svm.core.util.UserError$UserException: Classes that should be initialized at run time got initialized during image building:
 io.netty.buffer.PooledByteBufAllocator the class was requested to be initialized at run time (from jar:file:///Users/jvican/Code/nflx-notebooks-graphql-2.0/vertx-native/build/libs/vertx-native.jar!/META-INF/native-image/nflxnotebooks/bootloader/native-image.properties with 'io.netty.buffer.PooledByteBufAllocator' and from jar:file:///Users/jvican/.gradle/caches/modules-2/files-2.1/io.netty/netty-buffer/4.1.69.Final/c87da90e422b331ecd1e157ca77e5300348b6d0d/netty-buffer-4.1.69.Final.jar!/META-INF/native-image/io.netty/buffer/native-image.properties with 'io.netty.buffer.PooledByteBufAllocator'). io.netty.buffer.PooledByteBufAllocator has been initialized without the native-image initialization instrumentation and the stack trace can't be tracked. Try avoiding to initialize the class that caused initialization of io.netty.buffer.PooledByteBufAllocator

As reported before, using --trace-class-initialization=io.netty.buffer.PooledByteBufAllocator,\ io.vertx.core.buffer.impl.PartialPooledByteBufAllocator,\ io.netty.buffer.PooledByteBufAllocator,\ io.netty.util.AbstractReferenceCounted \ with GraalVM 21.3.0 doesn't work and the stack trace isn't printed, so I'm at a loss figuring out what's the culprit.

As expected, adding VertxInternal to the configuration mix makes it compile to native successfully.

@cescoffier Are you aware of other classes that need to be added to the runtime configuration for vertex 4.2.1 to compile to native successfully without VertxInternal?

jvican commented 2 years ago

Alright, actually took another close look at the commit that @vietj did to fix this and realized that need to change the VertxInternal reference by io.vertx.core.buffer.impl.VertxByteBufAllocator. I updated the native-image.properties file in our guides accordingly https://github.com/vertx-howtos/graal-native-image-howto/pull/create?base=vertx-howtos%3Amaster&head=jvican%3Apatch-2`

With those changes, vert.x 4.2.1 is compiling to native as expected.

himanshumps commented 2 years ago

@jvican ... Do you have a repo which compiles with vertx 4.2.1 to a native image ?

ksclarke commented 2 years ago

@himanshumps I made changes in a PR to the howto repo that does this, but I need to get back to fixing up the PR suggestions before it can be merged (it has formatting issues): https://github.com/vertx-howtos/graal-native-image-howto/pull/13#pullrequestreview-825627433 (but building / running the branch should work for you).