No build time init of classes used in UnsafeAccessedFieldBuildItem
#39831
Conversation
|
I think it should eventually be backported at least down to 3.8 |
|
@zakkak I've done a check on my branch for #39830, reverting to using The Mutiny native tests fail on 17 ( Here's the diff: diff --git a/core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageFeatureStep.java b/core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageFeatureStep.java
index 7fff7025f30..6f86d02d3dd 100644
--- a/core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageFeatureStep.java
+++ b/core/deployment/src/main/java/io/quarkus/deployment/steps/NativeImageFeatureStep.java
@@ -80,25 +80,30 @@ public void write(String s, byte[] bytes) {
ResultHandle beforeAnalysisParam = beforeAn.getMethodParam(0);
- MethodCreator registerAsUnsafeAccessed = file
- .getMethodCreator("registerAsUnsafeAccessed", void.class, Feature.BeforeAnalysisAccess.class)
- .setModifiers(Modifier.PRIVATE | Modifier.STATIC);
- for (UnsafeAccessedFieldBuildItem unsafeAccessedField : unsafeAccessedFields) {
- TryBlock tc = registerAsUnsafeAccessed.tryBlock();
- ResultHandle declaringClassHandle = tc.invokeStaticMethod(
- ofMethod(Class.class, "forName", Class.class, String.class),
- tc.load(unsafeAccessedField.getDeclaringClass()));
- ResultHandle fieldHandle = tc.invokeVirtualMethod(
- ofMethod(Class.class, "getDeclaredField", Field.class, String.class), declaringClassHandle,
- tc.load(unsafeAccessedField.getFieldName()));
- tc.invokeInterfaceMethod(
- ofMethod(Feature.BeforeAnalysisAccess.class, "registerAsUnsafeAccessed", void.class, Field.class),
- registerAsUnsafeAccessed.getMethodParam(0), fieldHandle);
- CatchBlockCreator cc = tc.addCatch(Throwable.class);
- cc.invokeVirtualMethod(ofMethod(Throwable.class, "printStackTrace", void.class), cc.getCaughtException());
+ if (!unsafeAccessedFields.isEmpty()) {
+ MethodCreator registerAsUnsafeAccessed = file
+ .getMethodCreator("registerAsUnsafeAccessed", void.class, Feature.BeforeAnalysisAccess.class)
+ .setModifiers(Modifier.PRIVATE | Modifier.STATIC);
+ ResultHandle thisClass = registerAsUnsafeAccessed.loadClassFromTCCL(GRAAL_FEATURE);
+ ResultHandle cl = registerAsUnsafeAccessed
+ .invokeVirtualMethod(ofMethod(Class.class, "getClassLoader", ClassLoader.class), thisClass);
+ for (UnsafeAccessedFieldBuildItem unsafeAccessedField : unsafeAccessedFields) {
+ TryBlock tc = registerAsUnsafeAccessed.tryBlock();
+ ResultHandle declaringClassHandle = tc.invokeStaticMethod(
+ ofMethod(Class.class, "forName", Class.class, String.class, boolean.class, ClassLoader.class),
+ tc.load(unsafeAccessedField.getDeclaringClass()), tc.load(false), cl);
+ ResultHandle fieldHandle = tc.invokeVirtualMethod(
+ ofMethod(Class.class, "getDeclaredField", Field.class, String.class), declaringClassHandle,
+ tc.load(unsafeAccessedField.getFieldName()));
+ tc.invokeInterfaceMethod(
+ ofMethod(Feature.BeforeAnalysisAccess.class, "registerAsUnsafeAccessed", void.class, Field.class),
+ registerAsUnsafeAccessed.getMethodParam(0), fieldHandle);
+ CatchBlockCreator cc = tc.addCatch(Throwable.class);
+ cc.invokeVirtualMethod(ofMethod(Throwable.class, "printStackTrace", void.class), cc.getCaughtException());
+ }
+ registerAsUnsafeAccessed.returnVoid();
+ overallCatch.invokeStaticMethod(registerAsUnsafeAccessed.getMethodDescriptor(), beforeAnalysisParam);
}
- registerAsUnsafeAccessed.returnVoid();
- overallCatch.invokeStaticMethod(registerAsUnsafeAccessed.getMethodDescriptor(), beforeAnalysisParam);
overallCatch.invokeStaticMethod(BUILD_TIME_INITIALIZATION,
overallCatch.marshalAsArray(String.class, overallCatch.load(""))); // empty string means initialize everything
@@ -188,4 +193,4 @@ public void write(String s, byte[] bytes) {
file.close();
}
-}
+}
\ No newline at end of file
diff --git a/extensions/mutiny/deployment/src/main/java/io/quarkus/mutiny/deployment/MutinyProcessor.java b/extensions/mutiny/deployment/src/main/java/io/quarkus/mutiny/deployment/MutinyProcessor.java
index 0802ea01dc8..b31d1958752 100644
--- a/extensions/mutiny/deployment/src/main/java/io/quarkus/mutiny/deployment/MutinyProcessor.java
+++ b/extensions/mutiny/deployment/src/main/java/io/quarkus/mutiny/deployment/MutinyProcessor.java
@@ -1,5 +1,6 @@
package io.quarkus.mutiny.deployment;
+import java.util.List;
import java.util.Optional;
import org.jboss.threads.ContextHandler;
@@ -10,6 +11,7 @@
import io.quarkus.deployment.builditem.ContextHandlerBuildItem;
import io.quarkus.deployment.builditem.ExecutorBuildItem;
import io.quarkus.deployment.builditem.ShutdownContextBuildItem;
+import io.quarkus.deployment.builditem.nativeimage.UnsafeAccessedFieldBuildItem;
import io.quarkus.mutiny.runtime.MutinyInfrastructure;
public class MutinyProcessor {
@@ -31,4 +33,12 @@ public void buildTimeInit(MutinyInfrastructure recorder) {
recorder.configureThreadBlockingChecker();
recorder.configureOperatorLogger();
}
+
+ @BuildStep
+ public List<UnsafeAccessedFieldBuildItem> jctoolsUnsafeAccessedFields() {
+ return List.of(
+ new UnsafeAccessedFieldBuildItem(
+ "org.jctools.util.UnsafeRefArrayAccess",
+ "REF_ELEMENT_SHIFT"));
+ }
} |
Yes that was the idea.
Thanks for trying this. I only tested with the reproducer from #39819 which worked for me. I will have a look at the mutiny failure and get back. |
This comment has been minimized.
This comment has been minimized.
zakkak
force-pushed
the
2024-04-02-fix-registerAsUnsafeAccessed
branch
from
a2407bc to
d658f77
Compare
|
That was an interesting one... The order in which you configure the unsafe accesses in regards to the order you configure class initialization is important (see https://github.com/quarkusio/quarkus/pull/39831/files?diff=unified&w=1#diff-7d1388bbdd07a9362ef261153d5cc4cbccd0f5f5f95446ad3f647f9cd38768bcR160-R162) @jponge with this update the native image build succeeds but we still need https://github.com/quarkusio/quarkus/pull/39830/files#diff-cb8a750bd3e4b8f2bafd28b98718213cc1b2e328fbd53822ae9361f5038b5375 to avoid the segmentation faults. At this point I really can't tell whether https://github.com/quarkusio/quarkus/pull/39830/files#diff-4fde0a5712f7c9b1f42e991f2e26671023e1a8944e68ae63f507b8a59657de0a needs to be applied or not. It's not clear to me what it does. I am also a bit concerned that we might have other issues waiting to appear as jctools uses unsafe quite extensively. In any case, regardless of the above, this PR appears to be in the right direction. Please have another look. |
That's a fun one indeed!
👍
I initially needed this when Mutiny started using JCTools to fix the native compilation on that particular class/field.
I will do, thanks! |
|
I would check @zakkak if native image would take care of removing the unused padding fields we have on JCTools. In case it will, using the padded variant won't be a big deal for native image (but it will, for JVM mode...) |
True and:
|
|
FYI netty/netty#13945 freshly created; especially with the jboss executor (which can still use fast thread locals Recyclers while allocating pooled ByteBuf on Jackson extension) the new JCTools release can further reduce our memory footprint |
This comment has been minimized.
This comment has been minimized.
|
I am looking at the breaking tests. It looks like the change moved some classes from being build-time initialized to run-time initialized and vice versa and we now need changes like https://github.com/quarkusio/quarkus/pull/39830/files#diff-cb8a750bd3e4b8f2bafd28b98718213cc1b2e328fbd53822ae9361f5038b5375 in more places. |
@franz1981 That's a good idea, but how can I test it? I did a search for I could try with a minimal example that directly uses JCTools but would that be enough? |
|
These are the type of fields which are used as padding: https://github.com/JCTools/JCTools/blob/master/jctools-core/src/main/java/org/jctools/queues/BaseMpscLinkedArrayQueue.java#L34 IDK how is possible to inspect the layout (similar to what JOL) of the user defined classes which native image compile (and likely, educated guess..) strip of unreachable/unused fields, to shrink them. Maybe is an allucination of mine, but I remember that native image could perform such optimization |
Mutiny currently uses the unpadded variants in https://github.com/JCTools/JCTools/tree/master/jctools-core/src/main/java/org/jctools/queues/unpadded (but they use Does that make sense? |
|
@franz1981 I created https://github.com/zakkak/issue-reproducers/tree/2024-04-03-jctools-test to demonstrate that the padding fields are indeed being eliminated, please have a look at the readme. |
Use the Feature classloader instead of the default one to prevent classes being loaded to register fields as unsafe accessed to end up being build time initialized. Similarly ensure that the registration is done after class initialization configuration to prevent classes from being configured for runtime initialization because of the unsafe access registration.
zakkak
force-pushed
the
2024-04-02-fix-registerAsUnsafeAccessed
branch
from
12ccf6d to
07f7063
Compare
|
That's super interesting @zakkak 👍 |
|
Thanks @zakkak for this so our educated guesses seems to hold, for the good and worse, because it means that padding is just some useless crap for native image, unless configured to not ignore it :/ Said that, the sole case where it matters, performance wise, is on the unbounded mpsc queues used as task mailboxes attached to the Netty event loops (1:1 per event loop); the others shouldn't be that highly contended... |
|
@franz1981 For Mutiny the strategy is to switch to atomic/unpadded as soon as there is a release. I don't think it's worth rushing a switch to atomic/padded in the interim. |
Status for workflow
|
Use the Feature classloader instead of the default one to prevent classes being loaded to register fields as unsafe accessed to end up being build time initialized.
Resolves issues like the one described in #39819 (comment)
Note: to better understand what changed use https://github.com/quarkusio/quarkus/pull/39831/files?diff=unified&w=1