-
Notifications
You must be signed in to change notification settings - Fork 263
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
fix(java): References within InvocationHandler, Issue #1364 #1365
Conversation
java/fury-core/src/main/java/org/apache/fury/serializer/JdkProxySerializer.java
Outdated
Show resolved
Hide resolved
java/fury-core/src/main/java/org/apache/fury/serializer/JdkProxySerializer.java
Outdated
Show resolved
Hide resolved
…dded JdkProxySerializer to native-image.properties
…pottless apply
…ptimizations from discussion
ffcd41f
to
bc5b56a
Compare
import org.apache.fury.util.Preconditions; | ||
import org.apache.fury.util.ReflectionUtils; | ||
|
||
/** Serializer for jdk {@link Proxy}. */ | ||
@SuppressWarnings({"rawtypes", "unchecked"}) | ||
public class JdkProxySerializer extends Serializer { | ||
|
||
// Make offset compatible with graalvm native image. | ||
private static final long PROXY_HANDLER_FIELD_OFFSET = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try catch block is still needed, graalvm only capture this pattern and replace the offset. This invocation can be placed in catch block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I leave the InvocationHandler stub initialization as is or move it to the static block as well? I'm new to grallvm but it's on my todo list 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave it as it looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I just stumbled over the try...catch mention. Is a try / catch really necessary or is the static block okay? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have something like this in mind?
static {
try {
// Make offset compatible with graalvm native image.
PROXY_HANDLER_FIELD_OFFSET =
Platform.objectFieldOffset(Proxy.class.getDeclaredField("h"));
} catch (NoSuchFieldException e) {
PROXY_HANDLER_FIELD_OFFSET = Platform.objectFieldOffset(Proxy.class, InvocationHandler.class);
}
}
But I think i have to work with a temp assignment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This compiles but I'm not sure if it works for graalvm?
static {
long proxyHanlderFieldOffset;
try {
// Make offset compatible with graalvm native image.
proxyHanlderFieldOffset = Platform.objectFieldOffset(Proxy.class.getDeclaredField("h"));
} catch (NoSuchFieldException e) {
proxyHanlderFieldOffset = Platform.objectFieldOffset(Proxy.class, InvocationHandler.class);
}
PROXY_HANDLER_FIELD_OFFSET = proxyHanlderFieldOffset;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I studied the redhat documentation in the link provided. So it seems for graalvm we assume that the name of the field is alway's 'h' and for other JVM's it could differ.
I updated the PR with this variant:
static {
if (GraalvmSupport.isGraalBuildtime()) {
try {
// Make offset compatible with graalvm native image.
PROXY_HANDLER_FIELD_OFFSET = Platform.objectFieldOffset(Proxy.class.getDeclaredField("h"));
} catch (NoSuchFieldException e) {
throw new RuntimeException(e);
}
} else {
// not all JVM implementations use 'h' as internal InvocationHandler name
PROXY_HANDLER_FIELD_OFFSET =
ReflectionUtils.getFieldOffset(Proxy.class, InvocationHandler.class);
}
}
My intention is to use the graalvm 'automatic fix' pattern when it's executed during graalvm build time and the other variant when executed during normal JVM execution.
Could this work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, may need a test. Could we add a test in integration_tests/graalvm_tests?
FYI, I tested following pattern, it works:
private static final Field field;
private static final long offset;
static {
field = ReflectionUtils.getField(Example.class, "f2");
offset = Platform.objectFieldOffset(field);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proxy in graalvm is a little complex, may need to add a json config file: https://www.graalvm.org/latest/reference-manual/native-image/guides/configure-dynamic-proxies/
If it's not easy to add, we can leave it in another PR
…heckstyle fixups
…raalvm related optimizations
…tub initializer on variable declaration
java/fury-core/src/main/java/org/apache/fury/serializer/JdkProxySerializer.java
Show resolved
Hide resolved
java/fury-core/src/main/java/org/apache/fury/util/Platform.java
Outdated
Show resolved
Hide resolved
…ptimizations from discussion
Hi @cn-at-osmit , thanks for contribute this fix, I'll merge it first and add graalvm tests in a following-up PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is the fix for the issue #1364 including a test case ;)