RustBuffer should be a data pointer #1779

bendk · 2023-10-04T18:51:12Z

RustBuffer is currently defined as:

pub struct RustBuffer {
    capacity: i32,
    len: i32,
    data: *mut u8,
}

I'd think this should change to just a *mut u8. We could write the capacity and len as the first 8 bytes to create an equivalent structure. Maybe we could drop one or both of these fields, the foreign FFI converter code can now calculate the size it needs so I'm not sure these are necessary anymore.

The reason for the change is that pointers are simpler to handle at the at the FFI layer than structs. I'm currently running into a 14 year-old ctypes issue that prevents us from returning RustBuffer from Python callbacks. I remember noticing that the structs were relatively slow on Kotlin when I was looking at callback interface performance. That's not a ton of data points, but my feeling is that making this switch will make things easier in general.

The text was updated successfully, but these errors were encountered:

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Python requires a hack to return RustBuffer from callbacks (RubiconCtypesPatch.py) because of an old ctypes bug (https://bugs.python.org/issue5710). The options I can see are: - Go back to the callback interface style, where we input a RustBuffer and write the output to it. - Pass the return type as an out pointer. This requires some special handling for void returns though. - Implement mozilla#1779, which changes RustBuffer to a simple `*mut u8` and then Python could return it. This is my preference, I don't like the idea of making the ABI worse just because of Python. TODO: If I make the Python tests fail, I see `RuntimeWarning: memory leak in callback function` in the console. I believe this is because of the rubicon hack, but I'm not sure. We should double check this before merging. Kotlin is not implemented yet. I think when we do implement it, we're going to want to face mozilla#1394 and make a decision there. I don't think it's reasonable to have consumers have to call a `close()` method on these interfaces. I also don't know how we would deal with this in Rust. ISTM that the same logic that says Kotlin objects need to be manually closed dictates that if you use a Kotlin object in Rust, there should be a `close()` method or something analogous to a to `AutoCloseable`. The Ruby coverall tests are now failing because they don't implement trait interfaces. We need to figure out a way for them to pass the test suite.

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Python requires a hack to return RustBuffer from callbacks (RubiconCtypesPatch.py) because of an old ctypes bug (https://bugs.python.org/issue5710). The options I can see are: - Go back to the callback interface style, where we input a RustBuffer and write the output to it. - Pass the return type as an out pointer. This requires some special handling for void returns though. - Implement mozilla#1779, which changes RustBuffer to a simple `*mut u8` and then Python could return it. This is my preference, I don't like the idea of making the ABI worse just because of Python. TODO: If I make the Python tests fail, I see `RuntimeWarning: memory leak in callback function` in the console. I believe this is because of the rubicon hack, but I'm not sure. We should double check this before merging. Kotlin is not implemented yet. I think when we do implement it, we're going to want to face mozilla#1394 and make a decision there. I don't think it's reasonable to have consumers have to call a `close()` method on these interfaces. I also don't know how we would deal with this in Rust. ISTM that the same logic that says Kotlin objects need to be manually closed dictates that if you use a Kotlin object in Rust, there should be a `close()` method or something analogous to a to `AutoCloseable`. The Ruby coverall tests are now failing because they don't implement trait interfaces. We need to figure out a way for them to pass the test suite. Remove all the debugging printouts

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Python requires a hack to return RustBuffer from callbacks (RubiconCtypesPatch.py) because of an old ctypes bug (https://bugs.python.org/issue5710). The options I can see are: - Go back to the callback interface style, where we input a RustBuffer and write the output to it. - Pass the return type as an out pointer. This requires some special handling for void returns though. - Implement mozilla#1779, which changes RustBuffer to a simple `*mut u8` and then Python could return it. This is my preference, I don't like the idea of making the ABI worse just because of Python. TODO: If I make the Python tests fail, I see `RuntimeWarning: memory leak in callback function` in the console. I believe this is because of the rubicon hack, but I'm not sure. We should double check this before merging. Kotlin is not implemented yet. I think when we do implement it, we're going to want to face mozilla#1394 and make a decision there. I don't think it's reasonable to have consumers have to call a `close()` method on these interfaces. I also don't know how we would deal with this in Rust. ISTM that the same logic that says Kotlin objects need to be manually closed dictates that if you use a Kotlin object in Rust, there should be a `close()` method or something analogous to a to `AutoCloseable`. The Ruby coverall tests are now failing because they don't implement trait interfaces. We need to figure out a way for them to pass the test suite. Remove all the debugging printouts Document this

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Unfortunately, we can't directly return the return value on Python because of an old ctypes bug (https://bugs.python.org/issue5710). Instead, input an out param for the return type. The other main possibility would be to change `RustBuffer` to be a simple `*mut u8` (mozilla#1779), which would then be returnable by Python. However, it seems bad to restrict ourselves from ever returning a struct in the future. Eventually, we want to stop using `RustBuffer` for all complex data types and that probably means using a struct instead in some cases. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Kotlin is not implemented yet. I think when we do implement it, we're going to want to face mozilla#1394 and make a decision there. I don't think it's reasonable to have consumers have to call a `close()` method on these interfaces. I also don't know how we would deal with this in Rust. ISTM that the same logic that says Kotlin objects need to be manually closed dictates that if you use a Kotlin object in Rust, there should be a `close()` method or something analogous to a to `AutoCloseable`. The Ruby coverall tests are now failing because they don't implement trait interfaces. We need to figure out a way for them to pass the test suite. Remove all the debugging printouts Document this

This adds support for the foreign bindings side to implement Rust traits and for both sides to pass `Arc<dyn [Trait]>` objects back and forth. I thought this would be a good time to redo the callback interface code. The trait interface code uses a foreign-supplied vtable rather than a single callback and method indexes like the old code used. Unfortunately, we can't directly return the return value on Python because of an old ctypes bug (https://bugs.python.org/issue5710). Instead, input an out param for the return type. The other main possibility would be to change `RustBuffer` to be a simple `*mut u8` (mozilla#1779), which would then be returnable by Python. However, it seems bad to restrict ourselves from ever returning a struct in the future. Eventually, we want to stop using `RustBuffer` for all complex data types and that probably means using a struct instead in some cases. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. -------------------- TODO ----------------------------- This currently requires "wrapping" the object every time it's passed across the FFI. If the foreign code receives a trait object, then passes it back to Rust. Rust now has a handle to the foreign impl and that foreign impl just calls back into Rust. I think mozilla#1730 could help solve this. I think there should be better tests for reference counts, but I'm waiting until we address the previous issue to implement them. Document this

Instead of registering a single method to implement a callback interface, foreign code now registers a VTable. VTable sounds fancy, but it's just a repr(C) struct where each field is a callback function. This gives us some more flexibility with the method signatures. Before, all arguments were passed using a RustBuffer, but not all FFI types can be written to a RustBuffer. In particular, I want to be able to pass callback function pointers. This also makes the callback interface FFI closer to the Rust one. I wanted to make it match exactly, but it didn't work out. Unfortunately, we can't directly return the return value on Python because of an old ctypes bug (https://bugs.python.org/issue5710). Instead, input an out param for the return type. The other main possibility would be to change `RustBuffer` to be a simple `*mut u8` (mozilla#1779), which would then be returnable by Python. However, it seems bad to restrict ourselves from ever returning a struct in the future. Eventually, we want to stop using `RustBuffer` for all complex data types and that probably means using a struct instead in some cases. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. Switched the keywords fixture tests to use traits rather than callback interfaces. The old tests were testing unsupported things like returning callback interfaces which just happened to pass. Removed the reexport-scaffolding-macro fixture. I don't think this one is giving us a lot of value anymore and I don't want to keep updating it when the FFI changes.

Instead of registering a single method to implement a callback interface, foreign code now registers a VTable. VTable sounds fancy, but it's just a repr(C) struct where each field is a callback function. This gives us some more flexibility with the method signatures. Before, all arguments were passed using a RustBuffer, but not all FFI types can be written to a RustBuffer. In particular, I want to be able to pass callback function pointers. This also makes the callback interface FFI closer to the Rust one. I wanted to make it match exactly, but it didn't work out. Unfortunately, we can't directly return the return value on Python because of an old ctypes bug (https://bugs.python.org/issue5710). Instead, input an out param for the return type. The other main possibility would be to change `RustBuffer` to be a simple `*mut u8` (mozilla#1779), which would then be returnable by Python. However, it seems bad to restrict ourselves from ever returning a struct in the future. Eventually, we want to stop using `RustBuffer` for all complex data types and that probably means using a struct instead in some cases. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. Removed the reexport-scaffolding-macro fixture. I don't think this one is giving us a lot of value anymore and I don't want to keep updating it when the FFI changes.

Instead of registering a single method to implement a callback interface, foreign code now registers a VTable. VTable sounds fancy, but it's just a repr(C) struct where each field is a callback function. This gives us some more flexibility with the method signatures. Before, all arguments were passed using a RustBuffer, but not all FFI types can be written to a RustBuffer. In particular, I want to be able to pass callback function pointers. This also makes the callback interface FFI closer to the Rust one. I wanted to make it match exactly, but it didn't work out. Unfortunately, we can't directly return the return value on Python because of an old ctypes bug (https://bugs.python.org/issue5710). Instead, input an out param for the return type. The other main possibility would be to change `RustBuffer` to be a simple `*mut u8` (#1779), which would then be returnable by Python. However, it seems bad to restrict ourselves from ever returning a struct in the future. Eventually, we want to stop using `RustBuffer` for all complex data types and that probably means using a struct instead in some cases. Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings templates. This matches the name in the Rust code and makes more sense for foreign trait implementations. Removed the reexport-scaffolding-macro fixture. I don't think this one is giving us a lot of value anymore and I don't want to keep updating it when the FFI changes.

bendk changed the title ~~RustBuffers should be pointers~~ RustBuffers should be a data pointer Oct 4, 2023

bendk changed the title ~~RustBuffers should be a data pointer~~ RustBuffer should be a data pointer Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RustBuffer should be a data pointer #1779

RustBuffer should be a data pointer #1779

bendk commented Oct 4, 2023

RustBuffer should be a data pointer #1779

RustBuffer should be a data pointer #1779

Comments

bendk commented Oct 4, 2023