From d7fe8c307afc7fb64b8e39209f105241816b8cd5 Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Sun, 1 Feb 2015 20:36:26 -0800 Subject: [PATCH] refactor(headers): switch from MuCell to OptCell It turns out, we don't need capabilities of MuCell (or even RefCell). We don't to have sophisticated interior mutability. We don't ever lend out references that may be mutated later. Instead, if there is no value to lend, then we generate the value, require interior mutability to save the value internally, and then we return a reference. We never ever change a value once it's been generated. It can be changed, but only via &mut self methods, where we can safely reason about mutability. This means we don't need keep borrow checking code at runtime, which helps performance. We also are able to reduce the amount of unsafe transmutes. The internal API more safely constrains the usage of unsafe, enforcing our invariants, instead of shotgunning unsafe usage with Refs and promising we're doing it correctly. On a machine with lower memory (1GB): Before: test bench_mock_hyper ... bench: 251772 ns/iter (+/- 128445) After: test bench_mock_hyper ... bench: 152603 ns/iter (+/- 25928) On a machine with more memory, weaker CPU: Before: test bench_mock_hyper ... bench: 129398 ns/iter (+/- 51740) After: test bench_mock_hyper ... bench: 115935 ns/iter (+/- 28555) Closes #252 --- Cargo.toml | 1 - src/header/cell.rs | 41 ++++++++++++ src/header/mod.rs | 154 ++++++++++++++++++++++++++++----------------- src/lib.rs | 1 - 4 files changed, 136 insertions(+), 61 deletions(-) create mode 100644 src/header/cell.rs diff --git a/Cargo.toml b/Cargo.toml index 8b579a2a..41c126b6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,6 @@ keywords = ["http", "hyper", "hyperium"] cookie = "*" log = ">= 0.2.0" mime = "*" -mucell = "*" openssl = "*" rustc-serialize = "*" time = "*" diff --git a/src/header/cell.rs b/src/header/cell.rs new file mode 100644 index 00000000..1ede213d --- /dev/null +++ b/src/header/cell.rs @@ -0,0 +1,41 @@ +use std::cell::UnsafeCell; +use std::ops::Deref; + +pub struct OptCell(UnsafeCell>); + +impl OptCell { + #[inline] + pub fn new(val: Option) -> OptCell { + OptCell(UnsafeCell::new(val)) + } + + #[inline] + pub fn set(&self, val: T) { + unsafe { + let opt = self.0.get(); + debug_assert!((*opt).is_none()); + *opt = Some(val) + } + } + + #[inline] + pub unsafe fn get_mut(&mut self) -> &mut T { + let opt = &mut *self.0.get(); + opt.as_mut().unwrap() + } +} + +impl Deref for OptCell { + type Target = Option; + #[inline] + fn deref<'a>(&'a self) -> &'a Option { + unsafe { &*self.0.get() } + } +} + +impl Clone for OptCell { + #[inline] + fn clone(&self) -> OptCell { + OptCell::new((**self).clone()) + } +} diff --git a/src/header/mod.rs b/src/header/mod.rs index 03ecdbe7..0c563e56 100644 --- a/src/header/mod.rs +++ b/src/header/mod.rs @@ -16,15 +16,16 @@ use std::iter::FromIterator; use std::borrow::IntoCow; use std::{mem, raw}; -use mucell::MuCell; use uany::{UnsafeAnyExt}; use unicase::UniCase; +use self::cell::OptCell; use {http, HttpResult, HttpError}; pub use self::shared::{Encoding, QualityItem, qitem}; pub use self::common::*; +mod cell; mod common; mod shared; pub mod parsing; @@ -115,7 +116,7 @@ fn header_name() -> &'static str { /// A map of header fields on requests and responses. #[derive(Clone)] pub struct Headers { - data: HashMap> + data: HashMap } // To prevent DOS from a server sending a never ending header. @@ -146,15 +147,10 @@ impl Headers { } let name = UniCase(Owned(name)); let mut item = match headers.data.entry(name) { - Entry::Vacant(entry) => entry.insert(MuCell::new(Item::raw(vec![]))), + Entry::Vacant(entry) => entry.insert(Item::new_raw(vec![])), Entry::Occupied(entry) => entry.into_mut() }; - - match &mut item.borrow_mut().raw { - &mut Some(ref mut raw) => raw.push(value), - // Unreachable - _ => {} - }; + item.mut_raw().push(value); }, None => break, } @@ -167,7 +163,7 @@ impl Headers { /// The field is determined by the type of the value being set. pub fn set(&mut self, value: H) { self.data.insert(UniCase(Borrowed(header_name::())), - MuCell::new(Item::typed(Box::new(value)))); + Item::new_typed(Box::new(value))); } /// Access the raw value of a header. @@ -186,19 +182,15 @@ impl Headers { // FIXME(reem): Find a better way to do this lookup without find_equiv. .get(&UniCase(Borrowed(unsafe { mem::transmute::<&str, &str>(name) }))) .and_then(|item| { - if let Some(ref raw) = item.borrow().raw { - return unsafe { mem::transmute(Some(&raw[])) }; + if let Some(ref raw) = *item.raw { + return Some(&raw[]); } - let worked = item.try_mutate(|item| { - let raw = vec![item.typed.as_ref().unwrap().to_string().into_bytes()]; - item.raw = Some(raw); - }); - debug_assert!(worked, "item.try_mutate should return true"); + let raw = vec![item.typed.as_ref().unwrap().to_string().into_bytes()]; + item.raw.set(raw); - let item = item.borrow(); let raw = item.raw.as_ref().unwrap(); - unsafe { mem::transmute(Some(&raw[])) } + Some(&raw[]) }) } @@ -212,14 +204,14 @@ impl Headers { /// headers.set_raw("content-length", vec![b"5".to_vec()]); /// ``` pub fn set_raw>(&mut self, name: K, value: Vec>) { - self.data.insert(UniCase(name.into_cow()), MuCell::new(Item::raw(value))); + self.data.insert(UniCase(name.into_cow()), Item::new_raw(value)); } /// Get a reference to the header field's value, if it exists. pub fn get(&self) -> Option<&H> { self.get_or_parse::().map(|item| { unsafe { - mem::transmute::<&H, &H>(downcast(&*item.borrow())) + downcast(&*item) } }) } @@ -227,15 +219,15 @@ impl Headers { /// Get a mutable reference to the header field's value, if it exists. pub fn get_mut(&mut self) -> Option<&mut H> { self.get_or_parse_mut::().map(|item| { - unsafe { downcast_mut(item.borrow_mut()) } + unsafe { downcast_mut(item) } }) } - fn get_or_parse(&self) -> Option<&MuCell> { + fn get_or_parse(&self) -> Option<&Item> { self.data.get(&UniCase(Borrowed(header_name::()))).and_then(get_or_parse::) } - fn get_or_parse_mut(&mut self) -> Option<&mut MuCell> { + fn get_or_parse_mut(&mut self) -> Option<&mut Item> { self.data.get_mut(&UniCase(Borrowed(header_name::()))).and_then(get_or_parse_mut::) } @@ -299,7 +291,7 @@ impl fmt::Debug for Headers { /// An `Iterator` over the fields in a `Headers` map. pub struct HeadersItems<'a> { - inner: Iter<'a, HeaderName, MuCell> + inner: Iter<'a, HeaderName, Item> } impl<'a> Iterator for HeadersItems<'a> { @@ -314,7 +306,7 @@ impl<'a> Iterator for HeadersItems<'a> { } /// Returned with the `HeadersItems` iterator. -pub struct HeaderView<'a>(&'a HeaderName, &'a MuCell); +pub struct HeaderView<'a>(&'a HeaderName, &'a Item); impl<'a> HeaderView<'a> { /// Check if a HeaderView is a certain Header. @@ -334,7 +326,7 @@ impl<'a> HeaderView<'a> { pub fn value(&self) -> Option<&'a H> { get_or_parse::(self.1).map(|item| { unsafe { - mem::transmute::<&H, &H>(downcast(&*item.borrow())) + downcast(&*item) } }) } @@ -342,13 +334,13 @@ impl<'a> HeaderView<'a> { /// Get just the header value as a String. #[inline] pub fn value_string(&self) -> String { - (*self.1.borrow()).to_string() + (*self.1).to_string() } } impl<'a> fmt::Display for HeaderView<'a> { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}: {}", self.0, *self.1.borrow()) + write!(f, "{}: {}", self.0, *self.1) } } @@ -376,29 +368,47 @@ impl<'a> FromIterator> for Headers { #[derive(Clone)] struct Item { - raw: Option>>, - typed: Option> + raw: OptCell>>, + typed: OptCell> } impl Item { - fn raw(data: Vec>) -> Item { + #[inline] + fn new_raw(data: Vec>) -> Item { Item { - raw: Some(data), - typed: None, + raw: OptCell::new(Some(data)), + typed: OptCell::new(None), } } - fn typed(ty: Box) -> Item { + #[inline] + fn new_typed(ty: Box) -> Item { Item { - raw: None, - typed: Some(ty), + raw: OptCell::new(None), + typed: OptCell::new(Some(ty)), } } + #[inline] + fn mut_raw(&mut self) -> &mut Vec> { + self.typed = OptCell::new(None); + unsafe { + self.raw.get_mut() + } + } + + #[inline] + fn mut_typed(&mut self) -> &mut Box { + self.raw = OptCell::new(None); + unsafe { + self.typed.get_mut() + } + } } -fn get_or_parse(item: &MuCell) -> Option<&MuCell> { - match item.borrow().typed { + +fn get_or_parse(item: &Item) -> Option<&Item> { + match *item.typed { Some(ref typed) if typed.is::() => return Some(item), Some(ref typed) => { warn!("attempted to access {:?} as wrong type", typed); @@ -407,17 +417,16 @@ fn get_or_parse(item: &MuCell) -> Option<&MuCell _ => () } - let worked = item.try_mutate(parse::); - debug_assert!(worked, "item.try_mutate should return true"); - if item.borrow().typed.is_some() { + parse::(item); + if item.typed.is_some() { Some(item) } else { None } } -fn get_or_parse_mut(item: &mut MuCell) -> Option<&mut MuCell> { - let is_correct_type = match item.borrow().typed { +fn get_or_parse_mut(item: &mut Item) -> Option<&mut Item> { + let is_correct_type = match *item.typed { Some(ref typed) if typed.is::() => Some(true), Some(ref typed) => { warn!("attempted to access {:?} as wrong type", typed); @@ -432,37 +441,39 @@ fn get_or_parse_mut(item: &mut MuCell) -> Option None => () } - parse::(item.borrow_mut()); - if item.borrow().typed.is_some() { + parse::(&item); + if item.typed.is_some() { Some(item) } else { None } } -fn parse(item: &mut Item) { - item.typed = match item.raw { +fn parse(item: &Item) { + match *item.raw { Some(ref raw) => match Header::parse_header(&raw[]) { - Some::(h) => Some(box h as Box), - None => None + Some::(h) => item.typed.set(box h as Box), + None => () }, None => unreachable!() - }; + } } +#[inline] unsafe fn downcast(item: &Item) -> &H { item.typed.as_ref().expect("item.typed must be set").downcast_ref_unchecked() } +#[inline] unsafe fn downcast_mut(item: &mut Item) -> &mut H { - item.typed.as_mut().expect("item.typed must be set").downcast_mut_unchecked() + item.mut_typed().downcast_mut_unchecked() } impl fmt::Display for Item { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - match self.typed { + match *self.typed { Some(ref h) => h.fmt_header(fmt), - None => match self.raw { + None => match *self.raw { Some(ref raw) => { for part in raw.iter() { match from_utf8(&part[]) { @@ -483,12 +494,14 @@ impl fmt::Display for Item { impl fmt::Debug for Box { + #[inline] fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { (**self).fmt_header(fmt) } } impl fmt::Display for Box { + #[inline] fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { (**self).fmt_header(fmt) } @@ -502,12 +515,14 @@ impl fmt::Display for Box { pub struct HeaderFormatter<'a, H: HeaderFormat>(pub &'a H); impl<'a, H: HeaderFormat> fmt::Display for HeaderFormatter<'a, H> { + #[inline] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.0.fmt_header(f) } } impl<'a, H: HeaderFormat> fmt::Debug for HeaderFormatter<'a, H> { + #[inline] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.0.fmt_header(f) } @@ -695,37 +710,58 @@ mod tests { } #[bench] - fn bench_header_get(b: &mut Bencher) { + fn bench_headers_new(b: &mut Bencher) { + b.iter(|| { + let mut h = Headers::new(); + h.set(ContentLength(11)); + h + }) + } + + #[bench] + fn bench_headers_from_raw(b: &mut Bencher) { + b.iter(|| Headers::from_raw(&mut mem("Content-Length: 10\r\n\r\n")).unwrap()) + } + + #[bench] + fn bench_headers_get(b: &mut Bencher) { let mut headers = Headers::new(); headers.set(ContentLength(11)); b.iter(|| assert_eq!(headers.get::(), Some(&ContentLength(11)))) } #[bench] - fn bench_header_get_miss(b: &mut Bencher) { + fn bench_headers_get_miss(b: &mut Bencher) { let headers = Headers::new(); b.iter(|| assert!(headers.get::().is_none())) } #[bench] - fn bench_header_set(b: &mut Bencher) { + fn bench_headers_set(b: &mut Bencher) { let mut headers = Headers::new(); b.iter(|| headers.set(ContentLength(12))) } #[bench] - fn bench_header_has(b: &mut Bencher) { + fn bench_headers_has(b: &mut Bencher) { let mut headers = Headers::new(); headers.set(ContentLength(11)); b.iter(|| assert!(headers.has::())) } #[bench] - fn bench_header_view_is(b: &mut Bencher) { + fn bench_headers_view_is(b: &mut Bencher) { let mut headers = Headers::new(); headers.set(ContentLength(11)); let mut iter = headers.iter(); let view = iter.next().unwrap(); b.iter(|| assert!(view.is::())) } + + #[bench] + fn bench_headers_fmt(b: &mut Bencher) { + let mut headers = Headers::new(); + headers.set(ContentLength(11)); + b.iter(|| headers.to_string()) + } } diff --git a/src/lib.rs b/src/lib.rs index cabc956e..e359f5b2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -134,7 +134,6 @@ extern crate openssl; #[cfg(test)] extern crate test; extern crate "unsafe-any" as uany; extern crate cookie; -extern crate mucell; extern crate unicase; pub use std::old_io::net::ip::{SocketAddr, IpAddr, Ipv4Addr, Ipv6Addr, Port};