-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: gauss-mix and als benchmarks. #3581
Conversation
Hello jovanstevanovic, thanks for contributing a PR to our project! We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address jovan -(dot)- stevanovic1 -(at)- outlook -(dot)- com. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
The PR looks good but the OCA check failed. Could it be that you used a different email address in your commits? |
a731297
to
eb7bf71
Compare
@@ -942,6 +942,37 @@ private static boolean objectStreamClassConstructor(JNIEnvironment jni, Breakpoi | |||
return true; | |||
} | |||
|
|||
private static boolean customTargetConstructorSerialization(JNIEnvironment jni, @SuppressWarnings("unused") Breakpoint bp, InterceptedState state) { |
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.
We need a comment here explaining what we catch and what we output.
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.
Provided a short description.
eb7bf71
to
31e5798
Compare
@@ -1241,6 +1279,9 @@ public static void onUnload() { | |||
"(Ljava/lang/ClassLoader;[Ljava/lang/Class;Ljava/lang/reflect/InvocationHandler;)Ljava/lang/Object;", BreakpointInterceptor::newProxyInstance), | |||
|
|||
brk("java/io/ObjectStreamClass", "<init>", "(Ljava/lang/Class;)V", BreakpointInterceptor::objectStreamClassConstructor), | |||
brk("jdk/internal/reflect/ReflectionFactory", |
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.
On Java 8, this class would be called sun.reflect.ReflectionFactory
- since this breakpoint is not optional, this would make the agent always fail on Java 8 as it won't be able to resolve this class. We could either make this breakpoint an optionalBrk
or we could generate the class name based on the JDK version (using com.oracle.svm.core.jdk.Package_jdk_internal_reflect#getQualifiedName
)
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.
Nice catch! Done!
31e5798
to
6734257
Compare
6734257
to
9d139df
Compare
Stacktrace:
Command to reproduce:
mx --env ni-ce benchmark renaissance-native-image:als -- --jvm=native-image --jvm-config=default-ce
mx --env ni-ce benchmark renaissance-native-image:gauss-mix -- --jvm=native-image --jvm-config=default-ce