Iox 36 untyped api #58

elBoberido · 2022-07-15T14:27:37Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes Create an untyped API that works on [u8] slices #36

elBoberido · 2022-07-15T14:30:31Z

examples/publisher_untyped.rs

+        let mut sample = publisher.loan(std::mem::size_of::<u32>(), std::mem::align_of::<u32>())?;
+        sample.as_mut().put_u32(counter);
+        publisher.publish(sample);


@neilisaac does this fit your use case?

I think I will introduce a loan_uninitialized and let loan zero the memory.

Yes, this looks great!

For reference, what I had was:

pub struct PubChunkHandle<'a> { data: *mut ffi::c_void, publisher: &'a Publisher, sent: bool, // used in Drop size: usize, } impl PubChunkHandle<'_> { /// Send the data via the associated Publisher pub fn publish(mut self) { unsafe { sys::iox_pub_publish_chunk(self.publisher.0, self.data) }; self.sent = true; } /// Safe interface to write into a shared memory chunk. pub fn as_uninit_slice(&mut self) -> &mut UninitSlice { unsafe { UninitSlice::from_raw_parts_mut(self.data.cast(), self.size) } } /// Expose the payload portion of the chunk as a mutable byte slice. /// This can be written to conveniently using bytes::BufMut. /// # Safety /// Despite this function not being marked as unsafe, the data returned by as_slice is uninitialized. pub fn as_slice(&mut self) -> &mut [u8] { unsafe { std::slice::from_raw_parts_mut(self.data.cast(), self.size) } } /// Cast the chunk to a given type T. /// This may be useful with a #[repr(C)] type for example. /// # Safety /// This function is marked as unsafe because it is the caller's responsibility /// to ensure that T can safely be written into the chunk. /// T must fit in the allocated chunk and must not reference data outside the chunk (ex. via pointers/references) pub unsafe fn as_uninitialized<T>(&mut self) -> &mut std::mem::MaybeUninit<T> { &mut *self.data.cast() } }

The main design difference there I think is the PubChunkHandle borrows the Publisher, but otherwise very similar.

Nice. Currently I plan to have this methods

pub fn loan_zeroed_slice(&self, len: usize) -> Result<SampleMut<[u8]>, IceoryxError> { unimplemented!() } pub fn loan_aligned_zeroed_slice(&self, len: usize, alignment: usize) -> Result<SampleMut<[u8]>, IceoryxError> { unimplemented!() } pub fn loan_uninit_slice(&self, len: usize) -> Result<SampleMut<[MaybeUninit<u8>]>, IceoryxError> { unimplemented!() } pub fn loan_aligned_uninit_slice(&self, len: usize, alignment: usize) -> Result<SampleMut<[MaybeUninit<u8>]>, IceoryxError> { unimplemented!() }

I have to check the ergonomics with [MaybeUninit<u8>]. Maybe this needs to be done in a different way.

It would also be possible to not restrict the API to [u8] slices but to allow all types with T: ShmSend. With this one can have dynamic data at runtime. At least the most simple one.

@elfenpiff what do you think about having a specialization Sample<cxx::span<T>> for the typed C++ API?

neilisaac · 2022-07-15T15:08:37Z

src/publisher.rs

@@ -10,15 +10,15 @@ use crate::IceoryxError;
 use std::marker::PhantomData;
 use std::mem::MaybeUninit;

-pub struct PublisherBuilder<'a, T: ShmSend> {
+pub struct PublisherBuilder<'a, T: ShmSend + ?Sized> {


is ?Sized accepted in a trait declaration ex. pub unsafe trait ShmSend: ?Sized {}, or is it special because it's an anti-constraint?

traits are ?Sized by default and one would have to set Sized as trait bounds. But since I want to have [u8] as type I cannot do it.

neilisaac · 2022-07-15T15:14:08Z

examples/publisher_untyped.rs

+        let mut sample = publisher.loan(std::mem::size_of::<u32>(), std::mem::align_of::<u32>())?;
+        sample.as_mut().put_u32(counter);
+        publisher.publish(sample);


Yes, this looks great!

For reference, what I had was:

pub struct PubChunkHandle<'a> { data: *mut ffi::c_void, publisher: &'a Publisher, sent: bool, // used in Drop size: usize, } impl PubChunkHandle<'_> { /// Send the data via the associated Publisher pub fn publish(mut self) { unsafe { sys::iox_pub_publish_chunk(self.publisher.0, self.data) }; self.sent = true; } /// Safe interface to write into a shared memory chunk. pub fn as_uninit_slice(&mut self) -> &mut UninitSlice { unsafe { UninitSlice::from_raw_parts_mut(self.data.cast(), self.size) } } /// Expose the payload portion of the chunk as a mutable byte slice. /// This can be written to conveniently using bytes::BufMut. /// # Safety /// Despite this function not being marked as unsafe, the data returned by as_slice is uninitialized. pub fn as_slice(&mut self) -> &mut [u8] { unsafe { std::slice::from_raw_parts_mut(self.data.cast(), self.size) } } /// Cast the chunk to a given type T. /// This may be useful with a #[repr(C)] type for example. /// # Safety /// This function is marked as unsafe because it is the caller's responsibility /// to ensure that T can safely be written into the chunk. /// T must fit in the allocated chunk and must not reference data outside the chunk (ex. via pointers/references) pub unsafe fn as_uninitialized<T>(&mut self) -> &mut std::mem::MaybeUninit<T> { &mut *self.data.cast() } }

The main design difference there I think is the PubChunkHandle borrows the Publisher, but otherwise very similar.

codecov-commenter · 2022-07-15T17:48:33Z

Codecov Report

Merging #58 (1e0284e) into master (a37117a) will increase coverage by 3.73%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   51.29%   55.02%   +3.73%     
==========================================
  Files          19       20       +1     
  Lines         774      945     +171     
==========================================
+ Hits          397      520     +123     
- Misses        377      425      +48

Impacted Files	Coverage Δ
src/error.rs	`0.00% <ø> (ø)`
src/introspection/memory.rs	`0.00% <ø> (ø)`
src/introspection/port.rs	`0.00% <ø> (ø)`
src/introspection/process.rs	`0.00% <ø> (ø)`
src/subscriber.rs	`47.22% <ø> (ø)`
src/sample_mut.rs	`70.00% <65.38%> (-17.50%)`	⬇️
src/sample.rs	`59.40% <69.56%> (+4.40%)`	⬆️
src/publisher.rs	`62.75% <76.78%> (+3.35%)`	⬆️
iceoryx-sys/src/publisher.rs	`81.96% <95.12%> (+1.57%)`	⬆️
iceoryx-sys/src/chunk_header.rs	`95.23% <95.23%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a37117a...1e0284e. Read the comment docs.

elBoberido · 2022-07-15T18:27:27Z

examples/publisher_untyped.rs

+        let sample = if counter % 2 == 1 {
+            // with initialized slice
+            let mut sample = publisher.loan_slice(std::mem::size_of::<u32>())?;
+            sample.as_mut().put_u32(counter);
+            sample
+        } else {
+            // with uninitialized slice
+            let sample = publisher.loan_uninit_slice(std::mem::size_of::<u32>())?;
+            unsafe {
+                let mut sample = sample.assume_init();
+                sample.as_mut().put_u32(counter);
+                sample
+            }
+        };
+        publisher.publish(sample);


@neilisaac what is your opinion? Maybe a bit awkward to call assume_init before the buffer is written but not that bad.

An alternative would be

let sample = publisher.loan_uninit_slice(std::mem::size_of::<u32>())?; let sample = unsafe { sample.slice_assume_init_mut().put_u32(counter); sample.assume_init() } publisher.publish(sample);

elBoberido · 2022-07-17T17:56:07Z

examples/publisher_untyped.rs

+        let sample = if counter % 2 == 1 {
+            // with initialized slice
+            let mut sample = publisher.loan_slice_with_alignment(
+                std::mem::size_of::<u32>(),
+                std::mem::align_of::<u32>(),
+            )?;
+            sample.as_mut().put_u32_le(counter);
+            sample
+        } else {
+            // with uninitialized slice
+            let mut sample = publisher.loan_uninit_slice_with_alignment(
+                std::mem::size_of::<u32>(),
+                std::mem::align_of::<u32>(),
+            )?;
+            unsafe {
+                sample.slice_assume_init_mut().put_u32_le(counter);
+                sample.assume_init()
+            }
+        };
+        publisher.publish(sample);


@neilisaac if there are no objections from you, I'll go with this API for the publisher

elBoberido · 2022-07-17T17:56:47Z

examples/subscriber_untyped.rs

+    let (subscriber, sample_receive_token) =
+        SubscriberBuilder::<[u8]>::new("Radar", "FrontLeft", "Counter")
+            .queue_capacity(5)
+            .create()?;
+
+    let sample_receiver = subscriber.get_sample_receiver(sample_receive_token);
+
+    loop {
+        if sample_receiver.has_data() {
+            while let Some(sample) = sample_receiver.take() {
+                println!("Receiving: {}", sample.as_ref().get_u32_le());
+            }


and with this for the subscriber

neilisaac · 2022-07-22T00:28:09Z

examples/publisher_untyped.rs

+            )?;
+            unsafe {
+                sample.slice_assume_init_mut().put_u32_le(counter);
+                sample.assume_init()


loan_slice_with_alignment seems sensible.

loan_uninit_slice_with_alignment doesn't seem to really provide a significant distinction here compared to loan_slice_with_alignment, since loan_slice_with_alignment returns an uninitialized slice without requiring unsafe handling.

The way I did it was also a little loose about initialization safety since it's always up to the user to populate the buffer with something meaningful before publishing regardless of how it's abstracted.

The publisher could loan a PubChunkHandle which owns the data and exposes different views for convenience (depending on how the user wants to write into the buffer). Regardless of which method is used, the PubChunkHandle owns the data and can publish it at will.

impl Publisher { pub fn loan_chunk(&self, size: usize) -> Result<PubChunkHandle, AllocationError> } pub struct PubChunkHandle<'a> { data: *mut ffi::c_void, publisher: &'a Publisher, sent: bool, size: usize, } impl PubChunkHandle<'_> { pub fn publish(mut self) pub fn as_uninit_slice(&mut self) -> &mut UninitSlice pub fn as_slice(&mut self) -> &mut [u8] pub unsafe fn as_uninitialized<T>(&mut self) -> &mut std::mem::MaybeUninit<T> }

The last one is probably a bit sketchy since the user data could probably overflow, and it's easy to accidentally use a type the contains pointers.

@neilisaac I think you overlooked the .fill_with(|| T::default()); in loan_slice_with_alignment which initializes the slice. loan_uninit_slice_with_alignment doesn't do this but is unsafe. I tried to match the proposed API for uninitialized constructors of Box, Rc, and Arc.

I like the idea to expose a different view and will add this for [u8]

unsafe fn try_as_uninit<T: ShmSend>(&mut self) -> Option<&mut MaybeUninit<T>>

This would return None if the size and alignment do not match the requirements for T.

@neilisaac I added the convenience functions and if you do not have any concerns, I would merge the PR

neilisaac

API looks good to me!

elBoberido · 2022-07-24T19:42:48Z

@neilisaac I'm currently implementing the reactor pattern for the WaitSet/Listener. If you are interested, I can ping you once I have a draft PR.

elBoberido commented Jul 15, 2022

View reviewed changes

elBoberido force-pushed the iox-36-untyped-API branch from fdd839e to 3b8f145 Compare July 15, 2022 14:32

neilisaac reviewed Jul 15, 2022

View reviewed changes

elBoberido force-pushed the iox-36-untyped-API branch from 213b49d to 9050861 Compare July 15, 2022 17:43

elBoberido force-pushed the iox-36-untyped-API branch from 9050861 to 83386f4 Compare July 15, 2022 18:25

elBoberido commented Jul 15, 2022

View reviewed changes

elBoberido added 8 commits July 17, 2022 18:27

iox-#36 Add functionality to publish slices

bcafe23

iox-#36 Add untyped publisher example

bc9f27b

iox-#36 Rename loan_uninitialized to loan_uninit

733c891

iox-#36 Adjust sys API to use raw pointer

61bc7a0

iox-#36 Fix clippy warnings

9bade79

iox-#36 Add functionality to receive slices

dfd43b9

iox-#36 Add untyped subscriber example

ce2f8d9

iox-#36 Add tests for slice publishing and receiving

a0f12f4

elBoberido force-pushed the iox-36-untyped-API branch from 83386f4 to a0f12f4 Compare July 17, 2022 17:45

iox-#36 Fix typos

03801d3

elBoberido commented Jul 17, 2022

View reviewed changes

elBoberido marked this pull request as ready for review July 17, 2022 17:57

neilisaac reviewed Jul 22, 2022

View reviewed changes

elBoberido added 3 commits July 22, 2022 17:11

iox-#36 Create ChunkHeader access

4aabf6f

iox-#36 Rename chunk to payload where appropriate

8c2703a

iox-#36 Add try_as and try_as_uninit

1e0284e

neilisaac approved these changes Jul 24, 2022

View reviewed changes

elBoberido merged commit ff36d1e into master Jul 24, 2022

elBoberido deleted the iox-36-untyped-API branch July 24, 2022 19:37

elBoberido self-assigned this Jul 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox 36 untyped api #58

Iox 36 untyped api #58

elBoberido commented Jul 15, 2022 •

edited

elBoberido Jul 15, 2022

neilisaac Jul 15, 2022

elBoberido Jul 15, 2022

neilisaac Jul 15, 2022

elBoberido Jul 15, 2022

neilisaac Jul 15, 2022

codecov-commenter commented Jul 15, 2022 •

edited

elBoberido Jul 15, 2022

elBoberido Jul 15, 2022

elBoberido Jul 17, 2022

elBoberido Jul 17, 2022

neilisaac Jul 22, 2022

elBoberido Jul 22, 2022

elBoberido Jul 22, 2022

neilisaac left a comment

elBoberido commented Jul 24, 2022

Iox 36 untyped api #58

Iox 36 untyped api #58

Conversation

elBoberido commented Jul 15, 2022 • edited

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 15, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilisaac left a comment

Choose a reason for hiding this comment

elBoberido commented Jul 24, 2022

elBoberido commented Jul 15, 2022 •

edited

codecov-commenter commented Jul 15, 2022 •

edited