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.

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

Premature GC of Pointer object when invoking getWideString #664

Closed
rprichard opened this issue May 29, 2016 · 25 comments
Closed

Premature GC of Pointer object when invoking getWideString #664

rprichard opened this issue May 29, 2016 · 25 comments
Labels

Comments

@rprichard
Copy link

(Copied from jna-users forum post on 2015-12-23, https://groups.google.com/forum/#!topic/jna-users/dCPnztnQnRw.)


I compiled JNA myself, and when I ran the test suite, the testLongStringGeneration(com.sun.jna.NativeTest) test sometimes failed with a "Invalid memory access" error indicating a JVM segfault. Here's what it looks like running JUnit from the command-line:

C:\rprichard\work\jna>c:\java\jdk1.8.0_60\bin\java -cp "build/jna.jar;build/jna-test.jar;lib/junit.jar;lib/hamcrest-core-1.3.jar" org.junit.runner.JUnitCore com.sun.jna.NativeTest
JUnit version 4.11
............E..E........JNA Warning: Encoding 'unsupported' is unsupported
JNA Warning: Encoding with fallback Cp1252
.......E
Time: 0.374
There were 3 failures:
1) testSynchronizedAccess(com.sun.jna.NativeTest)
java.lang.UnsatisfiedLinkError: Unable to load library 'testlib': Native library (win32-x86/testlib.dll) not found in re
source path ([file:/C:/rprichard/work/jna/build/jna.jar, file:/C:/rprichard/work/jna/build/jna-test.jar, file:/C:/rprich
ard/work/jna/lib/junit.jar, file:/C:/rprichard/work/jna/lib/hamcrest-core-1.3.jar])

... elided ... (test setup issue)

2) testOptionsInferenceFromInstanceField(com.sun.jna.NativeTest)
java.lang.UnsatisfiedLinkError: Unable to load library 'testlib': Native library (win32-x86/testlib.dll) not found in re
source path ([file:/C:/rprichard/work/jna/build/jna.jar, file:/C:/rprichard/work/jna/build/jna-test.jar, file:/C:/rprich
ard/work/jna/lib/junit.jar, file:/C:/rprichard/work/jna/lib/hamcrest-core-1.3.jar])

... elided ... (test setup issue)

3) testLongStringGeneration(com.sun.jna.NativeTest)
java.lang.Error: Invalid memory access
        at com.sun.jna.Native.getWideString(Native Method)
        at com.sun.jna.Pointer.getWideString(Pointer.java:696)
        at com.sun.jna.Memory.getWideString(Memory.java:561)
        at com.sun.jna.NativeTest.testLongStringGeneration(NativeTest.java:77)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at junit.framework.TestCase.runTest(TestCase.java:176)
        at junit.framework.TestCase.runBare(TestCase.java:141)
        at junit.framework.TestResult$1.protect(TestResult.java:122)
        at junit.framework.TestResult.runProtected(TestResult.java:142)
        at junit.framework.TestResult.run(TestResult.java:125)
        at junit.framework.TestCase.run(TestCase.java:129)
        at junit.framework.TestSuite.runTest(TestSuite.java:255)
        at junit.framework.TestSuite.run(TestSuite.java:250)
        at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:84)
        at org.junit.runners.Suite.runChild(Suite.java:127)
        at org.junit.runners.Suite.runChild(Suite.java:26)
        at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
        at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
        at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:138)
        at org.junit.runner.JUnitCore.run(JUnitCore.java:117)
        at org.junit.runner.JUnitCore.runMain(JUnitCore.java:96)
        at org.junit.runner.JUnitCore.runMainAndExit(JUnitCore.java:47)
        at org.junit.runner.JUnitCore.main(JUnitCore.java:40)

FAILURES!!!
Tests run: 29,  Failures: 3

I'm using 32-bit Windows 7 SP1, with jdk1.8.0_60.

I worked on the test a bit, and came up with this standalone test program that segfaults using the official JNA 4.2.1 JAR.

package com.example;
import com.sun.jna.Memory;
import com.sun.jna.Native;

public class JnaTest {
    public static void main(String[] args) {
        // This code below is reduced from a different test.  (I forget which.)
        new Memory(4);

        // The code below comes from NativeTest.testLongStringGeneration.
        StringBuilder buf = new StringBuilder();
        final int MAX = 2000000;
        for (int i=0;i < MAX;i++) {
            buf.append('a');
        }
        String s1 = buf.toString();
        Memory m = new Memory((MAX + 1)*Native.WCHAR_SIZE);
        m.setWideString(0, s1);
        m.getWideString(0);
    }
}

C:\rprichard\proj\JnaTest>c:\Java\jdk1.8.0_60\bin\java.exe -cp build\classes\main;lib\jna-4.2.1.jar com.example.JnaTest
Exception in thread "main" java.lang.Error: Invalid memory access
        at com.sun.jna.Native.getWideString(Native Method)
        at com.sun.jna.Pointer.getWideString(Pointer.java:696)
        at com.sun.jna.Memory.getWideString(Memory.java:561)
        at com.example.JnaTest.main(JnaTest.java:19)

With lots of printf debugging, I think I identified the cause. The final m.getWideString call ultimately calls Java_com_sun_jna_Native_getWideString. Once the JVM reaches this native function, there are no live references remaining to the Memory object. newJavaString calls back into the JVM, ((*env)->NewString), to create the 2MB String object, at which point the JVM finalizes the Memory object, which frees the buffer. The JVM then attempts to read from the freed buffer and segfaults.

@rprichard
Copy link
Author

Writing m to a static variable prevents the crash:

static Memory preserve;
...
    Memory m = new Memory((MAX + 1)*Native.WCHAR_SIZE);
    preserve = m;
...

The reduced test case still fails today. (I tested with Win7 32-bit, Java 1.8.0_92, and JNA 4.2.2.)

It isn't obvious to me how much freedom the JVM has in deciding when to finalize an object. (i.e. I couldn't tell whether the JVM was behaving correctly in this case.)

@matthiasblaesing
Copy link
Member

At this point I'd agree with @rprichard I ran some tests and was only able to reproduce it with the 32 bit VM on windows. It looks indeed like a GC issue, as running it with the "-Xms1G" option (effectively preventing GC), the problem is not reproduced.

The problem is also not reproduced with the 64bit VM, but the 64bit version runs with a different VM configuration (server), which most probably does less aggressive GC.

A slightly modified version gives better error information:

        Native.setProtected(false);

        // This code below is reduced from a different test.  (I forget which.)
        new Memory(4);

        // The code below comes from NativeTest.testLongStringGeneration.
        StringBuilder buf = new StringBuilder();
        final int MAX = 2000000;
        for (int i=0;i < MAX;i++) {
            buf.append('a');
        }
        String s1 = buf.toString();
        Memory m = new Memory((MAX + 1)*Native.WCHAR_SIZE);
        m.setWideString(0, s1);
        m.getWideString(0);

This results in the following hs_err.log:

https://gist.github.com/matthiasblaesing/efab6ce5f744c2146484adfab7010f2b

@matthiasblaesing
Copy link
Member

Another update: I replace Memory with a subclass reporting finalization:

public class MemoryWithFinalizer extends Memory {
    public MemoryWithFinalizer(long size) {
        super(size);
    }

    public MemoryWithFinalizer() {
    }

    @Override
    protected void finalize() {
        System.out.println("FINALIZER");
        super.finalize();
    } 
}

While I did not expect clear results, I got them. With the sample from #664 (comment) I get this (both finalizers, the one of the first dummy allocation and the real allocation for the string are run):

FINALIZER
FINALIZER
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x6f8a55f0, pid=1708, tid=0x000007f0
#
# JRE version: Java(TM) SE Runtime Environment (8.0_92-b14) (build 1.8.0_92-b14)
# Java VM: Java HotSpot(TM) Client VM (25.92-b14 mixed mode windows-x86 )
# Problematic frame:
# V  [jvm.dll+0x455f0]
#
# Failed to write core dump. Minidumps are not enabled by default on client versions of Windows
#
# An error report file with more information is saved as:
# E:\src\testsegfault\hs_err_pid1708.log
#
# If you would like to submit a bug report, please visit:
#   http://bugreport.java.com/bugreport/crash.jsp
#

rprichard added a commit to rprichard/pty4j that referenced this issue May 31, 2016
That finalizer is known to prematurely free memory, in at least one
case[1].  I noticed a non-reproducible "Invalid memory error" from a call
to winpty_open that I speculate is a similar problem.

[1] java-native-access/jna#664
@matthiasblaesing matthiasblaesing changed the title JVM segfault with testLongStringGeneration(com.sun.jna.NativeTest) test case Premature GC of Pointer object when invoking getWideString Jul 1, 2016
@matthiasblaesing
Copy link
Member

Ok - so the GC is more aggressive, than is good for us, so we need a strategy to prevent GC of the Pointer object while the Native#getWideString method is running. Some options I see:

  1. In Pointer class wrap all occurrences of calls into the native methods taking a raw pointer value into a try-finally wrapper, that adds a static strong ref before the call and releases that ref after the call. (drawback: synchronization overhead for each call)
  2. Again in pointer call wrap all calls into synchronized(this) blocks - as the monitor must be held for the duration of the call, this should GC (drawback: this is not reentrant, a callback from native code into java originating from another thread would dead-lock + synchronization overhead)
  3. Modify the Native methods to take a Pointer class instead of a raw long value. Now the native call is the element that prevents garbage collection, as the whole object is passed into the method. The native methods are package private, so this should be save.

@twall I'd vote for variant 3 - would you agree?

@twall
Copy link
Contributor

twall commented Jul 5, 2016

As long as it can be done without adversely affecting Direct mode performance.

The field extraction (peer pointer value) was originally moved out of native to improve performance (avoiding vm interactions from native code). Native code already supports extracting the "peer" field from a Pointer object; the main issue is that you don't want to require that since it's additional "talk to the VM" overhead within the native code.

Variant 1 might work without a static call; you just need something in the try/finally to trick the VM into keeping the this reference around. Maybe even just an internal field update after the native call might work.

I don't want to bend over too far backward without understanding the rationale going on within the VM that says it's OK to GC an object when you're still in the middle of executing one of that object's member functions. That to me seems wrong.

@rprichard
Copy link
Author

I don't want to bend over too far backward without understanding the rationale going on within the VM that says it's OK to GC an object when you're still in the middle of executing one of that object's member functions. That to me seems wrong.

I ran across a presentation on Java finalization by Hans Boehm (dated 2004?), http://www.hboehm.info/misc_slides/java_finalizers.pdf. Slide 31 is interesting:

  • Finalizer may begin executing as soon as object is no longer reachable, i.e. GC could otherwise collect it.
  • This may happen earlier than you think.
    • In last call to CleanResource.doSomething(), this pointer is last accessed to retrieve myIndex.
    • myIndex is final, can be read early.
    • Register containing this may be reused at that point, making CleanResource instance no longer reachable.
    • Resource.recycle() may run while Resource.doSomething() is still running. Oops.

Fixes are provided on slide 34, which include volatile fields/statics or use of locking. On the next slide, Boehm chooses to use locks, but I wonder if JNA would prefer to avoid locking for better performance. In any case, the locking is only used to prevent premature finalization, so there's no deadlock concern.

He also created an earlier slide deck that might be worth examining, http://www.hboehm.info/popl03/slides.pdf. Two slides mention a premature finalization problem:

  1. Myth 2: Finalizers run only after all other method calls on the object have completed.
  2. Late finalization is necessary, but early finalization may be a problem:

From the second slide:

  • Object is finalized when the collector discovers it to be unreachable.
  • One of its fields may still be in a register.
  • If that field is a handle / file descriptor:
    • finalizer may close it while being accessed.
    • or not?

@twall
Copy link
Contributor

twall commented Jul 5, 2016

Would applying a fix to all JNA's Pointer-accepting methods even completely
eliminate the issue? It'd still be possible for a user to allocate
memory and call a mapped function (which accepts a pointer) and you'd have
the same potential problem of premature GC (leaving the native function
trying to access freed memory).

Maybe it's better to document the issue and make clear what some options
are for mitigation.

On Tue, Jul 5, 2016 at 2:51 PM, Ryan Prichard notifications@github.com
wrote:

I don't want to bend over too far backward without understanding the
rationale going on within the VM that says it's OK to GC an object when
you're still in the middle of executing one of that object's member
functions. That to me seems wrong.

I ran across a presentation on Java finalization by Hans Boehm (dated
2004?), http://www.hboehm.info/misc_slides/java_finalizers.pdf. Slide 31
is interesting:

  • Finalizer may begin executing as soon as object is no longer
    reachable, i.e. GC could otherwise collect it.
  • This may happen earlier than you think.
    • In last call to CleanResource.doSomething(), this pointer is last
      accessed to retrieve myIndex.
    • myIndex is final, can be read early.
    • Register containing this may be reused at that point, making
      CleanResource instance no longer reachable.
    • Resource.recycle() may run while Resource.doSomething() is still
      running. Oops.

Fixes are provided on slide 34, which include volatile fields/statics or
use of locking. On the next slide, Boehm chooses to use locks, but I wonder
if JNA would prefer to avoid locking for better performance. In any case,
the locking is only used to prevent premature finalization, so there's no
deadlock concern.

He also created an earlier slide deck that might be worth examining,
http://www.hboehm.info/popl03/slides.pdf. Two slides mention a premature
finalization problem:

  1. Myth 2: Finalizers run only after all other method calls on the object
    have completed.
  2. Late finalization is necessary, but early finalization may be a
    problem:

From the second slide:

  • Object is finalized when the collector discovers it to be
    unreachable.
  • One of its fields may still be in a register.
  • If that field is a handle / file descriptor:
    • finalizer may close it while being accessed.
    • or not?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#664 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAuMrTvSYxTPcFLWwTQGCNO1rAgPZOSUks5qSqetgaJpZM4IpLri
.

@matthiasblaesing
Copy link
Member

I decided to check performance and implemented to variants, that both should fix the primary problem (premature GC in method call of Memory class). The two testbranches (see below) intend to stabilize the method calls in Pointer.

Testvariants:

T1 => only PrematureGCTest added to reproduce the problem, used as reference reference
https://github.com/matthiasblaesing/jna/tree/test_gc
T2 => Native modified to take Pointer/Function
https://github.com/matthiasblaesing/jna/tree/pass_pointer_to_native
T3 => Implement Boehm method (synchronized keepAlive method + synchronized block in finalizer
https://github.com/matthiasblaesing/jna/tree/prevent_gc_synchronized

Systems:

S1 => Windows 10 64Bit, VirtualBox, Oracle JDK 8u92 64 Bit
S2 => Windows 10 64Bit, VirtualBox, Oracle JDK 8u92 32 Bit
S3 => Ubuntu 16.04 (Linux 4.4.0) 64 Bit, OpenJDK 8 (custom build, 2016-05-05)
S4 => Ubuntu 16.04 (Linux 4.4.0) 64 Bit, Zulu 7u101

Testing:

3 Invokations of PerformanceTest without measurement
10 Invokations of PerformanceTest with measurement

Raw data:

All combinations of Systems + Testvariants were run. The numbers are provided in an OpenDocument table file:

http://www.doppel-helix.eu/Analyse.ods

Observations:

  • Both T2 + T3 fix the reported problem (premature free of memory by GC)
  • no clear winner in terms of performance - most differences in runtime are well within error margin

At this point I'd go with the pass_pointer_to_native branch. From my perspective this looks like the most stable approach. Once an object as passed the barrier into native called (even if that code calls back into java at some point) from the GC perspective the object can't be collected, as the native code is black box it can't reason about.

@matthiasblaesing
Copy link
Member

Another option (basicly picking up @twall 's suggestion: Make the user protect its objects. It is asked many times and it looks manageable (complete sample, see end of this message).

The implementation of GCProtection is compatible with java 1.5 (according to java doc, Closeable is present since then), the usage in try-with-resource is compatible 1.7.

package com.sun.jna;

import java.io.Closeable;
import java.util.IdentityHashMap;

public class PrematureGCTest {

    public static void main(String[] args) {
        Native.setProtected(false);

        // This code below is reduced from a different test.  (I forget which.)
        new MemoryWithFinalizer(4);

        for (int MAX = 50; MAX < 2000000; MAX += 100) {
            // The code below comes from NativeTest.testLongStringGeneration.
            StringBuilder buf = new StringBuilder();
            for (int i = 0; i < MAX; i++) {
                buf.append('a');
            }
            String s1 = buf.toString();
            Memory m = new MemoryWithFinalizer((MAX + 1) * Native.WCHAR_SIZE);
            try (GCProtection gcp = new com.sun.jna.GCProtection(m)) {
                m.setWideString(0, s1);
                m.getWideString(0);
            }
        }
        System.out.println("OK");
    }
}

class MemoryWithFinalizer extends Memory {

    public MemoryWithFinalizer(long size) {
        super(size);
    }

    public MemoryWithFinalizer() {
    }

    @Override
    protected void finalize() {
        System.out.println("FINALIZER");
        super.finalize();
    }
}

class GCProtection implements Closeable {
    private IdentityHashMap<Object, Object> holder = new IdentityHashMap<Object, Object>();

    public GCProtection(Object... objects) {
        for(Object o: objects) {
            holder.put(o, Boolean.TRUE);
        }
    }

    public synchronized void keepAlive() {
        // synchronize to prevent GC
    }

    @Override
    protected void finalize() throws Throwable {
        synchronized (this) {};
        super.finalize();
    }

    @Override
    public void close() {
        this.keepAlive();
    }
}

@twall
Copy link
Contributor

twall commented Jul 19, 2016

That looks reasonable.

On Sat, Jul 16, 2016 at 1:12 PM, matthiasblaesing notifications@github.com
wrote:

Another option (basicly picking up @twall https://github.com/twall 's
suggestion: Make the user protect its objects. It is asked many times and
it looks manageable (complete sample, see end of this message).

The implementation of GCProtection is compatible with java 1.5 (according
to java doc, Closeable is present since then), the usage in
try-with-resource is compatible 1.7.

package com.sun.jna;
import java.io.Closeable;import java.util.IdentityHashMap;
public class PrematureGCTest {

public static void main(String[] args) {
    Native.setProtected(false);

    // This code below is reduced from a different test.  (I forget which.)
    new MemoryWithFinalizer(4);

    for (int MAX = 50; MAX < 2000000; MAX += 100) {
        // The code below comes from NativeTest.testLongStringGeneration.
        StringBuilder buf = new StringBuilder();
        for (int i = 0; i < MAX; i++) {
            buf.append('a');
        }
        String s1 = buf.toString();
        Memory m = new MemoryWithFinalizer((MAX + 1) * Native.WCHAR_SIZE);
        try (GCProtection gcp = new com.sun.jna.GCProtection(m)) {
            m.setWideString(0, s1);
            m.getWideString(0);
        }
    }
    System.out.println("OK");
}

}
class MemoryWithFinalizer extends Memory {

public MemoryWithFinalizer(long size) {
    super(size);
}

public MemoryWithFinalizer() {
}

@Override
protected void finalize() {
    System.out.println("FINALIZER");
    super.finalize();
}

}
class GCProtection implements Closeable {
private IdentityHashMap<Object, Object> holder = new IdentityHashMap<Object, Object>();

public GCProtection(Object... objects) {
    for(Object o: objects) {
        holder.put(o, Boolean.TRUE);
    }
}

public synchronized void keepAlive() {
    // synchronize to prevent GC
}

@Override
protected void finalize() throws Throwable {
    synchronized (this) {};
    super.finalize();
}

@Override
public void close() {
    this.keepAlive();
}

}


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#664 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrcCygVNHeDXB2v-Q-1Zf6S_EpqVZks5qWRDsgaJpZM4IpLri
.

@matthiasblaesing
Copy link
Member

I'll pull back that last suggestion. I had some time to think about this and the longer I think about it, there surer I get that the Pointer object indeed should be passed to native.

@twall please have a look at my sample implementation:
https://github.com/matthiasblaesing/jna/commits/pass_pointer_to_native

The helper class proposed in my last comment would work, but is an ugly work-around the real problem. That solution would add much noise, that makes it harder to get the code right and to read it. While the underlying problem is in the bindings.

Are there numbers, that show the high overhead for the C->Java calls?

@twall
Copy link
Contributor

twall commented Aug 30, 2016

The thing to test for performance would be the field access on the Pointer
object (this is probably already documented somewhere in JVM maker's info).

The other concern would be ensuring you can still make direct mode calls,
but I don't think that's an issue here. Direct mode wants all parameters
to be in their final native form so that the stack may be passed more or
less directly as-is from JNI to the native function.

On Mon, Aug 29, 2016 at 3:07 PM, matthiasblaesing notifications@github.com
wrote:

I'll pull back that last suggestion. I had some time to think about this
and the longer I think about it, there surer I get that the Pointer object
indeed should be passed to native.

@twall https://github.com/twall please have a look at my sample
implementation:
https://github.com/matthiasblaesing/jna/commits/pass_pointer_to_native

The helper class proposed in my last comment would work, but is an ugly
work-around the real problem. That solution would add much noise, that
makes it harder to get the code right and to read it. While the underlying
problem is in the bindings.

Are there numbers, that show the high overhead for the C->Java calls?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#664 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrZCs4JWyMP9it_DG4Z7pr1HLEABkks5qky4GgaJpZM4IpLri
.

@matthiasblaesing
Copy link
Member

Ok - some more thought down and measuring done. Please see the complete testset here:

All data: http://www.doppel-helix.eu/Pointer_to_Native_Comparison.7z
Results: http://www.doppel-helix.eu/Pointer_to_Native_Comparison.ods

In the comparison:

pointer_to_native was the first implementation - this implementation passes the pointer object instead of the native pointer value to the native side. The drawback is, that a call C->Java is needed to access the field value of the pointer.

pointer_native_direct modifies the first approach by passing the pointer and its native value to the C side. While potentially causing longer call times (more arguments need to be marshalled), it removes the necessaty to callback from C to java (in the common case).

My interpretation of the results:

  • the callback overhead java->C is pretty big. if you only take the cases that can be handled purely on the C side (testInt*) the perfomance sinks to ~70% relative to the unmodified source.
  • the overhead for the callback java->C is small enough to be unnoticible if other JVM actions are required (see the String tests)
  • considering the error margins pointer_native_direct and baseline are equal looking only at the speed

The unittests are unaffected by the changes:

  • linux-x64: All tests pass
  • windows-x64: 2 tests fail (before and after: DirectTypeMapperTest and NativeTest#testSizeOf (bool should be 1, is 4)
  • windows-x86: 2 tests fails (one less that before the change (testLongStringGeneration), other errors see x64)

From my perspective this looks good. If you agree I'd polish the pointer_native_direct branch, add documentation why the pointer object itself is passed to native (to prevent regression in the future) and that should do the trick.

@twall
Copy link
Contributor

twall commented Sep 1, 2016

That's a pretty significant API change (would require a version bump) for
what would otherwise seem to be a problem with fairly narrow scope. While
the approach makes sense in theory, I'm worried about the scope of changes.

On Wed, Aug 31, 2016 at 3:35 PM, matthiasblaesing notifications@github.com
wrote:

Ok - some more thought down and measuring done. Please see the complete
testset here:

All data: http://www.doppel-helix.eu/Pointer_to_Native_Comparison.7z
Results: http://www.doppel-helix.eu/Pointer_to_Native_Comparison.ods

In the comparison:

pointer_to_native was the first implementation - this implementation
passes the pointer object instead of the native pointer value to the native
side. The drawback is, that a call C->Java is needed to access the field
value of the pointer.

pointer_native_direct modifies the first approach by passing the pointer
and its native value to the C side. While potentially causing longer call
times (more arguments need to be marshalled), it removes the necessaty to
callback from C to java (in the common case).

My interpretation of the results:

  • the callback overhead java->C is pretty big. if you only take the
    cases that can be handled purely on the C side (testInt*) the perfomance
    sinks to ~70% relative to the unmodified source.
  • the overhead for the callback java->C is small enough to be
    unnoticible if other JVM actions are required (see the String tests)
  • considering the error margins pointer_native_direct and baseline are
    equal looking only at the speed

The unittests are unaffected by the changes:

  • linux-x64: All tests pass
  • windows-x64: 2 tests fail (before and after: DirectTypeMapperTest
    and NativeTest#testSizeOf (bool should be 1, is 4)
  • windows-x86: 2 tests fails (one less that before the change
    (testLongStringGeneration), other errors see x64)

From my perspective this looks good. If you agree I'd polish the
pointer_native_direct branch, add documentation why the pointer object
itself is passed to native (to prevent regression in the future) and that
should do the trick.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#664 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrdYl1dMlQbKyffejOEOepoY93HzBks5qldeRgaJpZM4IpLri
.

@matthiasblaesing
Copy link
Member

Don't take this wrong: But what do you suggest as fix? I came up with different approaches and in the end it comes down to the fact, that JNA tries to play hide-and-seek with the garbadge collector. Maybe new developments in the JDK (project panama) will offer better control over the java<->native interaction, but as far a I see it, the only real option is to make the GC aware of the fact, that references to the Pointer objects still exists when calling into native.

My reading is, that it is just luck, that the problem is not visible with the primitive types: Assume you have a Memory object and the last call you will make to this object is an integer read (Memory#getInt), now you call Memory#getInt, this delegates to Pointer#getInt and that in turn calls into native code, before it does so it resolves the target address for the read.

This could result in a segfault:

  • Enter Pointer#getInt
  • Calculate the target address
  • [GC runs now and the Pointer#dispose]
  • call Native#getInt

From the GCs POV the Pointer object is not needed anymore and so the finalizer can run. If we are unlucky the finalizer finishes before the call to Native#getInt finishes.

With respect to the API break: I see one method that breaks and that is Native#getDirectByteBuffer. I'll revert the change to that method. All other methods have default visiblity and so are only visible in the same package. If someone got there he/she is already way out of line.

With regard to versioning: I changed the JNA_JNI_VERSION - whether this is minor or major can of course be discussed. You already bumped to 5.0.0 in january in that timeframe no release was done (4.2.2 was branched of before that) and I bumped it to 5.1.0.

With respect to the scope of the changes: My take is, that it looks worse than it is. Yes there are many methods changed and yes that can introduce regressions, but in the end there are these groups:

  • Native#invoke*: All functions gained a Function object as first parameter
  • Native#read_, Native#indexOf, Native#write_, Native#get_, Native#set_: All functions gained a Pointer object as first parameter

@twall
Copy link
Contributor

twall commented Sep 1, 2016

Matthias,

I really appreciate the work you've put into this, and I, too, am
surprised that this GC problem has not been more widespread, although it's
easy to see how one VM's implementation of finalization execution might
inadvertently spread to others, resulting in a de facto empirical behavior.

I think you've come up with a reasonable accommodation of both reference
preservation and avoiding field lookup (although I'm surprised that the
field lookup would have heavy performance implications -- I wouldn't expect
it to actually require a C->Java transition but would rather be implemented
as an internal structure lookup wholly on the native side). If this truly
is not exposed in the public API, then this should be ok (i'd probably be
ok with it even if it did involve public API changes, just trying to
gauge the resulting impact).

Previously this wasn't an issue when Pointer#getInt was itself a native
method, but all of the native bits were consolidated into the Native class
in order to isolate GC issues around native linkage cleanup. The intent
was to tightly couple the JNI library loading/unloading with a single
class; we had errors where the JNI library would be unloaded and then
methods from some other class be called and wind up with a segfault or link
error.

Thanks
T.

On Thu, Sep 1, 2016 at 5:20 AM, matthiasblaesing notifications@github.com
wrote:

Don't take this wrong: But what do you suggest as fix? I came up with
different approaches and in the end it comes down to the fact, that JNA
tries to play hide-and-seek with the garbadge collector. Maybe new
developments in the JDK (project panama) will offer better control over the
java<->native interaction, but as far a I see it, the only real option is
to make the GC aware of the fact, that references to the Pointer objects
still exists when calling into native.

My reading is, that it is just luck, that the problem is not visible with
the primitive types: Assume you have a Memory object and the last call you
will make to this object is an integer read (Memory#getInt), now you call
Memory#getInt, this delegates to Pointer#getInt and that in turn calls into
native code, before it does so it resolves the target address for the read.

This could result in a segfault:

  • Enter Pointer#getInt
  • Calculate the target address
  • [GC runs now and the Pointer#dispose]
  • call Native#getInt

From the GCs POV the Pointer object is not needed anymore and so the
finalizer can run. If we are unlucky the finalizer finishes before the call
to Native#getInt finishes.

With respect to the API break: I see one method that breaks and that is
Native#getDirectByteBuffer. I'll revert the change to that method. All
other methods have default visiblity and so are only visible in the same
package. If someone got there he/she is already way out of line.

With regard to versioning: I changed the JNA_JNI_VERSION - whether this is
minor or major can of course be discussed. You already bumped to 5.0.0 in
january in that timeframe no release was done (4.2.2 was branched of before
that) and I bumped it to 5.1.0.

With respect to the scope of the changes: My take is, that it looks worse
than it is. Yes there are many methods changed and yes that can introduce
regressions, but in the end there are these groups:

  • Native#invoke*: All functions gained a Function object as first
    parameter
  • Native#read_, Native#indexOf, Native#write_, Native#get_, Native#set_:
    All functions gained a Pointer object as first parameter


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#664 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAuMrTvNG1cj1lb9X8u7fKlGifxgysHJks5qlpj4gaJpZM4IpLri
.

@sijskes
Copy link

sijskes commented Sep 1, 2016

public String getWideString(long offset) {
String s = Native.getWideString(peer + offset);
Keeper.keep(this);
return s ;
}

public class Keeper
{
public static void keep( Object o )
{
}
}

An empty keep() might get optimized away.

in that case:

public class Keeper
{
public static Class last ;
public static void keep( Object o )
{
last = o.getClass();
}
}

when a public Class reference is objected to:

public class Keeper
{
public static int cnt ;
public static void keep( Object o )
{
cnt += o.hashCode();
}
}

@matthiasblaesing
Copy link
Member

@sijskes sorry this won't work or if it does it depends on implementation details. The moment the JVM decides to inline Keeper.keep it can optimize the o.getClass() away, as it can argue, that the this pointer reference can't change its class. As this method is short it is likely to get inlined, getClass itself is a native (I asume an intrisic) so I don't know whether or not hotspot will optimize this away (access to the class info is a structure lookup) but it is probable.

@sijskes
Copy link

sijskes commented Sep 1, 2016

@matthiasblaesing you mean that the ordering is changed? even if we change the 'last' to volatile? So on entry, the o.getClass() is done before the Native.get.. ??

@sijskes
Copy link

sijskes commented Sep 1, 2016

Is there are wrapper for a global reference?
ie:
jobject NewGlobalRef(JNIEnv *env, jobject obj);
void DeleteGlobalRef(JNIEnv *env, jobject globalRef);
then it can be used in
Ref r = new Ref(this)
try {
r.lock(); => NewGlobalRef
return Native.getWideString(peer + offset);
} finally {
r.unlock() => DeleteGlobalRef
}

@sijskes
Copy link

sijskes commented Sep 1, 2016

@matthiasblaesing re ordering, IMHO it depends on how you interpret
https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.3
o.getClass() could be a constant, in a inlined situation where there is a different compiled code for every derivation of Memory. as Memory is not final.

you are right in assuming o.getClass() as a const within the method, not sure if reordering is allowed here. but possibly because there is no memory access, and therefore consistent (maybe?).

@sijskes
Copy link

sijskes commented Sep 1, 2016

@matthiasblaesing btw, i vote for option 3 when Ref is to intensive. (passing this to jni domain)

@matthiasblaesing
Copy link
Member

With respect to the last comment: I don't see the point. The discussion is around the question whether or not this can be solved on the java level or if the solution has to be in the JNI layer. I don't get the intention.

With respect to ordering the JLS for Java SE 8 (section 12.6.2. Interaction with the Memory Model is relevant. An in fact it could be argued, that the store into a static field is enough to protect this:

An object B is definitely reachable at di from static fields if there exists a write w1 to a static field v of a class C such that the value written by w1 is a reference to B, the class C is loaded by a reachable classloader, and there does not exist a write w2 to v such that hb(w2, w1) is not true and both w1 and w2 come-before di.

To force the execution and establish the happens-before you'd need a synchronized block or a volatile write/read pair (without the read there is no order). This was tested above and the performance was worse than passing the whole Pointer object into the C layer and doing a read of the field from C. What is more the object itself needs to be held strong, not its class.

For the comparison:
http://www.doppel-helix.eu/Analyse.ods

  • "Reference" is the current state (no synchronization, raw pointer value is passed as long to native)
  • "Pointer" was my first try - passing the pointer to native and read the raw pointer value from the C side via a JNI call
  • "Boehm" was a second try, that placed a "synchonized(this){}" in a finally block after every relevant call to native (all methods in Pointer that call to a native method in the Native class)

I won't repeat my findings above the "Boehm" variant was nearly always slower than the "Pointer" variant. There is another drawback of the "Boehm" variant: as the synchronization is done on the object itself, this could introduce a deadlock, as other code could also synchronize on the same object.

The new numbers are a deeper look and show that in a microbenchmark the differences are much higher/more noticable. The idea behind passing the Pointer object and the pointer value into native code is:

  • its lock free (from the java side at least)
  • no native -> java overhead (the native pointer value is there)

My main concern is that the native parts need recompilation.

@matthiasblaesing
Copy link
Member

With respect to the vote: Which is option 3? There are three options on the table:

  • "The Boehm" method: hold the this object strongly with a try {} finally {synchronized(this){}} block (T3 in Premature GC of Pointer object when invoking getWideString #664 (comment))
  • "Pointer to native": Instead of the native pointer value pass the Pointer object itself to native and read the native pointer value from the field via JNI
  • "pointer_native_direct": This is a hybrid of "Pointer to native" and the current situation: The Pointer object is passed to native in addition to the raw pointer value. The Pointer is not used on the C side, passing it is only done to make the GC understand, that it must not collect it

"The Boehm" method is nice because it works fully on the java level - but then it cries for deadlocks.
"Pointer to native" works lock free and has the drawback having to use a JNI callback to read the field, "pointer_native_direct" has the benefits of "Pointer to native" and does not suffer from the performance for the JNI callback.

@sijskes
Copy link

sijskes commented Sep 1, 2016

@matthiasblaesing issues are a bad place to discuss things. my appologies.

I believe a Ref implementation with fore-mentioned globalref jni calls will keep the this reachable until execution of the finally block.

Otherwise the native part needs refactoring. Adding the this pointer to the call to the JNI will keep it reachable until return.

mstyura pushed a commit to mstyura/jna that referenced this issue Sep 9, 2024
java-native-access#664)

… life-time

Motivation:

We should not cache the local and remote address as these might change
during the life-time of the connection

Modifications:

- Override methods so we don't cache
- Also duplicate ByteBuffer in the QuicConnectionAddress to make things
safer.

Result:

Correctly return ids
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants