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

Make zero-copy and ZeroCopyBuffer work with Vec<T> of length 0 #48

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Make zero-copy and ZeroCopyBuffer work with Vec<T> of length 0 #48

merged 4 commits into from
Sep 22, 2023

Conversation

temeddix
Copy link
Contributor

@temeddix temeddix commented Sep 22, 2023

Summary

When enabling the zero-copy feature or using ZeroCopyBuffer, Vec<T> with length of 0 always made the Dart-Rust app crash. This turned out to be a behavior with isolate_callback_data, which returns proper length value when the length of Vec<T> was not 0, but returns an oddly big number when Vec<T> has zero length.

cunarist/rinf#165

Experiment

Adding some println!()s can help us inspect this problem.

// into_dart.rs

fn vec_to_dart_native_external_typed_data<T>(
    vec_from_rust: Vec<T>,
) -> DartCObject
where
    T: DartTypedDataTypeTrait,
{
    let mut vec = ManuallyDrop::new(vec_from_rust);
    vec.shrink_to_fit();
    let length = vec.len();
    assert_eq!(length, vec.capacity());
    let ptr = vec.as_mut_ptr();
    println!("{:?}, {:?}", ptr, length); // THIS LINE

    DartCObject {
        ty: DartCObjectType::DartExternalTypedData,
        value: DartCObjectValue {
            as_external_typed_data: DartNativeExternalTypedData {
                ty: T::dart_typed_data_type(),
                length: length as isize,
                data: ptr as *mut u8,
                peer: ptr as *mut c_void,
                callback: T::function_pointer_of_free_zero_copy_buffer(),
            },
        },
    }
}
// into_dart.rs

            #[doc(hidden)]
            #[no_mangle]
            pub(crate) unsafe extern "C" fn $free_zero_copy_buffer_func(
                isolate_callback_data: *mut c_void,
                peer: *mut c_void,
            ) {
                let len = (isolate_callback_data as isize) as usize;
                let ptr = peer as *mut $rust_type;
                println!("{:?}, {:?}", ptr, len); // THIS LINE
                drop(Vec::from_raw_parts(ptr, len, len));
            }
        )+

When passing in Vec<T> of 6 bytes with zero-copy feature enabled(or using ZeroCopyBuffer), we get this output:

0x1, 6
0x1, 6

When passing in Vec<T> of 0 bytes with zero-copy feature enabled(or using ZeroCopyBuffer), we get this output:

0x1, 0
0x1, 140208384468704

The length value from Rust Vec<T> is correct, but the length value from isolate_callback_data is very weird. This led to deallocating memory of a totally wrong location which always made the Dart-Rust app crash.

I've confirmed that this change effectively works on an actual project that uses Vec<T> of length 0 with zero-copy. Tests are also added in this PR.

@temeddix temeddix changed the title Make zero-copy work with Vec of length 0 Make zero-copy work with Vec<T> of length 0 Sep 22, 2023
@temeddix temeddix changed the title Make zero-copy work with Vec<T> of length 0 Make zero-copy and ZeroCopyBuffer work with Vec<T> of length 0 Sep 22, 2023
@shekohex
Copy link
Owner

So casting to isize made that zero number to be a very big number like 140208384468704? huh weird, I guess thats why using as is bad.

Copy link
Owner

@shekohex shekohex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shekohex
Copy link
Owner

Did you test this branch with your code? is that fixes your bug?

@temeddix
Copy link
Contributor Author

I tested with the code provided in this issue, which should obviously work. The code just simply involved passing zero-length Vec<T>s. I confirmed that with my forked allo-isolate, zero-length Vec<T> with zero-copy works properly.

@shekohex shekohex merged commit 6281240 into shekohex:master Sep 22, 2023
5 checks passed
@shekohex
Copy link
Owner

Will publish a new version very soon.

@temeddix
Copy link
Contributor Author

Thanks for notifying :)

@shekohex
Copy link
Owner

Published as v0.1.20 🎉

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.

2 participants