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

Proposal: use rust Layout to calculate Sample Layout #459

Open
xieyuschen opened this issue Oct 10, 2024 · 2 comments
Open

Proposal: use rust Layout to calculate Sample Layout #459

xieyuschen opened this issue Oct 10, 2024 · 2 comments

Comments

@xieyuschen
Copy link
Contributor

Method sample_layout calculates the layout by appending and call functions with unsafe.

pub(crate) fn sample_layout(&self, number_of_elements: usize) -> Layout {
unsafe {
Layout::from_size_align_unchecked(
align(
self.header.size + self.user_header.size + self.user_header.alignment - 1
+ self.payload.size * number_of_elements
+ self.payload.alignment
- 1,
self.header.alignment,
),
self.header.alignment,
)
}
}

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
Copy link
Contributor

@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
Copy link
Contributor Author

@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.

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

No branches or pull requests

2 participants