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

Add support for DartNativeExternalTypedData to enable zero-copy buffers #3

Merged
merged 6 commits into from Sep 10, 2021

Conversation

jerel
Copy link
Contributor

@jerel jerel commented Aug 23, 2021

Edit: this has now been updated to add a ZeroCopyBuffer type. Usage is now simply:

use allo_isolate::ZeroCopyBuffer;

let buffer: Vec<u8> = serialization_lib::serialize(&data);
isolate.post(ZeroCopyBuffer(buffer));

Since there is already an impl for `Vec` I'm not sure of a good way to add this zero-copy approach to `into_dart` without it conflicting with the existing copy approach so I've just included a working example below. For now we're doing the work shown in application code and passing the resulting `DartCObject` to `isolate.post()` and it works well. This PR adds the necessary types to enable that.

The Dart VM exposes both the DartNativeTypedData and the DartNativeExternalTypedData as a Uint8List but in the DartNativeExternalTypedData case the finalizer calls the provided callback to deallocate.

The implementation that sends a zero copy mutable buffer to Dart looks like this:

impl IntoDart for Vec<u8> {
    fn into_dart(mut self) -> DartCObject {
        self.shrink_to_fit();
        let length = self.len();
        assert!(length == self.capacity());
        let ptr = self.as_mut_ptr();
        std::mem::forget(self);

        DartCObject {
            ty: DartCObjectType::DartExternalTypedData,
            value: DartCObjectValue {
                as_external_typed_data: DartNativeExternalTypedData {
                    ty: DartTypedDataType::Uint8,
                    length: length as isize,
                    data: ptr,
                    peer: ptr,
                    callback: deallocate_rust_buffer,
                },
            },
        }
    }
}

@cmc5788
Copy link

cmc5788 commented Aug 23, 2021

For reference, we've been doing some experiments with Rust -> Flutter on embedded systems. Avoiding a buffer copy is more performant in some cases, and also enables zero-copy serialization approaches such as flat_buffers, capnproto, etc. At a large enough buffer size, doing a full copy using DartNativeTypedData will block the receiving isolate, and if that happens to be the main isolate, drop frames.

We're still doing some experiments to see just how effective this approach is, but the idea is to enable transmitting data from Rust -> Dart in a way that makes frame drops unlikely.

@shekohex
Copy link
Owner

shekohex commented Aug 24, 2021

That's really really interesting for me too, I guess we can swap the current Vec<u8> implementation with this new one and release it under v0.2.0-beta version or something and test it out in our applications.

Another approach is to create a wrapper type over anything that can be converted Into<Vec<u8>> à la ZeroCopyBuffer and that way we don't introduce any breaking changes and update the docs with an example.

In my opinion the second approach is better and maybe in the future like when releasing a v0.2 we can make it the default.

My current use case for this that I'm building something that uses ProtocolBuffers as Request Response approach, instead of calling a lot of FFI functions in both languages.

I would like to see if we could work on the ZeroCopyBuffer First with a nice API around it.

@jerel
Copy link
Contributor Author

jerel commented Aug 25, 2021

I would think that a ZeroCopyBuffer wrapper would be better than replacing Vec<u8> because there's likely still use cases where the existing copy approach is desirable. For example in some benchmarks with large amounts of data (100k structs) @cmc5788 observed frame drops during the call back to Rust to free the Rust owned memory, so I would think that letting the user choose the approach would be good.

I can add that to this PR if you like.

@shekohex
Copy link
Owner

Yes, That would be great.

Thank you.

@cmc5788
Copy link

cmc5788 commented Aug 25, 2021

FYI -- I haven't been able to reproduce the results from frame drops on native free so far, but with the ZeroCopyBuffer and serde / bincode I was able to run a test today that proves we can transmit essentially unlimited data between Rust -> Dart without frame drops. The point at which frames start dropping is when the Dart-side GC of the deserialized objects becomes the bottleneck, but ZeroCopyBuffer is actually very efficient when coupled with a binary serialization approach.

When running the same test using DartNativeTypedData with the buffer copy, frames are consistently dropped, and the larger the data transfer the more frames drop. So far this approach is seeming like it wouldn't have many downsides.

@shekohex
Copy link
Owner

shekohex commented Sep 9, 2021

That looks good so far, thank you @jerel.

Could you please fix the lints too?

src/into_dart.rs Outdated Show resolved Hide resolved
src/ffi.rs Show resolved Hide resolved
@shekohex
Copy link
Owner

Ok it sounds that it is something with CI uses nightly, which uses different rules for formatting.

Let me know if it is now ready, I will merge and fix the lints issue after that.

And then we would release a new version to crates.io

@jerel
Copy link
Contributor Author

jerel commented Sep 10, 2021

@shekohex ah, ok. I was surprised that CI failed this last time because I ran clippy and fmt locally. Yes, it's ready as far as I'm concerned.

@shekohex shekohex merged commit 5fe0cdf into shekohex:master Sep 10, 2021
@shekohex
Copy link
Owner

Thanks, Merged 🎉

@shekohex
Copy link
Owner

Available now since v0.1.9.

Thanks, everyone!

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

Successfully merging this pull request may close these issues.

None yet

3 participants