Refactor callbacks

[Spasi]

Like structs, native-to-Java callbacks look out-of-place in Java without special JVM support. Now that LWJGL 3 requires Java 8, we should rethink their design and implementation.

Current design

• Callback types are abstract classes and they extend a base abstract class (Callback.<T>).
• Callback types are represented by the corresponding abstract class in bindings (function parameters and struct members).
• SAM conversion is enabled with factory methods in the callback classes. There's a SAM interface in each class and a public static <CallbackClass> create(SAM sam) method.
• A JNI weak global reference is created that points to the callback instance and stored in the native callback object (as "user data").
• We check for premature GC when dereferencing the JNI weak global reference and display an appropriate error, without crashing.
• Callback instances must be freed, by calling the .free() method on the instance.

Pros

• Has worked without issues so far.
• Straightforward clean-up.
• Users are forced to store a strong reference to the callback instance. They don't have to RTFM, they figure it out on their own when callbacks start failing after the first GC. This then makes it relatively obvious that clean-up must take place.

Cons

• SAM conversion does not work with abstract classes, only with functional interfaces.
• There's both a class and an interface (SAM) for each callback type.
• Callback class names are arbitrary; they are not present as a type in the native API.
• The user is forced to store a strong reference to the callback instance.

Recent changes

• LWJGL 3 now requires Java 8. We can use static and default methods in interfaces.
• libffi has been replaced by dyncall. One important difference with dyncall is that the thunk pointer and the callback object handle are guaranteed to be one and the same. This means that we only need a single pointer to reference the entire (native and Java) callback structure.

Proposed design

• Each callback type is a functional interface.
• Callback argument decoding can happen directly in the interface, with a default method.
• The Callback class becomes a utility class that only creates and destroys callback objects.
• A callback object is a plain long value (opaque handle). Callback function arguments and member fields become long.
• There are method overloads that take a functional interface, create a callback object from it and pass it to the native function/struct.
• Callback type instances are stored in strong JNI global references.
  • Note that non-capturing lambdas and method references are never GCed anyway.
• The user may or may not store the callback object in Java code. In many bindings they don't have to store it, it's readily available via the API (e.g. glfwSet<Type>Callback function return the previously set callbacks).

Pros

• There's a single functional interface per callback type.
• One less indirection (no abstract class -> SAM delegation).
• Interface names won't show up in code that uses lambdas or method references.
• Callback object handles stored in user code will be simple long, not arbitrary types.
• Less callback objects stored in user code.
• No worries about premature GC.

Cons

• Memory leaks if the user doesn't perform clean-up. This is generally of low-concern, most applications will use a low-number of callbacks without much (if any) state.
• Non-obvious clean-up:
  • Must retrieve callback object handle using the appropriate binding API (e.g. glGetPointer(GL_DEBUG_CALLBACK_FUNCTION)), if the handle was not already stored in user code.
  • Must use Callback.free(handle) (vs cbInstance.free() currently).

Example 1, similar to current design:

private long keyCB; // callback handle is a long instead of an object
// ...
// callback setup, must pass lambda or method reference to the static create method
glfwSetKeyCallback(window, keyCB = GLFWKeyCallback.create((windowHandle, key, scancode, action, mods) -> {
   // ...
 }));
// cleanup
Callback.free(keyCB);

Example 2, without storing the callback handle:

// callback setup, lambda or method reference passed directly to the API
glfwSetKeyCallback(window, (windowHandle, key, scancode, action, mods) -> {
   // ...
 });
// cleanup
Callback.free(glfwSetKeyCallback(window, null));

[ShadowLordAlpha]

I would prefer it be in MemoryUtil instead of Callback as its the class that already does most of the memory manipulation stuff and from what it looks like most of the Callback classes use will be internal except for the free() method. Also I would like it if the callbacks return the object instead of a pointer though this is mostly because Java works with objects and not pointers for the most part though I would be fine ether way. Though I would very much like this change as my code already does something similar to your second example though looks less nice because of the way lambdas have to be used

[Spasi]

I would prefer it be in MemoryUtil instead of Callback as its the class that already does most of the memory manipulation stuff and from what it looks like most of the Callback classes use will be internal except for the free() method.

A fine idea, thanks!

Also I would like it if the callbacks return the object instead of a pointer though this is mostly because Java works with objects and not pointers for the most part though I would be fine ether way.

This is something I'd like to avoid:

• There's no need for a wrapper object. It would also be an extra indirection in the native-to-Java callback path.
• I'd like to minimize LWJGL-specific abstractions/utilities visible in user code (in the spirit of making LWJGL code look very similar to the corresponding C code). Note that in example 1 above there's only one mention of GLFWKeyCallback and no mention at all in example 2.
• I've written a lot of GLFW samples and always find it annoying having to remember callback type names. Auto-complete helps in the glfwSet methods, but not when writing the fields where the callback instances are stored.

[ShadowLordAlpha]

I am probably misunderstanding what you are trying to say so correct me if I get something wrong but

There's no need for a wrapper object. It would also be an extra indirection in the native-to-Java callback path.

No need for one as its fully legal to store it and return it latter as the Interface class without wrapping it in another object. You can even wrap other objects in lambda methods the same way you would wrap lambda methods in full classes.

I'd like to minimize LWJGL-specific abstractions/utilities visible in user code (in the spirit of making LWJGL code look very similar to the corresponding C code). Note that in example 1 above there's only one mention of GLFWKeyCallback and no mention at all in example 2.

The return type would simply change from a long to the callback interface. Nothing else would need to change as it would still be a Functional interface. It would also allow for the extra overload methods that take the long (though I suppose the object type methods would be the helper methods) to be removed and not seen from the user code.

I've written a lot of GLFW samples and always find it annoying having to remember callback type names. Auto-complete helps in the glfwSet methods, but not when writing the fields where the callback instances are stored.

I don't know how garbage collection works with lambda functions as I have not really looked that closely at it however you said that currently you use a weak reference to an object to store it. You could do something similar for the lambda functions if needed without the weak reference part though I don't really know why lwjgl was storing that in the first pace instead of a normal reference unless memory was a concern for some reason in which case it should have been a soft reference not a weak one.

[Spasi]

No need for one as its fully legal to store it and return it latter as the Interface class without wrapping it in another object. You can even wrap other objects in lambda methods the same way you would wrap lambda methods in full classes.

There are two pieces of state:

• The interface implementation. Can be an anonymous class instance, a lambda, or a method reference.
• The native callback object (currently implemented with dyncall).

The native callback object already contains a (JNI global) reference to the interface implementation. The interface implementation does NOT know anything about the native callback object (it can't know anything if we want lambdas to work). If we want an object return value, the only option is to wrap the native callback object handle in a class. What would be the point of that?

[ShadowLordAlpha]

Its supposed to return the previously set callback or null so I was suggesting a way to possibly do that (a map or something with the long value as the key and the function as the value) in object form but it should be fine ether way if nothing other than the long is needed to reset the function to what it was before if it is changed

[Spasi]

Sorry, I'm probably not explaining things clearly. Lets try again.

The dyncall callback object (DCCallback*) is a struct. The long value you get from <CallbackInterface>.create(...) is an opaque pointer to that struct. The first member of that struct is the thunk, which is basically the function pointer passed to a native API that expects a callback pointer. Since it's the first member in the struct, the DCCallback* pointer is also the function pointer. You have both using a single long value.

The only thing missing now is the Java callback (the instance that implements the callback interface). This is also stored in the callback object struct, in the userdata field. Since the struct is opaque, dyncall has a function for retrieving the userdata field: dcbGetUserData(DCCallback*). So you can do this:

long keyCB = GLFWKeyCallback.create((windowHandle, key, scancode, action, mods) -> {
   // ...
 });

GLFWKeyCallback javaCallback = memGlobalRefToObject(dcbGetUserData(keyCB));
// or simply
GLFWKeyCallback javaCallback = Callback.get(keyCB);

[ShadowLordAlpha]

I still think it would be more like java to return the actual callback (return memGlobalRefToObject(dcbGetUserData(keyCB)) instead of just the long value) though as long as its easy enough to reach the java object it would be fine and getting the long would probably also save some calls to recreate or get the object pointer later if it is reset as the callback.

Mostly I was thinking of libraries that may chose the way of simply overriding the current callback with their own and calling the former callback after they have finished processing the data. Not the best way of doing things but far easier than some other ways. Though again as long as the java callback can be easily reached its fine.

[dustContributor]

I have no issue with opaque pointers. It'd be really straightforward to make wrapper "create" that call Callback.get(cb) inside if needed.

Also not a lot of code would actually call these "GLFWKeyCallback.create" anyway, so making it slightly more verbose for the cases where you do want to retrieve the callback object shouldn't have much impact, if at all.

[Spasi]

After implementing the proposed design above and changing all sample code to use it, I ended up not liking it very much. Specifically, I didn't like having to write memFreeCallback(cbHandle); in clean-up code. After a few more experiments, the final design is a bit weird, but basically what @ShadowLordAlpha requested. If someone reviews the current implementation and has better ideas, please don't hesitate to post here or open a new issue.

edit: obsolete, see below

[octylFractal]

The change you made makes it impossible to implement multiple callbacks now, since the shared callback(long) and set(long) defaults conflicts among the different callbacks. Is it just not advised to directly implement more than one callback?

[Spasi]

@kenzierocks Could you share an example please? You're talking about multiple callbacks with compatible function signatures, right?

[Spasi]

The previous solution was too confusing and error prone. After further discussion a new approach has been implemented that is compatible with existing code, supports all code styles and has no clean-up surprises.

Final design

Example 1 (anonymous class):

private GLFWKeyCallback keyCB;
// callback setup
glfwSetKeyCallback(window, keyCB = new GLFWKeyCallback() {
    @Override
    public void invoke(long window, int key, int scancode, int action, int mods) {
        // ...
    }
});
// cleanup
keyCB.free();

Example 2 (indirect lambda):

private GLFWKeyCallback keyCB;
// callback setup
glfwSetKeyCallback(window, keyCB = GLFWKeyCallback.create((windowHnd, key, scancode, action, mods) -> {
    // ...
}));
// cleanup
keyCB.free();

Example 3 (direct lambda):

// callback setup
glfwSetKeyCallback(window, (windowHnd, key, scancode, action, mods) -> {
    // ...
});
// cleanup
glfwSetKeyCallback(window, null).free();

Implementation

• Each callback type now generates an interface and an abstract class. For example, the GLFW callback type GLFWkeyfun generates the GLFWKeyCallbackI interface and the GLFWKeyCallback abstract class.
• Callback types as function or struct setter parameters are mapped to the corresponding interface.
• Callback types as function or struct getter return values are mapped to the corresponding abstract class.
• With the new class hierarchy, there is no free() method in callback interfaces, only in abstract classes. This should make it obvious that clean-up requires a stateful instance and that interfaces are there to enable input lambdas.

[octylFractal]

This is what I used to do @Spasi, extending the SAM interface. This might not be supported, would you recommend I do something like this instead?

[Spasi]

Method references are perfect for this case. See MouseHelp with method references.

Also, don't forget to clean up your callbacks. LWJGL provides a convenient method for freeing GLFW callbacks: org.lwjgl.glfw.Callbacks.glfwFreeCallbacks(long window)

[octylFractal]

OK, I'll do that instead. Thanks!