eclipse-iceoryx / iceoryx2

Eclipse iceoryx2™ - true zero-copy inter-process-communication in pure Rust
https://iceoryx.io
Apache License 2.0
1.03k stars 40 forks source link

Proposal: use rust Layout to calculate Sample Layout #459

Open xieyuschen opened 1 month ago

xieyuschen commented 1 month ago

Method sample_layout calculates the layout by appending and call functions with unsafe. https://github.com/eclipse-iceoryx/iceoryx2/blob/ca22976c4bce5f9b92c53e3a0e1571ad9d8a086d/iceoryx2/src/service/static_config/message_type_details.rs#L104-L118

I'm not sure why we use this approach to calculate layout(probably to keep the same with iceoryx in c++). I found we can rely on rust to calculate the layout as if we define a structure for the whole sample. Hence, sample_layout_genric::<i32, bool, i64>(2) works as if we define a structure S1:

        #[repr(C)]
        struct S1 {
            _a: i32,
            _user_header: bool,
            _layout: [i64; 2],
        }

The sample_layout_genric is shown below:

    pub(crate) fn sample_layout_genric<Header, UserHeader, Payload>(&self, n: usize) -> Layout {
        let layout_header = Layout::new::<Header>();
        let layout_user_header = Layout::new::<UserHeader>();
        let layout_array = Layout::array::<Payload>(n).ok().unwrap();
        layout_header
            .extend(layout_user_header)
            .ok()
            .unwrap()
            .0
            .extend(layout_array)
            .ok()
            .unwrap()
            .0
            .pad_to_align()
    }

I have the committed the change for you to check in my forked repo.

Do you think it's a better solution comparing with current one? Note that I don't know whether we have the compatible concerns, so please let me know if the underlying layout calculation is crucial.

@elfenpiff @elBoberido @orecham

elfenpiff commented 1 month ago

@xieyuschen it seems that I can look at your proposal earliest the beginning of next week. But when wrote this piece of code I was still coming from the C++ world and was unaware of extend or pad_to_align in Layout so most likely your approach is the more idiomatic one.

If all corner cases are tested and the tests are passed for both approaches I would prefer your proposal.

xieyuschen commented 1 month ago

@xieyuschen it seems that I can look at your proposal earliest the beginning of next week. But when wrote this piece of code I was still coming from the C++ world and was unaware of extend or pad_to_align in Layout so most likely your approach is the more idiomatic one.

If all corner cases are tested and the tests are passed for both approaches I would prefer your proposal.

Acked. The calculated layouts are different between the current approach and my proposal, so that's probably a concern for the compatibility issue. I will try to revise the existing code for the project to test, so we can know more about this approach when you dive into it next week.