Pointer already mapped

[fpapai]

Regarding #326,

Changes are the following:

• map of pointer-to-callback is replaced with a map of pointer-to-array-of-callbacks to handle callbacks with different signatures mapped to the same native pointer.
• modified related unit test

Not sure if this was the best solution, I hope that you can give me feedback on it.
I did not benchmark it.

[matthiasblaesing]

Thank you. In general this looks sane to me. I would like to understand though, when the same pointer can sanely assigned to different Callback signatures?

It would be great if you could come up with a microbenchmark, that helps to understand the impact of the change.

[matthiasblaesing]

Can you explain why this logic is necessary? Shouldn't a direct addition work?

[fpapai]

You are right.
As in native-to-java callback using JNA practically it is not possible to create such situation.
The code can just create the array[1] with the reference.
I'll modify the code shortly.

[matthiasblaesing]

What do you think about switching the signature of this to:

private static void addCallback(Map<Pointer, Reference<Callback>[]> pointerCallbackMap, Pointer pointer, Callback cb)

[fpapai]

I'd rather not change it, as your proposal suggests that it will do more than now: to update pointerCallbackMap.
That update is in a synchronized block on that object. It is nice to see the related get / put in one location.
If all sync block is extracted as well, then there is an extra map-removal operation in that block that would not belong.
The code for reference:

...
        Map<Callback, CallbackReference> map = direct ? directCallbackMap : callbackMap;
        synchronized(pointerCallbackMap) {
            Reference<Callback>[] array = pointerCallbackMap.get(p);
            Callback cb = getTypeAssignableCallback(type, array);
            if (cb != null) {
                return cb;
            }
            cb = createCallback(type, p);
            pointerCallbackMap.put(p, addCallback(array, cb));

            // No CallbackReference for this callback
            map.remove(cb);
            return cb;
        }
...

However I'd find a better name for the current addCallback. Perhaps 'expandArrayWithCallback'.

[twall]

The original idea was that when you actually have native code calling a Java callback, bad things will happen if two different signatures are used, since a Java method expects well-specified types. Unfortunately, the original implementation didn't account for non-code placeholders, or otherwise special values that mean something other than a literal function address. Some code uses a generic function pointer or a constant value (void *(*))-1) in order to represent the notion of "use the default callback". When there are special values like that, it might make sense to somehow tag the values so they are recognizable by JNA when marshalling/unmarshalling.

…

On Sun, Nov 22, 2020 at 2:07 PM Matthias Bläsing ***@***.***> wrote: ***@***.**** commented on this pull request. Thank you. In general this looks sane to me. I would like to understand though, when the same pointer can sanely assigned to different Callback signatures? It would be great if you could come up with a microbenchmark, that helps to understand the impact of the change. ------------------------------ In src/com/sun/jna/CallbackReference.java <#1268 (comment)> : > @@ -451,7 +490,14 @@ private static Pointer getFunctionPointer(Callback cb, boolean direct) { if (cbref == null) { cbref = new CallbackReference(cb, callingConvention, direct); map.put(cb, cbref); - pointerCallbackMap.put(cbref.getTrampoline(), new WeakReference<Callback>(cb)); + + Reference<Callback>[] array = pointerCallbackMap.get(cbref.getTrampoline()); + Class<?> cbClass = findCallbackClass(cb.getClass()); + Callback existingCb = getTypeAssignableCallback(cbClass, array); + if (existingCb == null) { + pointerCallbackMap.put(cbref.getTrampoline(), addCallback(array, cb)); + } Can you explain why this logic is necessary? Shouldn't a direct addition work? ------------------------------ In src/com/sun/jna/CallbackReference.java <#1268 (comment)> : > return cb; } } + private static Callback getTypeAssignableCallback(Class<?> type, Reference<Callback>[] array) { + if (array != null) { + for (int i=0;i < array.length;i++) { + Callback cb = array[i].get(); + if (cb != null && type.isAssignableFrom(cb.getClass())) { + return cb; + } + } + } + return null; + } + + private static Reference<Callback>[] addCallback(Reference<Callback>[] array, Callback cb) { What do you think about switching the signature of this to: private static void addCallback(Map<Pointer, Reference<Callback>[]> pointerCallbackMap, Pointer pointer, Callback cb) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1268 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFYZLLM45PXRRRQN2CHC5DSRFOPTANCNFSM4TUZSRDQ> .

[matthiasblaesing]

I can see an approach where a callback takes a structure as a reference and the library defines, that all argument candidates contain a marker as the first element of the structure. The callback could then safely examine the first elements and dispatch based on that content. In my idea the base implementation is implemented by a single callback, but on the outside, typesafe callbacks are defined.

In that case the java side could validly encounter the same function pointer as a callback with different signatures. I admit, that this is kind of constructed, but I'm pretty sure, that if I can come up with this, others have done so too.

[fpapai]

Thank you. In general this looks sane to me. I would like to understand though, when the same pointer can sanely assigned to different Callback signatures?

It would be great if you could come up with a microbenchmark, that helps to understand the impact of the change.

Regarding your question:
the use case is an API defined in a C header, one java application code that uses that API to talk to many 'external' (not under my control) DLLs implementing the API. Those DLLs for various reasons may reuse function pointers (feature incompleteness, compiler optimizations). They have the right to reuse them.

Does the project have a benchmarking setup or examples?

[matthiasblaesing]

I ran benchmarked this with jmh on this code:

public class StructureInstantiationBenchmark {
    static interface DummyCallback extends Callback {
        public void dummy();
    }


    @Benchmark
    public Callback[] testExceptionPointerConstructor() throws IOException, URISyntaxException {
        Callback[] cr = new Callback[1000];
        for(int i = 1; i <= cr.length; i++) {
            cr[i - 1] = CallbackReference.getCallback(DummyCallback.class, Pointer.createConstant(i));
        }
        return cr;
    }
}

The performance hit is somewhere between 2.5 and 5%, but the 5% has a wide error margin, so I would consider it minimal.

@fpapai it would be great if you could add an entry to CHANGES.md for this and squash the commits into a single one.

[fpapai]

@fpapai it would be great if you could add an entry to CHANGES.md for this and squash the commits into a single one.

Thank you for the benchmark.
Done: updated CHANGES.md and squashed the commits.

[matthiasblaesing]

Thank you for taking care of this - its appreciated.