Expose Quality of Service settings (Priority, Congestion Control, Express) the Sample was published with #730
Conversation
…ype to Sample::QoS
…ol priority to user one
|
Holding on in merging this PR since it will break the memory layout for zenoh-c and zenoh-cpp at least. |
| @@ -326,6 +329,8 @@ pub struct Sample { | |||
| pub kind: SampleKind, | |||
| /// The [`Timestamp`] of this Sample. | |||
| pub timestamp: Option<Timestamp>, | |||
| /// Quality of service settings this sample was sent with. | |||
| pub qos: QoS, | |||
There was a problem hiding this comment.
I'm not a huge fan of how Sample keeps on getting more public fields. Maybe we can use the API break to make it a bit more private?
|
|
||
| /// Structure containing quality of service data | ||
| #[derive(Debug, Default, Copy, Clone, Eq, PartialEq)] | ||
| pub struct QoS { |
There was a problem hiding this comment.
Using accessors, this could be a bitfield and occupy a single byte instead of 3
There was a problem hiding this comment.
What is the benefit of reducing 3 bytes to one on the system with word size >= 4 bytes ? As a separate struct it will still tend to occupy 4 bytes at least, no ?
There was a problem hiding this comment.
It's all about breakpoints and, to be fair, I think this layout just barely fits in Sample's current padding, but that also means the next field is guaranteed to make Sample fatter, which we've shown in previous layout improvements to be rather performances sensitive for us.
API-wise, it's tempting to have only public fields, but that turns any future change to the struct's fields an API break. Builder patterns are just as convenient and give us much more flexibility in the future, both for layout optimization and for field additions.
There was a problem hiding this comment.
@Mallets @OlivierHecart can you consider my proposal above of moving Sample to an accessor pattern?
I think the proposal is in principle ok. The question is: should this PR be blocked by the proposal or should we merge it while we rework |
|
I'm fine with either, but I'd like at least Keep in mind that if we unblock them with a first branch, they'll have to deal with the breaking change immediately after. |
I'm fine with your proposal for Rust. Similar PRs are available in:
|
- remove inner type accessor
Expose Quality of Service settings (Priority, Congestion Control, Express flag) the sample was sent with (closes #694)