Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: db-shootout benchmark. #3570

Closed
wants to merge 1 commit into from
Closed

Fix: db-shootout benchmark. #3570

wants to merge 1 commit into from

Conversation

jovanstevanovic
Copy link
Member

Stacktraces:

java.lang.UnsatisfiedLinkError: Unsupported JNI version 0x0, required by /tmp/jna-118974/jna17979804611692108780.tmp
at com.oracle.svm.jni.JNILibraryInitializer.checkSupportedJNIVersion(JNILibraryInitializer.java:82)
at com.oracle.svm.jni.JNILibraryInitializer.callOnLoadFunction(JNILibraryInitializer.java:75)
at com.oracle.svm.jni.JNILibraryInitializer.initialize(JNILibraryInitializer.java:131)
at com.oracle.svm.core.jdk.NativeLibrarySupport.addLibrary(NativeLibrarySupport.java:186)
at com.oracle.svm.core.jdk.NativeLibrarySupport.loadLibrary0(NativeLibrarySupport.java:142)
at com.oracle.svm.core.jdk.NativeLibrarySupport.loadLibraryAbsolute(NativeLibrarySupport.java:101)
at java.lang.ClassLoader.loadLibrary(ClassLoader.java:195)
at java.lang.Runtime.load0(Runtime.java:768)
at java.lang.System.load(System.java:1835)
at com.sun.jna.Native.loadNativeDispatchLibraryFromClasspath(Native.java:851)
at com.sun.jna.Native.loadNativeDispatchLibrary(Native.java:826)
at com.sun.jna.Native.(Native.java:140)

After solving a problem with loading native library, the next problem occurred:

Caused by: java.lang.IllegalArgumentException: Can't determine class with native methods from the current context (class com.oracle.svm.core.classinitialization.ClassInitializationInfo)
at com.sun.jna.Native.findDirectMappedClass(Native.java:1281)
at com.sun.jna.Native.register(Native.java:1252)
at net.openhft.chronicle.hash.impl.util.jna.PosixFallocate.(PosixFallocate.java:18)
at com.oracle.svm.core.classinitialization.ClassInitializationInfo.invokeClassInitializer(ClassInitializationInfo.java:375)
at com.oracle.svm.core.classinitialization.ClassInitializationInfo.initialize(ClassInitializationInfo.java:295)
... 19 more

Command to reproduce (benchmark):
mx --env ni-ce benchmark renaissance-native-image:db-shootout -- --jvm=native-image --jvm-config=default-ce

Link to simple reproducer:
https://github.com/jovanstevanovic/DBShootoutException

@graalvmbot
Copy link
Collaborator

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.

vjovanov
vjovanov reviewed Jul 12, 2021
View reviewed changes
substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/trace/AccessAdvisor.java
@@ -58,6 +58,7 @@
internalCallerFilter.addOrGetChildren("com.sun.nio.zipfs.**", RuleNode.Inclusion.Exclude);
internalCallerFilter.addOrGetChildren("java.io.**", RuleNode.Inclusion.Exclude);
internalCallerFilter.addOrGetChildren("java.lang.**", RuleNode.Inclusion.Exclude);
internalCallerFilter.addOrGetChildren("java.lang.ClassLoader$NativeLibrary", RuleNode.Inclusion.Include);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment why.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

vjovanov
vjovanov reviewed Jul 12, 2021
View reviewed changes
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SecuritySubstitutions.java
@@ -218,7 +218,7 @@ private static Object replaceAccessControlContext(Object obj) {
@NeverInline("Starting a stack walk in the caller frame")
protected Class<?>[] getClassContext() {
final Pointer startSP = readCallerStackPointer();
return StackTraceUtils.getClassContext(1, startSP);
return StackTraceUtils.getClassContext(0, startSP);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Or why it was?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the reasons this benchmark didn't work was that stacktrace was not equal to the one on java, and it was not equal because we are skipping the first frame. (https://github.com/java-native-access/jna/blob/d367f0220be55b5638b3d324473ef0a7e654d83c/src/com/sun/jna/Native.java#L1516 in this line of code benchmark is accessing caller class and it was shifted by one in NI)

vjovanov
vjovanov reviewed Jul 12, 2021
View reviewed changes
substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/StackTraceTests.java Outdated
assertSame(StackTraceTests.class, classes[0]);
assertTrue(classes.length > 1);
assertSame(StackTraceTests.class, classes[1]);
assertTrue(classes.length > 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the same as in Java now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's the same. It can be simply tested by starting db-shootout benchmark (command is in PR description).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to run these JUnit tests on the HotSpot VM too as part of the gate runs.

Also testing more cases would be good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a small reproducer for this problem here: https://github.com/jovanstevanovic/DBShootoutException, to confirm the exact same behavior of my changes on both hotspot and ni.

Agree, we should expand this test.

@jovanstevanovic jovanstevanovic mentioned this pull request Jul 15, 2021
Closed
christianwimmer
christianwimmer suggested changes Jul 16, 2021
View reviewed changes
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/StackTraceUtils.java Outdated
@@ -123,7 +124,8 @@ public static boolean shouldShowFrame(FrameInfoQueryResult frameInfo, boolean sh

if (!showReflectFrames && ((clazz == java.lang.reflect.Method.class && "invoke".equals(frameInfo.getSourceMethodName())) ||
(clazz == java.lang.reflect.Constructor.class && "newInstance".equals(frameInfo.getSourceMethodName())) ||
(clazz == java.lang.Class.class && "newInstance".equals(frameInfo.getSourceMethodName())))) {
(clazz == java.lang.Class.class && "newInstance".equals(frameInfo.getSourceMethodName())) ||
(clazz == ClassInitializationInfo.class))) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think we can do it that way. What are the failing cases here? What does ClassInitializationInfo have to do with reflection?

What you probably want is to annotate ClassInitializationInfo with InternalVMMethod, so that our internal class initialization logic is ignored in most stack walk. But that needs careful testing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is what I'm trying to achieve, to ignore ClassInitializationInfo during stack walk. I'll try with InternalVMMethod annotation.

substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/StackTraceTests.java Outdated
assertSame(StackTraceTests.class, classes[0]);
assertTrue(classes.length > 1);
assertSame(StackTraceTests.class, classes[1]);
assertTrue(classes.length > 2);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to run these JUnit tests on the HotSpot VM too as part of the gate runs.

Also testing more cases would be good.

@jovanstevanovic jovanstevanovic force-pushed the fix-db-shootout branch 6 times, most recently from 898251b to 3059c86 Compare July 20, 2021 14:31
@jovanstevanovic jovanstevanovic deleted the fix-db-shootout branch August 25, 2021 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vjovanov vjovanov vjovanov left review comments

@christianwimmer christianwimmer christianwimmer requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jovanstevanovic @graalvmbot @vjovanov @christianwimmer