PacketBuffer rewrite #9

Boddlnagg · 2017-07-20T10:16:54Z

This adresses the points discussed in #5. Details:

Methods that add package data now take the data as &[u8] to not require the caller to allocate a Vec
Creating a buffer with at most 28 bytes of data (minus the headers this corresponds to e.g. a single packet with 14 bytes of data), will not allocate on the heap, but store the buffer inline on the stack. The inline size could be increased at the cost of growing the PacketBuffer type.
The implementation ensures correct 4-byte alignment of packets on ARM.
The empty new() constructor has been removed, because having the buffer always contain at least one package allows some optimizations and I can't see reason for ever needing an empty PacketBuffer. new now has the signature of from_data instead.
I renamed with_data to push_data, because I think of it more as similar to the Vec API instead of a builder API. push_data still returns &mut self, so it can be chained. If you want me to undo these API changes, I can do that.
I renamed PacketList::length() to len() to make it consistent with Rust's standard library (all the methods to get the length of something are called len(), see https://static.rust-lang.org/doc/master/std/index.html?search=length)
Multiple non-sysex packets with the same timestamp are now combined into a single packet by push_data. This matches the behavior of MIDIPacketListAdd, and there are unit tests to ensure this (by comparing the result of using MIDIPacketListAdd vs. PacketBuffer).

This PR also includes an upgrade to version 2.0 of coremidi-sys (the first two commits), getting rid of coremidi_sys_ext. If you want to have these as a seperate PR, I can do that, too.

chris-zen · 2017-09-03T10:13:22Z

src/lib.rs

+
+
+#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+type AlignmentMarker = [u8; 0]; // unaligned


What about other architectures than arm or x86 ? would it be possible to have the unaligned case for all the non arm-aarch64 architectures ?

Yes, that would be possible. I'm just not sure wheter "unaligned" is the right default, so I did it in such a way that it would error on other architectures. Is macOS/iOS even available for other architectures?

Is macOS/iOS even available for other architectures?

Good point. I guess that failing in case of unknown architecture is an acceptable solution here.

chris-zen · 2017-09-03T10:15:08Z

src/lib.rs

+    //       pointing to valid instances of MIDIPacketList.
+    //       This type must NOT implement `Copy`!
+    inner: PacketListInner,
+    _do_not_construct: AlignmentMarker


is _do_not_construct a convention name for such cases ? If not, could you rename to something like _alignment_marker ?

It's not a convention. I just did this to solve two problems at once, namely (1) to have the correct alignment and (2) to give a strong hint that any code that would try to construct an instance of this type would be wrong (but this is only relevant within the library, not for users of the library). I can rename if you prefer.

Not feeling strong about this renaming, but given that it is not exposed to the public through the API, (1) is the main point to hint, so the proposed name looks more intuitive to me. You can focus on other comments first, and if having enough time do this ;-)

chris-zen · 2017-09-03T10:17:20Z

src/lib.rs

+#[repr(packed)]
+struct PacketListInner {
+    num_packets: u32,
+    data: [MIDIPacket; 0]


I am wondering if the differentiation between PacketList and PacketListInner is really necessary. Couldn't we just put the alignment marker after the data field and rename PacketListInner to PacketList ?

Yes, PacketListInner is necessary, because of a lack of the #[repr(packed = "N")] feature in Rust (see rust-lang/rust#33158). We want an alignment of 4 on ARM and 1 on other architectures, but #[repr(packed)] always gives an alignment of 1. So I'm defining PacketListInner with an alignment of 1, and using that within PacketList together with AlignmentMarker, so PacketList will choose the alignment as max(alignof(PacketListInner), alignof(AlignmentMarker)), which results in the correct alignment.

chris-zen · 2017-09-03T10:19:42Z

src/packets.rs

+    //       This type must NOT implement `Copy`!
+    //       On ARM, this must be 4-byte aligned.
+    inner: PacketInner,
+    _do_not_construct: AlignmentMarker


Same comments as with PacketList/PacketListInner

Instead of storing a pointer, use references to PacketType directly, and disallow the creation of PacketTpye values.

Main improvement is that small packet lists are allocated directly on the stack. When adding packets to a list, it is now also possible for them to be merged with previous packets (same behaviour as MIDIPacketListAdd).

This is consistent with the naming in Rust's standard library.

Boddlnagg · 2017-09-03T13:40:14Z

The merge commit that you pushed introduced a failure. I force-pushed a rebased version with the fix (let's see if Travis agrees that it's correct 😉 ). And thanks for the review!

chris-zen · 2017-09-03T14:19:55Z

src/packets.rs

+
+    /// Call this only with larger length values (won't make the buffer smaller)
+    unsafe fn set_len(&mut self, new_length: usize) {
+        if new_length < INLINE_PACKET_BUFFER_SIZE { return; }


If it is an External case it could have a length shorter than INLINE_PACKET_BUFFER_SIZE, right ?

The way I've implemented it, the External case will only ever be created with sizes larger than INLINE_PACKET_BUFFER_SIZE (see the implementation of new).

I should probably document this somewhere ...

chris-zen · 2017-09-03T14:38:33Z

src/packets.rs

+    fn last_written_packet(&self) -> &Packet {
+        // NOTE: This requires that there always is at least one packet in the buffer
+        //       (which is okay because we do not provide an empty constructor)
+        unsafe { &*(self.storage.get_slice()[self.last_written_pkt_offset..].as_ptr() as *const Packet) }


Could you please avoid the one-liner here and use intermediary vars to make it more readable ?

chris-zen · 2017-09-03T14:40:46Z

src/packets.rs

-        pkt.timeStamp = timestamp as MIDITimeStamp;
-        pkt.length = data_len as UInt16;
-        pkt.data[0..data_len].clone_from_slice(&data);
+    pub fn push_packet(&mut self, time: MIDITimeStamp, data: &[u8]) -> &mut Self {


Does data represent a Packet or a list of MIDI bytes ? The name push_packet is confusing. I'd rename it to push_data to make this differentiation clear.

Following the same logic I would also rename assign_packet and extend_packet for the PacketBufferStorage.

assign_packet assigns data to a single MIDIPacket, extend_packet extends the data inside a single MIDIPacket, so I think that these names are okay. You are right that push_packet either creates a new packet or extends an existing one, so push_data is probably better. I will rename it.

I now remember why I didn't call it push_data from the beginning: It is required that the data that is being pushed in a single call of this function actually is a valid packet and not arbitrary MIDI bytes. E.g. it is not allowed to push a NOTE ON followed by a SYSEX event in a single "packet". Also see the original documentation of MIDIPacketListAdd. There, a "packet" is called an "event", so we could also call this push_event. I think I still prefer push_data now, under the condition that the documentation makes it clear what is allowed as data passed to the function.

chris-zen · 2017-09-03T14:43:20Z

src/packets.rs

+                previous_packet.data()[0] != 0xF0 && // previous packet not a sysex
+                previous_packet.data()[0] & 0b10000000 != 0 && // but first byte is a status byte
+                previous_data_len + data.len() < MAX_PACKET_DATA_LENGTH; // enough space left in the packet
+        }


Could this block be extracted into a private function ?

let (can_merge, previous_data_len) = check_mergeable(...)

chris-zen · 2017-09-03T14:47:16Z

src/packets.rs

+    fn get_next_offset(&self) -> usize {
+        let length = self.last_written_packet().inner.length as usize;
+        let next_unadjusted = self.last_written_pkt_offset + PACKET_HEADER_SIZE + length;
+        if cfg!(any(target_arch = "arm", target_arch = "aarch64")) {


I've seen this check several times by now. If by any chance we need to cover a new architecture that requires u32 alignment, we will need to change several of those checks which is prone to error. Do you think it could be extracted into a macro and used all around ?

if align_u32!() { ... }

I basically copied the logic from here. Trying to think about a way to extract this that works at the expression level (i.e. if cfg!(...) { ... }) and also on the item definition level (i.e. #[cfg(...)]), I can only come up with this:

#[cfg(any(target_arch = "arm", target_arch = "aarch64"))] mod alignment { type AlignmentMarker = [u32; 0]; const NEEDS_ALIGNMENT: bool = true; } #[cfg(not(any(target_arch = "arm", target_arch = "aarch64")))] mod alignment { type AlignmentMarker = [u8; 0]; const NEEDS_ALIGNMENT: bool = false; }

... and then using if alignment::NEEDS_ALIGNMENT { ... }

Would that be okay?

That's a very interesting approach. I like it.

chris-zen

@Boddlnagg I really appreciate the effort you have put on this improvement. I finally had some time to review and learn from it. I added some more comments.

I really like you updated to coremidi-sys 2.0, I didn't realised of the new release. And also the approach of using an enum for the storage to cover inline and external cases.

Given that it touches some code that was contributed by @raphlinus previously, I would like to give him some time to review too (or decline if he is too busy ;-).

Boddlnagg · 2017-09-03T15:24:56Z

@chris-zen Thanks for the review! I will push a commit with the changes that you requested (is it okay if I just put them all in a single commit?). Actually, I am responsible for the 2.0 version of coremidi-sys, after looking at this library and talking to the developer of coremidi-sys 1.0, who happens to be a friend of mine 😉

raphlinus

I had a quick look, and everything looks good to me. I haven't had a chance to test it with my synthesizer though.

Glad to see interest in this type of thing picking up!

Boddlnagg · 2017-09-04T15:49:07Z

I pushed a commit addressing the review comments

chris-zen

Thank you very much ! Great work ;-)

Boddlnagg · 2017-09-14T06:23:45Z

Thanks! Do you plan to release a new version soon? Then I will be able to update midir :-)

chris-zen · 2017-09-25T01:54:17Z

Done. Released 0.3.0

chris-zen reviewed Sep 3, 2017

View reviewed changes

Boddlnagg added 8 commits September 3, 2017 15:38

Use upgraded version of coremidi-sys

afc9f20

Depend on now-published version of sys crate

fbfdae7

Add more packet tests

45d0044

Change semantics of PacketList type

30f4054

Instead of storing a pointer, use references to PacketType directly, and disallow the creation of PacketTpye values.

Change semantics of Packet in the same way as PacketList

effc202

Simplify access to PacketList fields and validate layout

f554dd3

PacketBuffer rewrite

07983d5

Main improvement is that small packet lists are allocated directly on the stack. When adding packets to a list, it is now also possible for them to be merged with previous packets (same behaviour as MIDIPacketListAdd).

Rename PacketList::length -> len

f72086a

This is consistent with the naming in Rust's standard library.

Boddlnagg force-pushed the buffer-rewrite branch from f915b06 to f72086a Compare September 3, 2017 13:39

chris-zen reviewed Sep 3, 2017

View reviewed changes

raphlinus approved these changes Sep 3, 2017

View reviewed changes

Addressed review comments & cleanup

2e6b442

chris-zen approved these changes Sep 13, 2017

View reviewed changes

chris-zen merged commit 6087394 into chris-zen:master Sep 13, 2017

Boddlnagg mentioned this pull request May 9, 2024

misaligned pointer dereference error in PacketListIterator::next #49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PacketBuffer rewrite #9

PacketBuffer rewrite #9

Boddlnagg commented Jul 20, 2017 •

edited

Loading

chris-zen Sep 3, 2017

Boddlnagg Sep 3, 2017

chris-zen Sep 3, 2017 •

edited

Loading

chris-zen Sep 3, 2017

Boddlnagg Sep 3, 2017 •

edited

Loading

chris-zen Sep 3, 2017

chris-zen Sep 3, 2017

Boddlnagg Sep 3, 2017

chris-zen Sep 3, 2017

Boddlnagg commented Sep 3, 2017 •

edited

Loading

chris-zen Sep 3, 2017

Boddlnagg Sep 3, 2017

Boddlnagg Sep 3, 2017

chris-zen Sep 3, 2017

chris-zen Sep 3, 2017

chris-zen Sep 3, 2017

chris-zen Sep 3, 2017

Boddlnagg Sep 3, 2017

Boddlnagg Sep 3, 2017

chris-zen Sep 3, 2017

chris-zen Sep 3, 2017

Boddlnagg Sep 3, 2017 •

edited

Loading

chris-zen Sep 3, 2017

chris-zen left a comment

Boddlnagg commented Sep 3, 2017

raphlinus left a comment

Boddlnagg commented Sep 4, 2017

chris-zen left a comment

Boddlnagg commented Sep 14, 2017

chris-zen commented Sep 25, 2017



		#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
		type AlignmentMarker = [u8; 0]; // unaligned

PacketBuffer rewrite #9

PacketBuffer rewrite #9

Conversation

Boddlnagg commented Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-zen Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Boddlnagg Sep 3, 2017 • edited Loading

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

Boddlnagg commented Sep 3, 2017 • edited Loading

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

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

Boddlnagg Sep 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-zen left a comment

Choose a reason for hiding this comment

Boddlnagg commented Sep 3, 2017

raphlinus left a comment

Choose a reason for hiding this comment

Boddlnagg commented Sep 4, 2017

chris-zen left a comment

Choose a reason for hiding this comment

Boddlnagg commented Sep 14, 2017

chris-zen commented Sep 25, 2017

Boddlnagg commented Jul 20, 2017 •

edited

Loading

chris-zen Sep 3, 2017 •

edited

Loading

Boddlnagg Sep 3, 2017 •

edited

Loading

Boddlnagg commented Sep 3, 2017 •

edited

Loading

Boddlnagg Sep 3, 2017 •

edited

Loading