From 88bf7d22337c39f47a17749c13f5f3f9adfc3723 Mon Sep 17 00:00:00 2001 From: whitequark Date: Sun, 26 Feb 2017 18:20:46 +0000 Subject: [PATCH] firmware: specialize protocol read/write functions. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this commit, proto::io::{read,write}_* functions were taking a &mut {Read,Write}. This means a lot of virtual dispatch. After this commit, all these functions are specialized for the specific IO trait. This could be achieved with just changing the signature from fn read_x(reader: &mut Read) to fn read_x(reader: &mut R) but the functions were also grouped into ReadExt and WriteExt traits as a refactoring. Initially, it was expected that the generic traits from the byteorder crate could be used, but they require endianness to be specified on every call and thus aren't very ergonomic. They also lack the equivalent to our read_string and read_bytes. Thus, it seems fine to just define a slightly different extension trait. This also optimized the test_rpc_timing test: 1.7ms→1.2ms. --- artiq/firmware/ksupport/lib.rs | 2 +- artiq/firmware/libproto/analyzer_proto.rs | 12 +-- artiq/firmware/libproto/io.rs | 75 ---------------- artiq/firmware/libproto/lib.rs | 85 +++++++++++++++++- artiq/firmware/libproto/moninj_proto.rs | 36 ++++---- artiq/firmware/libproto/rpc_proto.rs | 36 ++++---- artiq/firmware/libproto/session_proto.rs | 102 +++++++++++----------- artiq/firmware/runtime/rtio_dma.rs | 16 ++-- 8 files changed, 185 insertions(+), 179 deletions(-) delete mode 100644 artiq/firmware/libproto/io.rs diff --git a/artiq/firmware/ksupport/lib.rs b/artiq/firmware/ksupport/lib.rs index 3a0e7e881..52cf287e7 100644 --- a/artiq/firmware/ksupport/lib.rs +++ b/artiq/firmware/ksupport/lib.rs @@ -126,7 +126,7 @@ extern fn rpc_send_async(service: u32, tag: CSlice, data: *const *const ()) rpc_proto::send_args(&mut writer, service, tag.as_ref(), data)?; writer.position() }; - proto::io::write_u32(&mut slice, length as u32) + proto::WriteExt::write_u32(&mut slice, length as u32) }).unwrap_or_else(|err| { assert!(err.kind() == std::io::ErrorKind::WriteZero); diff --git a/artiq/firmware/libproto/analyzer_proto.rs b/artiq/firmware/libproto/analyzer_proto.rs index 27151b3bf..45153fdd4 100644 --- a/artiq/firmware/libproto/analyzer_proto.rs +++ b/artiq/firmware/libproto/analyzer_proto.rs @@ -1,5 +1,5 @@ use std::io::{self, Write}; -use io::*; +use WriteExt; #[derive(Debug)] pub struct Header { @@ -12,11 +12,11 @@ pub struct Header { impl Header { pub fn write_to(&self, writer: &mut Write) -> io::Result<()> { - write_u32(writer, self.sent_bytes)?; - write_u64(writer, self.total_byte_count)?; - write_u8(writer, self.overflow_occurred as u8)?; - write_u8(writer, self.log_channel)?; - write_u8(writer, self.dds_onehot_sel as u8)?; + writer.write_u32(self.sent_bytes)?; + writer.write_u64(self.total_byte_count)?; + writer.write_u8(self.overflow_occurred as u8)?; + writer.write_u8(self.log_channel)?; + writer.write_u8(self.dds_onehot_sel as u8)?; Ok(()) } } diff --git a/artiq/firmware/libproto/io.rs b/artiq/firmware/libproto/io.rs deleted file mode 100644 index c91f606f1..000000000 --- a/artiq/firmware/libproto/io.rs +++ /dev/null @@ -1,75 +0,0 @@ -use std::io::{self, Read, Write}; -use std::vec::Vec; -use std::string::String; -use byteorder::{ByteOrder, NetworkEndian}; - -// FIXME: replace these with byteorder core io traits once those are in -pub fn read_u8(reader: &mut Read) -> io::Result { - let mut bytes = [0; 1]; - reader.read_exact(&mut bytes)?; - Ok(bytes[0]) -} - -pub fn write_u8(writer: &mut Write, value: u8) -> io::Result<()> { - let bytes = [value; 1]; - writer.write_all(&bytes) -} - -pub fn read_u16(reader: &mut Read) -> io::Result { - let mut bytes = [0; 2]; - reader.read_exact(&mut bytes)?; - Ok(NetworkEndian::read_u16(&bytes)) -} - -pub fn write_u16(writer: &mut Write, value: u16) -> io::Result<()> { - let mut bytes = [0; 2]; - NetworkEndian::write_u16(&mut bytes, value); - writer.write_all(&bytes) -} - -pub fn read_u32(reader: &mut Read) -> io::Result { - let mut bytes = [0; 4]; - reader.read_exact(&mut bytes)?; - Ok(NetworkEndian::read_u32(&bytes)) -} - -pub fn write_u32(writer: &mut Write, value: u32) -> io::Result<()> { - let mut bytes = [0; 4]; - NetworkEndian::write_u32(&mut bytes, value); - writer.write_all(&bytes) -} - -pub fn read_u64(reader: &mut Read) -> io::Result { - let mut bytes = [0; 8]; - reader.read_exact(&mut bytes)?; - Ok(NetworkEndian::read_u64(&bytes)) -} - -pub fn write_u64(writer: &mut Write, value: u64) -> io::Result<()> { - let mut bytes = [0; 8]; - NetworkEndian::write_u64(&mut bytes, value); - writer.write_all(&bytes) -} - -pub fn read_bytes(reader: &mut Read) -> io::Result> { - let length = read_u32(reader)?; - let mut value = Vec::new(); - value.resize(length as usize, 0); - reader.read_exact(&mut value)?; - Ok(value) -} - -pub fn write_bytes(writer: &mut Write, value: &[u8]) -> io::Result<()> { - write_u32(writer, value.len() as u32)?; - writer.write_all(value) -} - -pub fn read_string(reader: &mut Read) -> io::Result { - let bytes = read_bytes(reader)?; - String::from_utf8(bytes) - .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "invalid UTF-8")) -} - -pub fn write_string(writer: &mut Write, value: &str) -> io::Result<()> { - write_bytes(writer, value.as_bytes()) -} diff --git a/artiq/firmware/libproto/lib.rs b/artiq/firmware/libproto/lib.rs index 97d95adad..3ecf3c278 100644 --- a/artiq/firmware/libproto/lib.rs +++ b/artiq/firmware/libproto/lib.rs @@ -9,8 +9,6 @@ extern crate log; extern crate dyld; extern crate std_artiq as std; -pub mod io; - // Internal protocols. pub mod kernel_proto; @@ -19,3 +17,86 @@ pub mod analyzer_proto; pub mod moninj_proto; pub mod session_proto; pub mod rpc_proto; + +use std::io::{Read, Write, Result, Error, ErrorKind}; +use std::vec::Vec; +use std::string::String; +use byteorder::{ByteOrder, NetworkEndian}; + +pub trait ReadExt: Read { + fn read_u8(&mut self) -> Result { + let mut bytes = [0; 1]; + self.read_exact(&mut bytes)?; + Ok(bytes[0]) + } + + fn read_u16(&mut self) -> Result { + let mut bytes = [0; 2]; + self.read_exact(&mut bytes)?; + Ok(NetworkEndian::read_u16(&bytes)) + } + + fn read_u32(&mut self) -> Result { + let mut bytes = [0; 4]; + self.read_exact(&mut bytes)?; + Ok(NetworkEndian::read_u32(&bytes)) + } + + fn read_u64(&mut self) -> Result { + let mut bytes = [0; 8]; + self.read_exact(&mut bytes)?; + Ok(NetworkEndian::read_u64(&bytes)) + } + + fn read_bytes(&mut self) -> Result> { + let length = self.read_u32()?; + let mut value = Vec::new(); + value.resize(length as usize, 0); + self.read_exact(&mut value)?; + Ok(value) + } + + fn read_string(&mut self) -> Result { + let bytes = self.read_bytes()?; + String::from_utf8(bytes) + .map_err(|_| Error::new(ErrorKind::InvalidData, "invalid UTF-8")) + } +} + +impl ReadExt for R {} + +pub trait WriteExt: Write { + fn write_u8(&mut self, value: u8) -> Result<()> { + let bytes = [value; 1]; + self.write_all(&bytes) + } + + fn write_u16(&mut self, value: u16) -> Result<()> { + let mut bytes = [0; 2]; + NetworkEndian::write_u16(&mut bytes, value); + self.write_all(&bytes) + } + + fn write_u32(&mut self, value: u32) -> Result<()> { + let mut bytes = [0; 4]; + NetworkEndian::write_u32(&mut bytes, value); + self.write_all(&bytes) + } + + fn write_u64(&mut self, value: u64) -> Result<()> { + let mut bytes = [0; 8]; + NetworkEndian::write_u64(&mut bytes, value); + self.write_all(&bytes) + } + + fn write_bytes(&mut self, value: &[u8]) -> Result<()> { + self.write_u32(value.len() as u32)?; + self.write_all(value) + } + + fn write_string(&mut self, value: &str) -> Result<()> { + self.write_bytes(value.as_bytes()) + } +} + +impl WriteExt for W {} diff --git a/artiq/firmware/libproto/moninj_proto.rs b/artiq/firmware/libproto/moninj_proto.rs index f5d0940cd..de233d3c0 100644 --- a/artiq/firmware/libproto/moninj_proto.rs +++ b/artiq/firmware/libproto/moninj_proto.rs @@ -1,5 +1,5 @@ use std::io::{self, Read, Write}; -use io::*; +use {ReadExt, WriteExt}; #[derive(Debug)] pub enum HostMessage { @@ -16,20 +16,20 @@ pub enum DeviceMessage { impl HostMessage { pub fn read_from(reader: &mut Read) -> io::Result { - Ok(match read_u8(reader)? { + Ok(match reader.read_u8()? { 0 => HostMessage::Monitor { - enable: if read_u8(reader)? == 0 { false } else { true }, - channel: read_u32(reader)?, - probe: read_u8(reader)? + enable: if reader.read_u8()? == 0 { false } else { true }, + channel: reader.read_u32()?, + probe: reader.read_u8()? }, 1 => HostMessage::Inject { - channel: read_u32(reader)?, - overrd: read_u8(reader)?, - value: read_u8(reader)? + channel: reader.read_u32()?, + overrd: reader.read_u8()?, + value: reader.read_u8()? }, 2 => HostMessage::GetInjectionStatus { - channel: read_u32(reader)?, - overrd: read_u8(reader)? + channel: reader.read_u32()?, + overrd: reader.read_u8()? }, _ => return Err(io::Error::new(io::ErrorKind::InvalidData, "unknown packet type")) }) @@ -40,16 +40,16 @@ impl DeviceMessage { pub fn write_to(&self, writer: &mut Write) -> io::Result<()> { match *self { DeviceMessage::MonitorStatus { channel, probe, value } => { - write_u8(writer, 0)?; - write_u32(writer, channel)?; - write_u8(writer, probe)?; - write_u32(writer, value)?; + writer.write_u8(0)?; + writer.write_u32(channel)?; + writer.write_u8(probe)?; + writer.write_u32(value)?; }, DeviceMessage::InjectionStatus { channel, overrd, value } => { - write_u8(writer, 1)?; - write_u32(writer, channel)?; - write_u8(writer, overrd)?; - write_u8(writer, value)?; + writer.write_u8(1)?; + writer.write_u32(channel)?; + writer.write_u8(overrd)?; + writer.write_u8(value)?; } } Ok(()) diff --git a/artiq/firmware/libproto/rpc_proto.rs b/artiq/firmware/libproto/rpc_proto.rs index 0918e0637..a7424a905 100644 --- a/artiq/firmware/libproto/rpc_proto.rs +++ b/artiq/firmware/libproto/rpc_proto.rs @@ -1,7 +1,7 @@ use std::io::{self, Read, Write}; use std::str; use cslice::{CSlice, CMutSlice}; -use io::*; +use {ReadExt, WriteExt}; use self::tag::{Tag, TagIterator, split_tag}; unsafe fn recv_value(reader: &mut Read, tag: Tag, data: &mut *mut (), @@ -18,19 +18,19 @@ unsafe fn recv_value(reader: &mut Read, tag: Tag, data: &mut *mut (), Tag::None => Ok(()), Tag::Bool => consume_value!(u8, |ptr| { - *ptr = read_u8(reader)?; Ok(()) + *ptr = reader.read_u8()?; Ok(()) }), Tag::Int32 => consume_value!(u32, |ptr| { - *ptr = read_u32(reader)?; Ok(()) + *ptr = reader.read_u32()?; Ok(()) }), Tag::Int64 | Tag::Float64 => consume_value!(u64, |ptr| { - *ptr = read_u64(reader)?; Ok(()) + *ptr = reader.read_u64()?; Ok(()) }), Tag::String => { consume_value!(CMutSlice, |ptr| { - let length = read_u32(reader)? as usize; + let length = reader.read_u32()? as usize; *ptr = CMutSlice::new(alloc(length)? as *mut u8, length); reader.read_exact((*ptr).as_mut())?; Ok(()) @@ -47,7 +47,7 @@ unsafe fn recv_value(reader: &mut Read, tag: Tag, data: &mut *mut (), Tag::List(it) | Tag::Array(it) => { struct List { elements: *mut (), length: u32 }; consume_value!(List, |ptr| { - (*ptr).length = read_u32(reader)?; + (*ptr).length = reader.read_u32()?; let tag = it.clone().next().expect("truncated tag"); (*ptr).elements = alloc(tag.size() * (*ptr).length as usize)?; @@ -93,24 +93,24 @@ unsafe fn send_value(writer: &mut Write, tag: Tag, data: &mut *const ()) -> io:: }) } - write_u8(writer, tag.as_u8())?; + writer.write_u8(tag.as_u8())?; match tag { Tag::None => Ok(()), Tag::Bool => consume_value!(u8, |ptr| - write_u8(writer, *ptr)), + writer.write_u8(*ptr)), Tag::Int32 => consume_value!(u32, |ptr| - write_u32(writer, *ptr)), + writer.write_u32(*ptr)), Tag::Int64 | Tag::Float64 => consume_value!(u64, |ptr| - write_u64(writer, *ptr)), + writer.write_u64(*ptr)), Tag::String => consume_value!(CSlice, |ptr| - write_string(writer, str::from_utf8((*ptr).as_ref()).unwrap())), + writer.write_string(str::from_utf8((*ptr).as_ref()).unwrap())), Tag::Tuple(it, arity) => { let mut it = it.clone(); - write_u8(writer, arity)?; + writer.write_u8(arity)?; for _ in 0..arity { let tag = it.next().expect("truncated tag"); send_value(writer, tag, data)? @@ -120,7 +120,7 @@ unsafe fn send_value(writer: &mut Write, tag: Tag, data: &mut *const ()) -> io:: Tag::List(it) | Tag::Array(it) => { struct List { elements: *const (), length: u32 }; consume_value!(List, |ptr| { - write_u32(writer, (*ptr).length)?; + writer.write_u32((*ptr).length)?; let tag = it.clone().next().expect("truncated tag"); let mut data = (*ptr).elements; for _ in 0..(*ptr).length as usize { @@ -139,7 +139,7 @@ unsafe fn send_value(writer: &mut Write, tag: Tag, data: &mut *const ()) -> io:: Tag::Keyword(it) => { struct Keyword<'a> { name: CSlice<'a, u8>, contents: () }; consume_value!(Keyword, |ptr| { - write_string(writer, str::from_utf8((*ptr).name.as_ref()).unwrap())?; + writer.write_string(str::from_utf8((*ptr).name.as_ref()).unwrap())?; let tag = it.clone().next().expect("truncated tag"); let mut data = &(*ptr).contents as *const (); send_value(writer, tag, &mut data) @@ -150,7 +150,7 @@ unsafe fn send_value(writer: &mut Write, tag: Tag, data: &mut *const ()) -> io:: Tag::Object => { struct Object { id: u32 }; consume_value!(*const Object, |ptr| - write_u32(writer, (**ptr).id)) + writer.write_u32((**ptr).id)) } } } @@ -166,7 +166,7 @@ pub fn send_args(writer: &mut Write, service: u32, tag_bytes: &[u8], trace!("send<{}>({})->{}", service, args_it, return_it); } - write_u32(writer, service)?; + writer.write_u32(service)?; for index in 0.. { if let Some(arg_tag) = args_it.next() { let mut data = unsafe { *data.offset(index) }; @@ -175,8 +175,8 @@ pub fn send_args(writer: &mut Write, service: u32, tag_bytes: &[u8], break } } - write_u8(writer, 0)?; - write_bytes(writer, return_tag_bytes)?; + writer.write_u8(0)?; + writer.write_bytes(return_tag_bytes)?; Ok(()) } diff --git a/artiq/firmware/libproto/session_proto.rs b/artiq/firmware/libproto/session_proto.rs index 7448e1550..5a9955f86 100644 --- a/artiq/firmware/libproto/session_proto.rs +++ b/artiq/firmware/libproto/session_proto.rs @@ -1,12 +1,12 @@ use std::io::{self, Read, Write}; use std::vec::Vec; use std::string::String; -use io::*; +use {ReadExt, WriteExt}; fn read_sync(reader: &mut Read) -> io::Result<()> { let mut sync = [0; 4]; for i in 0.. { - sync[i % 4] = read_u8(reader)?; + sync[i % 4] = reader.read_u8()?; if sync == [0x5a; 4] { break } } Ok(()) @@ -47,37 +47,37 @@ pub enum Request { impl Request { pub fn read_from(reader: &mut Read) -> io::Result { read_sync(reader)?; - Ok(match read_u8(reader)? { + Ok(match reader.read_u8()? { 1 => Request::Log, 2 => Request::LogClear, 3 => Request::SystemInfo, - 4 => Request::SwitchClock(read_u8(reader)?), - 5 => Request::LoadKernel(read_bytes(reader)?), + 4 => Request::SwitchClock(reader.read_u8()?), + 5 => Request::LoadKernel(reader.read_bytes()?), 6 => Request::RunKernel, 7 => Request::RpcReply { - tag: read_bytes(reader)? + tag: reader.read_bytes()? }, 8 => Request::RpcException { - name: read_string(reader)?, - message: read_string(reader)?, - param: [read_u64(reader)? as i64, - read_u64(reader)? as i64, - read_u64(reader)? as i64], - file: read_string(reader)?, - line: read_u32(reader)?, - column: read_u32(reader)?, - function: read_string(reader)? + name: reader.read_string()?, + message: reader.read_string()?, + param: [reader.read_u64()? as i64, + reader.read_u64()? as i64, + reader.read_u64()? as i64], + file: reader.read_string()?, + line: reader.read_u32()?, + column: reader.read_u32()?, + function: reader.read_string()? }, 9 => Request::FlashRead { - key: read_string(reader)? + key: reader.read_string()? }, 10 => Request::FlashWrite { - key: read_string(reader)?, - value: read_bytes(reader)? + key: reader.read_string()?, + value: reader.read_bytes()? }, 11 => Request::FlashErase, 12 => Request::FlashRemove { - key: read_string(reader)? + key: reader.read_string()? }, _ => return Err(io::Error::new(io::ErrorKind::InvalidData, "unknown request type")) }) @@ -126,77 +126,77 @@ impl<'a> Reply<'a> { write_sync(writer)?; match *self { Reply::Log(ref log) => { - write_u8(writer, 1)?; - write_string(writer, log)?; + writer.write_u8(1)?; + writer.write_string(log)?; }, Reply::SystemInfo { ident, finished_cleanly } => { - write_u8(writer, 2)?; + writer.write_u8(2)?; writer.write(b"AROR")?; - write_string(writer, ident)?; - write_u8(writer, finished_cleanly as u8)?; + writer.write_string(ident)?; + writer.write_u8(finished_cleanly as u8)?; }, Reply::ClockSwitchCompleted => { - write_u8(writer, 3)?; + writer.write_u8(3)?; }, Reply::ClockSwitchFailed => { - write_u8(writer, 4)?; + writer.write_u8(4)?; }, Reply::LoadCompleted => { - write_u8(writer, 5)?; + writer.write_u8(5)?; }, Reply::LoadFailed(reason) => { - write_u8(writer, 6)?; - write_string(writer, reason)?; + writer.write_u8(6)?; + writer.write_string(reason)?; }, Reply::KernelFinished => { - write_u8(writer, 7)?; + writer.write_u8(7)?; }, Reply::KernelStartupFailed => { - write_u8(writer, 8)?; + writer.write_u8(8)?; }, Reply::KernelException { name, message, param, file, line, column, function, backtrace } => { - write_u8(writer, 9)?; - write_string(writer, name)?; - write_string(writer, message)?; - write_u64(writer, param[0] as u64)?; - write_u64(writer, param[1] as u64)?; - write_u64(writer, param[2] as u64)?; - write_string(writer, file)?; - write_u32(writer, line)?; - write_u32(writer, column)?; - write_string(writer, function)?; - write_u32(writer, backtrace.len() as u32)?; + writer.write_u8(9)?; + writer.write_string(name)?; + writer.write_string(message)?; + writer.write_u64(param[0] as u64)?; + writer.write_u64(param[1] as u64)?; + writer.write_u64(param[2] as u64)?; + writer.write_string(file)?; + writer.write_u32(line)?; + writer.write_u32(column)?; + writer.write_string(function)?; + writer.write_u32(backtrace.len() as u32)?; for &addr in backtrace { - write_u32(writer, addr as u32)? + writer.write_u32(addr as u32)? } }, Reply::RpcRequest { async } => { - write_u8(writer, 10)?; - write_u8(writer, async as u8)?; + writer.write_u8(10)?; + writer.write_u8(async as u8)?; }, Reply::FlashRead(ref bytes) => { - write_u8(writer, 11)?; - write_bytes(writer, bytes)?; + writer.write_u8(11)?; + writer.write_bytes(bytes)?; }, Reply::FlashOk => { - write_u8(writer, 12)?; + writer.write_u8(12)?; }, Reply::FlashError => { - write_u8(writer, 13)?; + writer.write_u8(13)?; }, Reply::WatchdogExpired => { - write_u8(writer, 14)?; + writer.write_u8(14)?; }, Reply::ClockFailure => { - write_u8(writer, 15)?; + writer.write_u8(15)?; }, } Ok(()) diff --git a/artiq/firmware/runtime/rtio_dma.rs b/artiq/firmware/runtime/rtio_dma.rs index a0ce08350..3a2ae706d 100644 --- a/artiq/firmware/runtime/rtio_dma.rs +++ b/artiq/firmware/runtime/rtio_dma.rs @@ -2,7 +2,7 @@ use std::vec::Vec; use std::string::String; use std::btree_map::BTreeMap; use std::mem; -use proto::io; +use proto::WriteExt; #[derive(Debug)] struct Entry { @@ -33,14 +33,14 @@ impl Manager { let length = /*length*/1 + /*channel*/3 + /*timestamp*/8 + /*address*/2 + /*data*/data.len() * 4; let writer = &mut self.recording; - io::write_u8(writer, length as u8).unwrap(); - io::write_u8(writer, (channel >> 24) as u8).unwrap(); - io::write_u8(writer, (channel >> 16) as u8).unwrap(); - io::write_u8(writer, (channel >> 8) as u8).unwrap(); - io::write_u64(writer, timestamp).unwrap(); - io::write_u16(writer, address as u16).unwrap(); + writer.write_u8(length as u8).unwrap(); + writer.write_u8((channel >> 24) as u8).unwrap(); + writer.write_u8((channel >> 16) as u8).unwrap(); + writer.write_u8((channel >> 8) as u8).unwrap(); + writer.write_u64(timestamp).unwrap(); + writer.write_u16(address as u16).unwrap(); for &word in data { - io::write_u32(writer, word).unwrap(); + writer.write_u32(word).unwrap(); } }