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

Use C structs/tagged unions for Records/Enums #2156

Open
bendk opened this issue Jun 17, 2024 · 2 comments
Open

Use C structs/tagged unions for Records/Enums #2156

bendk opened this issue Jun 17, 2024 · 2 comments
Labels

Comments

@bendk
Copy link
Contributor

bendk commented Jun 17, 2024

Currently these types are always serialized into a RustBuffer which has been long believed to be inefficient (Issue 244, ADR-002). We should explore passing these as repr(C) structs and tagged unions to avoid the overhead of serialization and allocating a buffer.

These could either be passed across the FFI as a pointer/reference or as the struct directly. Some of the recent changes could help here, for example we now have both FfiType::Struct and FfiType::Reference.

Any change here should be accompanied by a benchmark that measures performance changes.

@bendk bendk added the FFI-1.0 label Jun 17, 2024
@bendk
Copy link
Contributor Author

bendk commented Jun 17, 2024

Any change here should be accompanied by a benchmark that measures performance changes.

Interestingly, the very first benchmark tackled an issue related to this: the callback interface system that passed a RustBuffer struct across the FFI was slow on Kotlin and Python and performance was improved by passing data and len arguments rather than a single RustBuffer argument. After doing some research on it, I now believe this was caused by the amd64 calling conventions. When that PR landed, RustBuffer contained a data pointer and 2 u32 fields for a total of 2 words. By convention, callers pack that data into 2 registers. This is normally an optimization, but when the data needs to flow through libffi, JNA, and then be unpacked by Python/Kotlin I believe it actually slowed things down.

I'm not sure what we should do about this: I might be misdiagnosing the issue, maybe it's a non-issue that only happens with very particular structs, or maybe we should prefer passing structs using pointers to avoid the performance hit.

@badboy
Copy link
Member

badboy commented Jun 19, 2024

In the past (before switching to UniFFI) Glean used c structs/tagged enums to pass some data and we encountered at least one edge case/bug in JNA causing data corruption along the way. That's probably (hopefully?) fixed now, if we go this route we need to have extensive testing to ensure it works.

fwiw, I did have this idea for UniFFI a long while ago as well, just never acted on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants