-
Notifications
You must be signed in to change notification settings - Fork 289
Bindings IR #2457
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
Bindings IR #2457
Conversation
ea4e1ff to
8dec2cd
Compare
99f3818 to
f1d15da
Compare
8453674 to
6a3b744
Compare
|
Updating this PR to use less macros and syn parsing. Now, instead of wrapping everything with the The main issue was that when we were auto-generating the pass IR types, we would copy all the The new system requires extra boilerplate compared to the last, but it also feels much less magical. I like this trade-off overall, even when you ignore the issue with There's no longer specialized code to convert The other improvement is that the new Node types can operate more dynamically, i.e. the new pipeline is based around |
fb63312 to
c219ada
Compare
d9b45e3 to
bf4acdd
Compare
e097998 to
d1b6813
Compare
mhammond
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really quite remarkable and I know is the result of a lot of work - thanks!
While it's going to be an interesting (but optional!) ride for existing bindings, I'm 100% sure this is going to improve making and rationalizing about bindings improvements.
|
|
||
| // Checksums, these are used to check that the bindings were built against the same | ||
| // exported interface as the loaded library. | ||
| let mut checksums = vec![]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bindings using this wouldn't face #1789 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal was to try to duplicate the current logic with this one, so I think that would still be a bug but I'm not totally sure.
d1b6813 to
975176c
Compare
b3318ff to
4e1440a
Compare
badboy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In yesterday's UniFFI meeting we discussed this.
We agreed that it's worth experimenting with this further and for that to merge this PR.
The mozilla-central code can make use of it to evaluate it further. We also will try to move over one of the built-in generates (most likely Python). This should give us an idea of (a) whether it's possible, (b) it's a real improvement for UniFFI, (c) gauge the effort this takes.
This is not yet a final commitment on this new IR approach. The recommendation for any external users is to NOT use it yet.
@bendk: I hope this summary is accurate. Correct me if there's something wrong.
4e1440a to
31da7f9
Compare
|
Push one more change to this:
My plan is to merge this tomorrow unless there are any more suggestions/concerns. |
fd984a4 to
42b4863
Compare
Adding a new system for bindings generation, modeled after a compiler pipeline with a lot of small steps. See the internals docs for a high-level description of this. This commit only contains the general pipeline, not any language-specific ones. The plan is to use this to create a `uniffi-bindgen-gecko-js` pipeline, then start switching over Python/Swift/Kotlin.
42b4863 to
493a64f
Compare
|
Push a couple more tweaks:
I'm still hoping to merge this one tomorrow. |
|
I was out on Thursday, but now I'm back and can confirm that the Gecko JS bindings work with this new version. Let's merge! |
Adding a new system for bindings generation, modeled after a compiler pipeline with a lot of small steps. See the internal docs in this PR for a high-level overview.
Planning out the work
I have a patch in
uniffi-bindgen-gecko-jsthat builds on this to rework those bindings. I'm hoping to move Python/Kotlin/Swift over to using this system, but there's no code to do that yet. I'm pretty confident that will work though, because it's working with JS and the previous version of this was working with Python.This means that this PR only contains the general half of a pipeline, to see how this could be finished for a particular bindings language, check out the patch for the gecko js bindings. I recommend focusing on the code in the
pipelinesdirectory and the JS templates, the C++ code is weird and specific to the Firefox codebase.The way I'd like to land this all is:
0.29.2release with the code0.30.0with that work, but I don't think there's any particular urgency to it.Comparison to previous works
This is a continuation of #2333 and several other attempts that I never ended up pushing out.
The IRs
The IR types are mostly similar to before, with one big difference: IRs contain items from every crate involved,
Modulenodes are children of theRootnode. My feeling is that this will set us up to finally tackle issues that require cross-module coordination like #1896 and #2430.The passes
I tried sooooo many ways to express the pipeline logic in a nice way. In the end I was inspired by the nanopass framework, where you have a bunch of little passes. That keeps the logic modular and readable. The modules in
uniffi_bindgen/src/pipeline/general, are only doing a single thing. Before, we had one big pass that converted everything, which was harder to understand.The downside of a nanopass framework is that it's pretty easy to make
rustcexplode. My initial try created a separate set of IR node type for each pass, but once I got to a dozen or so the build time foruniffi_bindgenwent to about 30s. Then it started getting killed by the OOM killer and I decided it was time for a new direction.To avoid this, the new system creates 2 IRs, but then splits up the work into multiple
passfunctions. Even though each step works on the same IR, it still kind of feels like they're transforming the types. Instead of adding a new field, they now set a value for a field that was previously empty.This achieves more-or-less the same effect as the nanopass system, but with much faster compile times. My not-very-scientific measurement is that it went from 12s before to 17s now. That's not great, but if this is successful we can hopefully delete a bunch of the old code and get times down to something reasonable again.
The CLI
I replaced the
peekanddiffcommands with thepipelinecommand. If you have time, read the internals doc about it and take it for a test drive. IMO it's pretty fun walking through each pipeline step.