panama-backend - basic integration with jextract #499

jcrist1 · 2024-06-14T10:27:10Z

I've implemented a basic integration with jextract for a pure java backend. This is just a baseline to start building on, that generates the C2 backend, and uses that with jextract to create the java interface to the native code. There's no code generation yet for ergonomic interfaces. I feel like enums and structs will be relatively straightforward, so once again I will start off building the wrappers for opaque type and cleanup. Also I don't actually know any java yet 🤪, but no time like the present to learn. This is as discussed in #144

Edit (2024-07-11):

I've been struggling a bit to make time for this lately as other priorities have taken precedence but I've managed another big chunk today.

I'm still missing the actual complete codegen pipeline. For the time being I'm just copy pasting generated code from the insta snapshots into the project, to test that it works, as I don't want to implement all of the features yet, and it's more convenient to just panic. Otherwise, this is starting to come together, so I'm going to add the checklist again to keep track of what I need to do still.

jcrist1 · 2024-06-14T10:32:01Z

tool/src/java/mod.rs

+
+    let mut lib_file = File::create(&lib_path)?;
+    for include in include_files {
+        writeln!(lib_file, "#include \"{include}\"")?;


jextract suggests loading all headers into a single library file. This gets transformed into an enormous monster of java

jcrist1 · 2024-06-14T10:33:26Z

tool/src/lib.rs

+            attr_validator.support.renaming = true;
+            attr_validator.support.disabling = true;
+            attr_validator.support.iterators = true;
+            attr_validator.support.iterables = true;
+            attr_validator.support.indexing = true;
+            attr_validator.support.constructors = true;
+            attr_validator.support.named_constructors = true;
+            attr_validator.support.memory_sharing = true;
+            attr_validator.support.accessors = true;


Just enabled everything for generating some code to start off with. Will adjust later

jcrist1 · 2024-06-14T21:18:07Z

I now manually implemented an opaque type with cleanup as well. This will be the model for the template. I also added a benchmark where I just instantiate the opaque struct because I was quite curious what the cost of GC was. With GC on the benchmark throughput more than halved. going from ~2.7M/s to 1.2 M/s. A slow down from about 350ns to ~800ns, which is just interesting. For comparison the opaque formatter in the example took ~8000ns, so this could be about a 10x speedup. All in all this seems worth pursuing and my biggest open questions for this have been resolved.

Manishearth

this is neat but i'm kind of surprised it works??

Manishearth · 2024-06-18T23:58:22Z

tool/src/java/mod.rs

+) -> std::io::Result<FileMap> {
+    let files = FileMap::default();
+    let mut context = c2::CContext::new(tcx, files, false);
+    context.run();


Note that after #495, the C2 backend can now be directly invoked per-type in a cleaner fashion, if you decide that's more useful

I'd prefer to implement that as a followup.

I actually implemented that. Biggest problem is that I need the C runtime too, which I made pub (crate)

Manishearth · 2024-06-18T23:58:43Z

feature_tests/java/somelib/src/main/java/dev/diplomattest/somelib/Opaque.java

+        somelib_h.Opaque_destroy(this.internal);
+    }
+
+    public void assertStruct(MyStruct struct) {


question: wait, jextract generated this from the C header files? How did it know that these C header files correspond to a class with methods?

Sadly no, that would be amazing 😃. That’s just a hand rolled wrapper for a more ergonomic type. I’m using this to build the templating for opaque types

Everything in the ntv package was generated using jextract from the c backend.

Aha! Okay, carry on!

jcrist1 · 2024-06-28T12:12:47Z

feature_tests/java/somelib/src/main/java/dev/diplomattest/somelib/Opaque.java

+import java.nio.charset.StandardCharsets;
+
+
+public class Opaque {


Although the tool doesn't actually generate code, the class body has now been been copied from an insta snapshot.

☝️ no longer valid 😝

jcrist1 · 2024-06-28T12:13:15Z

tool/src/java/snapshots/diplomat_tool__java__test__opaque.snap

+import java.lang.ref.Cleaner;
+
+
+public class Opaque {


Here is the snapshot from where we copied the body

jcrist1 · 2024-07-11T13:18:11Z

tool/src/java/mod.rs

+                // We can probably do boxed returns by just relying on jna
+                // public double[] asBoxedSLice() {
+                //     try (var arena = Arena.ofConfined()) {
+                //         var nativeVal = somelib_h.Float64Vec_as_boxed_slice(arena, internal);
+                //         var data = dev.diplomattest.somelib.ntv.DiplomatF64View.data(nativeVal);
+                //         var len = dev.diplomattest.somelib.ntv.DiplomatF64View.len(nativeVal);
+                //         var returnVal = data.asSlice(0, len * JAVA_DOUBLE.byteSize()).toArray(JAVA_DOUBLE);
+                //         Native.free(data.address());
+                //         return returnVal;
+                //     }
+                // }


Here's what a boxed slice would look like using jna. It should be the only place where we'd need to use it.

jcrist1 · 2024-09-13T11:53:35Z

feature_tests/java/somelib/src/jmh/java/dev/diplomattest/somelib/OpaqueBench.java

+import org.openjdk.jmh.runner.options.Options;
+import org.openjdk.jmh.runner.options.OptionsBuilder;
+
+public class OpaqueBench {


I don't have a shell command to actually run this yet. I only ran it from IntelliJ, but want to keep it as it'll be nice to have to compare to the kotlin backend once the icu4x example can be made to work

jcrist1 · 2024-09-13T11:58:06Z

feature_tests/java/somelib/src/main/java/dev/diplomattest/somelib/ntv/FILE.java

A lot of files seem to be generated by default. It seems to be possible to pass in every single function and struct to jextract to specify which ones should be used to generate code, but I can't see a way to only forbid these standard constructions which are causing a lot of noise. I'd like to move that investigation to a followup.

jcrist1 · 2024-09-13T12:01:15Z

feature_tests/src/slices.rs

@@ -17,6 +17,7 @@ mod ffi {
            Box::new(Self(v.to_string()))
        }

+        #[diplomat::attr(supports = memory_sharing, disable)]


@Manishearth I had an issue with passing owned strings in java as then I needed to pass an arena around which felt contradictory to the idea of an owned parameter.

yeah you need to instead call diplomat_alloc and write to that string when you see this kind of thing.

well, you need to do that for all strings, the difference with owned strings is that you don't free it afterwards

jcrist1 · 2024-09-13T12:02:00Z

runtime/src/lib.rs

@@ -33,6 +33,7 @@ pub type DiplomatStr = [u8];
 pub type DiplomatStr16 = [u16];

 /// Like [`u8`], but interpreted explicitly as a raw byte as opposed to a numerical value.
+///


I think this came from a clippy fix, but I'm not sure

jcrist1 · 2024-09-13T12:02:38Z

tool/src/c/mod.rs

@@ -38,15 +38,15 @@ pub(crate) fn attr_support() -> BackendAttrSupport {
    a
 }

+#[derive(askama::Template)]
+#[template(path = "c/runtime.h.jinja", escape = "none")]
+pub(crate) struct Runtime;


need access to the c runtime for the java backend so that jextract can process it.

jcrist1 · 2024-09-13T12:04:12Z

tool/src/java/mod.rs

+    converted_value: Cow<'cx, str>,
+}
+
+mod arena {


I wanted param conversions to not have to be aware of the context wherein they are done, and so introduced a kind of inversion of control, where if a conversion needs an arena it returns a variant that can only be accessed by providing an arena.

jcrist1 · 2024-09-13T12:05:52Z

tool/src/java/mod.rs

+
+impl<'a, 'cx> TyGenContext<'a, 'cx> {
+    #[allow(unused)]
+    fn java_to_native<T: TyPosition>(


As per your comment last review @Manishearth I tried to harmonise the param conversion to just a language conversion, so it can be reused in e.g. the struct conversion. It feels a lot cleaner but I admit the control inversion above is a little weird.

jcrist1 · 2024-09-13T12:08:02Z

Okay @Manishearth I think this could do with another review. I substantially redid the native <-> java conversion code. Also added the ci steps. I'm still missing a ci step that actually runs the tests instead of skipping, but I'd like to wrap this up, and focus on smaller pieces at a time.

jcrist1 · 2024-09-13T13:36:24Z

🎉 🎉 🎉 🎉 🎉 🎉

Manishearth

oops, never posted this

Manishearth · 2024-09-13T17:47:16Z

feature_tests/java/somelib/src/main/java/dev/diplomattest/somelib/ntv/FILE.java

@@ -0,0 +1,47 @@
+// Generated by jextract


nit: we shouldn't need FILE

Manishearth · 2024-09-13T17:47:41Z

feature_tests/java/somelib/src/main/java/dev/diplomattest/somelib/ntv/NestedBorrowedFields.java

@@ -0,0 +1,219 @@
+// Generated by jextract


nit: we don't need any of these either

this comment was supposed to be on the threading files below this

Manishearth · 2024-09-13T17:50:43Z

feature_tests/src/slices.rs

@@ -17,6 +17,7 @@ mod ffi {
            Box::new(Self(v.to_string()))
        }

+        #[diplomat::attr(supports = memory_sharing, disable)]


yeah you need to instead call diplomat_alloc and write to that string when you see this kind of thing.

well, you need to do that for all strings, the difference with owned strings is that you don't free it afterwards

Manishearth · 2024-09-13T18:17:33Z

tool/src/java/mod.rs

+    //   /path/to/mylib/include/mylib.h
+
+    let package = format!("{domain}.{lib_name}.ntv");
+    let mut command = std::process::Command::new("jextract");


thought: would be nice to accept the jextract binary location from the CLI/config

Manishearth · 2024-09-19T01:05:21Z

Alright, so my biggest concern with this is making the JDK a part of the gen step. "Run tests for a different backend" isn't a common task for Diplomat development, but "regen test code for all backends" is (since it's common enough to add stuff to the feature_tests crate), and this means all Diplomat devs need to set up JDK, something that can often be very finicky.

I would like for the Java backend to be a part of this repo, however. We have a couple routes in front of us, and I'd like to know what you think:

Diplomat devs get used to setting up JDK, potentially with us merging PRs that temporarily break the java gen task (we can keep it a separate CI job) with the understanding that one of us with a JDK setup will fix it. I can commit to doing so, though I'd rather not.
- Downside: This is going to be really annoying for other Diplomat devs. I'm open to this but I'd rather not go down this route.
This backend is added in-tree, however we do not check in generated code for it, and CI runs gen-java before running tests.
- Downside: we lose out on Java diffs
This backend is added in-tree, and we check in regular generated code but not the jextracted code, and CI runs jextract (or perhaps some command like diplomat-tool --backend jextractwrapper)
- Downside: Kind of weird, we still lose out on diffs but not the important ones
This backend is maintained as rust-diplomat/java as a separate crate, and has full CI there.
- Downside: Diplomat changes to the HIR don't automatically get nicely propagated there, it's possible for the backends to diverge more in features. Right now even if I don't maintain the Kotlin backend whenever I change something it is relatively easy for me to ensure that Kotlin is kept up to date most of the time.
We manually generate bindings using JEP 454 APIs rather than using jextract

Downside: this is a bunch of extra work

The last one is my preferred end state, honestly: when we were considering Panama we largely intended for the JEP 454 java.lang.foreign (etc) APIs to be used, but not jextract. I think jextract is a great way to bootstrap this project, though.

A hybrid route I find somewhat appealing is to take option 4 or 5 above ("don't check in the full bindings"), and over time move to a diplomat that doesn't need jextract. A particularly cool option would be for diplomat to generate an ntv/ folder that is API compatible with jextract, so we can run tests swapping out the diplomat-generated ntv with the jextracted ntv.

@jcrist1 what do you think? Ultimately you're the one doing the work here, so I don't want to dictate anything, but I want to try and resolve the tradeoff with the impact on other backends, and try to understand your own preferences here.

jcrist1 · 2024-09-19T19:55:25Z

@Manishearth thanks for the feedback. I'll probably just implement a filter to remove the extraneous generated files from jextract ... depending on the conclusion of what to do with this backend

... need to set up JDK, something that can often be very finicky.

write once run... something something ... I forget the rest 😛

Honestly the jextract thing bothers me from a rust devx perspective as well as an end user perspective. I agree that generating the JEP 454 binding code will probably be the way to go longer term, but I gotta be honest, I'm not super enthusiastic about doing that myself, not right now at least. While jextract generates a lot of superfluous stuff and is a bit of a pain to setup, it also seems like the easiest way to get functional bindings.

So for that moving a java backend to its own repo in the diplomat project is a nice compromise. We could get a working backend, and slowly transition to diplomat generated JEP 454 code. Having a repo in the rust-diplomat project would give the panama backend a bit more legitimacy (and hopefully direct other contributors to it) while signalling that the support level is lower and also allowing a more customised dev-setup. We could definitely open an issue around setting up the JEP 454 generation but it's not the most motivating kind of coding for me so progress there will be slow. I would like to pin the diplomat dependency to a specific commit and maybe set up an alert for when the latest main breaks it. That way people can still have a working tool, but I could get notified to address incompatible changes to the diplomat dependency so maintenance wouldn't fall on your shoulders.

jcrist1 · 2024-09-19T20:01:54Z

@Manishearth If we're agreed I can try and set up a standalone bin project tomorrow, if you can create the rust-diplomat/java repo. I'll see if I can get an "initial commit" PR by end of day

Manishearth · 2024-09-19T21:07:03Z

@jcrist1 here you go: https://github.com/rust-diplomat/diplomat-java

jcrist1 · 2024-09-20T12:50:03Z

Closing in favor of rust-diplomat/diplomat-java#1

panama-backend - basic integration with jextract

e260451

jcrist1 commented Jun 14, 2024

View reviewed changes

panama-backend - bench / test / and cleaning

7c5b60d

Manishearth reviewed Jun 18, 2024

View reviewed changes

panama-backend - add generated code for ICU4X / basic opaque generation

037aacf

jcrist1 commented Jun 28, 2024

View reviewed changes

panama-backend - add enums and structs / limited slice support

c8282c9

jcrist1 commented Jul 11, 2024

View reviewed changes

panam-backend - semicolon

5272832

Manishearth mentioned this pull request Jul 11, 2024

Tracking: Diplomat needs for mls-rs #534

Open

5 tasks

jcrist1 added 17 commits July 17, 2024 22:10

panama-backend - fixing structs

dc8969f

panama-backend - fix struct generation

aa87b13

panama-backend - basic lifetime bounds

c3cb18f

panama-backend - revert lifetime experimentation

e863425

panama-backend - fix lifetimes

68e6beb

Merge remote-tracking branch 'origin/main' into panama-backend

e58ed73

panama-backend - remove accidental type

7005062

panama-backend - adding overrides

c02c92f

panamaa-backend - complete pipeline

fdc619b

panama-backend - clippy

031272d

panama-backend - manually address clippy issues

a323918

panama-backend - accept tests / gen all code

127d6ff

panama-backend - remove java from gen as ci would need jextract

2b10d7a

panama-backend - fix test

69cbab6

panama-backend - remove printlns

eb6e890

Merge remote-tracking branch 'origin/main' into panama-backend

7cd8e8a

panama-backend - fix merge issues

ee58338

jcrist1 added 9 commits September 13, 2024 11:39

panama-backend - clippy

bc7c3fe

Merge remote-tracking branch 'origin/main' into panama-backend

47f7ce9

panama-backend - fix merge issues

dfe88e3

panama-backend - fix kotlin test from merge

42d941b

panama-backend - more test fixes

4aa75d6

panama-backend - fix demo changes and some cpp stuff

d633c3b

panama-backend - mac os S**t

7f37a99

panama-backend - more test fixing

bfc71ea

panama-backend - remove IntelliJ cruft

14ae4a5

jcrist1 commented Sep 13, 2024

View reviewed changes

jcrist1 requested a review from Manishearth September 13, 2024 12:08

jcrist1 added 2 commits September 13, 2024 15:31

panama-backend - troubleshoot mvn test

b00adf6

panama-backend - remove pom comments

8675eb3

Manishearth reviewed Sep 18, 2024

View reviewed changes

This was referenced Sep 20, 2024

tool-and-c-backend-pub - Make some features of the tool pub for exter… #694

Merged

move-panama-backend rust-diplomat/diplomat-java#1

Merged

jcrist1 closed this Sep 20, 2024

		import java.nio.charset.StandardCharsets;


		public class Opaque {

panama-backend - basic integration with jextract #499

panama-backend - basic integration with jextract #499

Conversation

jcrist1 commented Jun 14, 2024 • edited Loading

Edit (2024-07-11):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrist1 commented Jun 14, 2024 • edited Loading

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrist1 Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrist1 commented Sep 13, 2024 • edited Loading

jcrist1 commented Sep 13, 2024

Manishearth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Sep 19, 2024

jcrist1 commented Sep 19, 2024 • edited Loading

jcrist1 commented Sep 19, 2024 • edited Loading

Manishearth commented Sep 19, 2024

jcrist1 commented Sep 20, 2024

jcrist1 commented Jun 14, 2024 •

edited

Loading

jcrist1 commented Jun 14, 2024 •

edited

Loading

jcrist1 Sep 13, 2024 •

edited

Loading

jcrist1 commented Sep 13, 2024 •

edited

Loading

jcrist1 commented Sep 19, 2024 •

edited

Loading

jcrist1 commented Sep 19, 2024 •

edited

Loading