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

Deprecate C based bindings and default to pure dart bindings #660

Closed
3 tasks done
mahesh-hegde opened this issue Mar 23, 2023 · 6 comments · Fixed by #1091
Closed
3 tasks done

Deprecate C based bindings and default to pure dart bindings #660

mahesh-hegde opened this issue Mar 23, 2023 · 6 comments · Fixed by #1091

Comments

@mahesh-hegde
Copy link
Contributor

mahesh-hegde commented Mar 23, 2023

I believe pure dart bindings are the future.

If there's a significant disrepancy in performance between C based and Dart based bindings even after adapting FFI varargs, we can discuss about making small changes in SDK to compensate. It's very unlikely that any factor affecting JNI bindings performance cannot be implemented in SDK once and all, instead of shoving C code into each library.

But until then, defaulting to pure dart bindings simplifies user experience and dev experience. From some recent issues we know getting started is too intimidating for users.

In general, I propose upcoming v0.4 as a stability release, to lay solid groundwork for upcoming feature development1. @HosseinYousefi is already working on a test suite overhaul.

Steps:

  • Preliminary benchmark suite. Enough to prove that pure dart bindings do not affect performance significantly.

  • Remove C based tests & examples.

  • Update user documentation.


Footnotes

  1. Part of the blame is on me for eg: shoving too many integration tests and options.

@seifibrahim32
Copy link

seifibrahim32 commented Mar 28, 2023

@mahesh-hegde the C bindings are necessary , because I read JNI tips from Android Documentarion, it cant interop without C headers, but if you need to propose this , so you can use free function (use C++/C language as usual for freeing the JNI pointers inside C bindings).

@mahesh-hegde
Copy link
Contributor Author

@seifibrahim32 I don't understand your point.

But I wrote this library and sure have an idea what I am proposing :)

@seifibrahim32
Copy link

Awesome I would know that mahesh, what I understood from you is you need to use Dart parameters only in the package and remove C and deprecated or unsupport the C bindings,

so I tell you that if you need to use Android ,for example you cant use without C.

I read this documentarion and says it at all.
https://www.google.com/url?sa=t&source=web&rct=j&url=https://developer.android.com/training/articles/perf-jni&ved=2ahUKEwjageD72_79AhUYUKQEHdLNCokQFnoECBkQAQ&usg=AOvVaw0dRMAFHNpjKhf-EJZ3A3gX

For desktop , I think with you it would be better

@mahesh-hegde
Copy link
Contributor Author

mahesh-hegde commented Mar 28, 2023

There's still C code involved in Pure Dart bindings, just that it's only in support library package:jni and you don't have to compile C code for each project. That makes these bindings much more similar to ffigen's bindings, and easier to use.

There's a slight performance hit last time I benchmarked, due to argument marshalling. FFI varargs should fix most of that.

Edit: and yeah there's no difference for desktop. You need jni dll which can be built using jni:setup but you don't generate any additional C bindings.

@seifibrahim32
Copy link

seifibrahim32 commented Apr 6, 2023

There's still C code involved in Pure Dart bindings, just that it's only in support library package:jni and you don't have to compile C code for each project. That makes these bindings much more similar to ffigen's bindings, and easier to use.

There's a slight performance hit last time I benchmarked, due to argument marshalling. FFI varargs should fix most of that.

Edit: and yeah there's no difference for desktop. You need jni dll which can be built using jni:setup but you don't generate any additional C bindings.

@mahesh-hegde I understood your point , you mean JNI can make autodetection for any Java code without putting extra C , correct point.

We should minimize this marshalling resources of C bindings generated by jnigen , also you proved it correctly because indeed Android documentation says it. , but if we will do it for non Android , we shouldnt make JNI spawn on Desktop ?.

@HosseinYousefi HosseinYousefi transferred this issue from dart-archive/jnigen Nov 17, 2023
@mit-mit
Copy link
Member

mit-mit commented Jan 19, 2024

Maybe a good first step here is to change the default so that bindings_type: dart_only is on by default, and you have to add bindings_type: c_based if you want that?

@HosseinYousefi HosseinYousefi added this to the JNI / JNIgen 0.9.0 milestone Mar 26, 2024
@HosseinYousefi HosseinYousefi self-assigned this Apr 8, 2024
@HosseinYousefi HosseinYousefi moved this to Todo in JNIgen tracker Apr 8, 2024
@HosseinYousefi HosseinYousefi moved this from Todo to In Progress in JNIgen tracker Apr 8, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in JNIgen tracker Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants