From d5aa795aa5f316bc5f43508df5bc41cba0a61ea8 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 27 Feb 2014 19:53:03 -0800 Subject: [PATCH 1/4] std: Add cfg(test) to UnsafeArc assertions This is a ubiquitous type in concurrent code, and the assertions are causing significant code bloat for simple operations such as reading the pointer (injecting a failure point, etc). I am testing executable sizes with no I/O implementations (everything stubbed out to return nothing), and this took the size of a libnative executable from 328K to 207K (37% reduction in size), so I think that this is one assertion that's well worth configuring off for now. --- src/libstd/sync/arc.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/libstd/sync/arc.rs b/src/libstd/sync/arc.rs index 5c452018b9b7e..10369a52f0f17 100644 --- a/src/libstd/sync/arc.rs +++ b/src/libstd/sync/arc.rs @@ -80,7 +80,8 @@ impl UnsafeArc { #[inline] pub fn get(&self) -> *mut T { unsafe { - assert!((*self.data).count.load(Relaxed) > 0); + // FIXME(#12049): this needs some sort of debug assertion + if cfg!(test) { assert!((*self.data).count.load(Relaxed) > 0); } return &mut (*self.data).data as *mut T; } } @@ -90,7 +91,8 @@ impl UnsafeArc { #[inline] pub fn get_immut(&self) -> *T { unsafe { - assert!((*self.data).count.load(Relaxed) > 0); + // FIXME(#12049): this needs some sort of debug assertion + if cfg!(test) { assert!((*self.data).count.load(Relaxed) > 0); } return &(*self.data).data as *T; } } @@ -109,7 +111,8 @@ impl Clone for UnsafeArc { unsafe { // This barrier might be unnecessary, but I'm not sure... let old_count = (*self.data).count.fetch_add(1, Acquire); - assert!(old_count >= 1); + // FIXME(#12049): this needs some sort of debug assertion + if cfg!(test) { assert!(old_count >= 1); } return UnsafeArc { data: self.data }; } } @@ -127,7 +130,8 @@ impl Drop for UnsafeArc{ // Must be acquire+release, not just release, to make sure this // doesn't get reordered to after the unwrapper pointer load. let old_count = (*self.data).count.fetch_sub(1, SeqCst); - assert!(old_count >= 1); + // FIXME(#12049): this needs some sort of debug assertion + if cfg!(test) { assert!(old_count >= 1); } if old_count == 1 { let _: ~ArcData = cast::transmute(self.data); } From d89074c8ae11d52b3b92d73bc684f5b964a05cf5 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 27 Feb 2014 20:25:13 -0800 Subject: [PATCH 2/4] std: Remove lots of allocations from log settings Most of these are unnecessary because we're only looking at static strings. This also moves to Vec in a few places instead of ~[T]. This didn't end up getting much of a code size win (update_log_settings is the third largest function in the executables I'm looking at), but this seems like a generally nice improvement regardless. --- src/libstd/rt/crate_map.rs | 4 +- src/libstd/rt/logging.rs | 194 ++++++++++++++++++------------------- 2 files changed, 96 insertions(+), 102 deletions(-) diff --git a/src/libstd/rt/crate_map.rs b/src/libstd/rt/crate_map.rs index 409b77d1a3f86..ff54a80ce997f 100644 --- a/src/libstd/rt/crate_map.rs +++ b/src/libstd/rt/crate_map.rs @@ -123,7 +123,7 @@ fn version(crate_map: &CrateMap) -> i32 { fn do_iter_crate_map<'a>( crate_map: &'a CrateMap<'a>, - f: |&ModEntry|, + f: |&'a ModEntry<'a>|, visited: &mut ~[*CrateMap<'a>]) { let raw = crate_map as *CrateMap<'a>; if visited.bsearch(|a| (*a as uint).cmp(&(raw as uint))).is_some() { @@ -149,7 +149,7 @@ fn do_iter_crate_map<'a>( } /// Iterates recursively over `crate_map` and all child crate maps -pub fn iter_crate_map<'a>(crate_map: &'a CrateMap<'a>, f: |&ModEntry|) { +pub fn iter_crate_map<'a>(crate_map: &'a CrateMap<'a>, f: |&'a ModEntry<'a>|) { let mut v = ~[]; do_iter_crate_map(crate_map, f, &mut v); } diff --git a/src/libstd/rt/logging.rs b/src/libstd/rt/logging.rs index b86a9612d7061..aa024a53b89ec 100644 --- a/src/libstd/rt/logging.rs +++ b/src/libstd/rt/logging.rs @@ -13,13 +13,14 @@ use from_str::from_str; use iter::Iterator; use libc::exit; use option::{Some, None, Option}; +use os; use rt::crate_map::{ModEntry, CrateMap, iter_crate_map, get_crate_map}; -use str::StrSlice; +use str::{Str, StrSlice}; use vec::{ImmutableVector, MutableTotalOrdVector, OwnedVector}; -#[cfg(test)] use cast::transmute; +use vec_ng::Vec; -struct LogDirective { - name: Option<~str>, +struct LogDirective<'a> { + name: Option<&'a str>, level: u32 } @@ -58,36 +59,39 @@ fn parse_log_level(level: &str) -> Option { /// and return a vector with log directives. /// Valid log levels are 0-255, with the most likely ones being 1-4 (defined in std::). /// Also supports string log levels of error, warn, info, and debug -fn parse_logging_spec(spec: ~str) -> ~[LogDirective]{ - let mut dirs = ~[]; +fn parse_logging_spec<'a>(spec: &'a str) -> Vec> { + let mut dirs = Vec::new(); for s in spec.split(',') { - let parts: ~[&str] = s.split('=').collect(); - let mut log_level; - let mut name = Some(parts[0].to_owned()); - match parts.len() { - 1 => { + if s.len() == 0 { continue } + let mut parts = s.split('='); + let log_level; + let name; + match (parts.next(), parts.next(), parts.next()) { + (Some(part0), None, None) => { //if the single argument is a log-level string or number, //treat that as a global fallback - let possible_log_level = parse_log_level(parts[0]); + let possible_log_level = parse_log_level(part0); match possible_log_level { Some(num) => { name = None; log_level = num; }, - _ => { - log_level = MAX_LOG_LEVEL + None => { + log_level = MAX_LOG_LEVEL; + name = Some(part0); } } } - 2 => { - let possible_log_level = parse_log_level(parts[1]); + (Some(part0), Some(part1), None) => { + let possible_log_level = parse_log_level(part1); match possible_log_level { Some(num) => { + name = Some(part0); log_level = num; }, _ => { rterrln!("warning: invalid logging spec '{}', \ - ignoring it", parts[1]); + ignoring it", part1); continue } } @@ -98,8 +102,7 @@ fn parse_logging_spec(spec: ~str) -> ~[LogDirective]{ continue } } - let dir = LogDirective {name: name, level: log_level}; - dirs.push(dir); + dirs.push(LogDirective { name: name, level: log_level }); } return dirs; } @@ -134,27 +137,24 @@ fn update_entry(dirs: &[LogDirective], entry: &ModEntry) -> u32 { /// Set log level for every entry in crate_map according to the sepecification /// in settings -fn update_log_settings(crate_map: &CrateMap, settings: ~str) { - let mut dirs = ~[]; - if settings.len() > 0 { - if settings == ~"::help" || settings == ~"?" { - rterrln!("\nCrate log map:\n"); +fn update_log_settings(crate_map: &CrateMap, settings: &str) { + if settings == "::help" || settings == "?" { + rterrln!("\nCrate log map:\n"); - let mut entries = ~[]; - iter_crate_map(crate_map, |entry| entries.push(entry.name.to_owned())); - entries.sort(); + let mut entries = Vec::new(); + iter_crate_map(crate_map, |entry| entries.push(entry.name)); + entries.as_mut_slice().sort(); - for name in entries.iter() { - rterrln!(" {}", *name); - } - unsafe { exit(1); } + for name in entries.iter() { + rterrln!(" {}", *name); } - dirs = parse_logging_spec(settings); + unsafe { exit(1); } } + let dirs = parse_logging_spec(settings); let mut n_matches: u32 = 0; iter_crate_map(crate_map, |entry| { - let m = update_entry(dirs, entry); + let m = update_entry(dirs.as_slice(), entry); n_matches += m; }); @@ -169,18 +169,12 @@ fn update_log_settings(crate_map: &CrateMap, settings: ~str) { /// Configure logging by traversing the crate map and setting the /// per-module global logging flags based on the logging spec pub fn init() { - use os; - let log_spec = os::getenv("RUST_LOG"); match get_crate_map() { Some(crate_map) => { match log_spec { - Some(spec) => { - update_log_settings(crate_map, spec); - } - None => { - update_log_settings(crate_map, ~""); - } + Some(spec) => update_log_settings(crate_map, spec.as_slice()), + None => update_log_settings(crate_map, ""), } }, _ => { @@ -197,124 +191,124 @@ pub fn init() { // Tests for parse_logging_spec() #[test] fn parse_logging_spec_valid() { - let dirs = parse_logging_spec(~"crate1::mod1=1,crate1::mod2,crate2=4"); + let dirs = parse_logging_spec("crate1::mod1=1,crate1::mod2,crate2=4"); + let dirs = dirs.as_slice(); assert_eq!(dirs.len(), 3); - assert!(dirs[0].name == Some(~"crate1::mod1")); + assert_eq!(dirs[0].name, Some("crate1::mod1")); assert_eq!(dirs[0].level, 1); - assert!(dirs[1].name == Some(~"crate1::mod2")); + assert_eq!(dirs[1].name, Some("crate1::mod2")); assert_eq!(dirs[1].level, MAX_LOG_LEVEL); - assert!(dirs[2].name == Some(~"crate2")); + assert_eq!(dirs[2].name, Some("crate2")); assert_eq!(dirs[2].level, 4); } #[test] fn parse_logging_spec_invalid_crate() { // test parse_logging_spec with multiple = in specification - let dirs = parse_logging_spec(~"crate1::mod1=1=2,crate2=4"); + let dirs = parse_logging_spec("crate1::mod1=1=2,crate2=4"); + let dirs = dirs.as_slice(); assert_eq!(dirs.len(), 1); - assert!(dirs[0].name == Some(~"crate2")); + assert_eq!(dirs[0].name, Some("crate2")); assert_eq!(dirs[0].level, 4); } #[test] fn parse_logging_spec_invalid_log_level() { // test parse_logging_spec with 'noNumber' as log level - let dirs = parse_logging_spec(~"crate1::mod1=noNumber,crate2=4"); + let dirs = parse_logging_spec("crate1::mod1=noNumber,crate2=4"); + let dirs = dirs.as_slice(); assert_eq!(dirs.len(), 1); - assert!(dirs[0].name == Some(~"crate2")); + assert_eq!(dirs[0].name, Some("crate2")); assert_eq!(dirs[0].level, 4); } #[test] fn parse_logging_spec_string_log_level() { // test parse_logging_spec with 'warn' as log level - let dirs = parse_logging_spec(~"crate1::mod1=wrong,crate2=warn"); + let dirs = parse_logging_spec("crate1::mod1=wrong,crate2=warn"); + let dirs = dirs.as_slice(); assert_eq!(dirs.len(), 1); - assert!(dirs[0].name == Some(~"crate2")); + assert_eq!(dirs[0].name, Some("crate2")); assert_eq!(dirs[0].level, 2); } #[test] fn parse_logging_spec_global() { // test parse_logging_spec with no crate - let dirs = parse_logging_spec(~"warn,crate2=4"); + let dirs = parse_logging_spec("warn,crate2=4"); + let dirs = dirs.as_slice(); assert_eq!(dirs.len(), 2); - assert!(dirs[0].name == None); + assert_eq!(dirs[0].name, None); assert_eq!(dirs[0].level, 2); - assert!(dirs[1].name == Some(~"crate2")); + assert_eq!(dirs[1].name, Some("crate2")); assert_eq!(dirs[1].level, 4); } // Tests for update_entry #[test] fn update_entry_match_full_path() { - let dirs = ~[LogDirective {name: Some(~"crate1::mod1"), level: 2 }, - LogDirective {name: Some(~"crate2"), level: 3}]; - let level = &mut 0; - unsafe { - let entry= &ModEntry {name:"crate1::mod1", log_level: level}; - let m = update_entry(dirs, transmute(entry)); - assert!(*entry.log_level == 2); - assert!(m == 1); + let dirs = [LogDirective { name: Some("crate1::mod1"), level: 2 }, + LogDirective { name: Some("crate2"), level: 3 }]; + let mut level = 0; + { + let entry = &ModEntry { name: "crate1::mod1", log_level: &mut level }; + assert_eq!(update_entry(dirs, entry), 1); } + assert_eq!(level, 2); } #[test] fn update_entry_no_match() { - let dirs = ~[LogDirective {name: Some(~"crate1::mod1"), level: 2 }, - LogDirective {name: Some(~"crate2"), level: 3}]; - let level = &mut 0; - unsafe { - let entry= &ModEntry {name: "crate3::mod1", log_level: level}; - let m = update_entry(dirs, transmute(entry)); - assert!(*entry.log_level == DEFAULT_LOG_LEVEL); - assert!(m == 0); + let dirs = [LogDirective { name: Some("crate1::mod1"), level: 2 }, + LogDirective { name: Some("crate2"), level: 3 }]; + let mut level = 0; + { + let entry = &ModEntry { name: "crate3::mod1", log_level: &mut level }; + assert_eq!(update_entry(dirs, entry), 0); } + assert_eq!(level, DEFAULT_LOG_LEVEL); } #[test] fn update_entry_match_beginning() { - let dirs = ~[LogDirective {name: Some(~"crate1::mod1"), level: 2 }, - LogDirective {name: Some(~"crate2"), level: 3}]; - let level = &mut 0; - unsafe { - let entry= &ModEntry {name: "crate2::mod1", log_level: level}; - let m = update_entry(dirs, transmute(entry)); - assert!(*entry.log_level == 3); - assert!(m == 1); + let dirs = [LogDirective { name: Some("crate1::mod1"), level: 2 }, + LogDirective { name: Some("crate2"), level: 3 }]; + let mut level = 0; + { + let entry= &ModEntry {name: "crate2::mod1", log_level: &mut level}; + assert_eq!(update_entry(dirs, entry), 1); } + assert_eq!(level, 3); } #[test] fn update_entry_match_beginning_longest_match() { - let dirs = ~[LogDirective {name: Some(~"crate1::mod1"), level: 2 }, - LogDirective {name: Some(~"crate2"), level: 3}, - LogDirective {name: Some(~"crate2::mod"), level: 4}]; - let level = &mut 0; - unsafe { - let entry = &ModEntry {name: "crate2::mod1", log_level: level}; - let m = update_entry(dirs, transmute(entry)); - assert!(*entry.log_level == 4); - assert!(m == 1); + let dirs = [LogDirective { name: Some("crate1::mod1"), level: 2 }, + LogDirective { name: Some("crate2"), level: 3 }, + LogDirective { name: Some("crate2::mod"), level: 4 }]; + let mut level = 0; + { + let entry = &ModEntry { name: "crate2::mod1", log_level: &mut level }; + assert_eq!(update_entry(dirs, entry), 1); } + assert_eq!(level, 4); } #[test] fn update_entry_match_default() { - let dirs = ~[LogDirective {name: Some(~"crate1::mod1"), level: 2 }, - LogDirective {name: None, level: 3} - ]; - let level = &mut 0; - unsafe { - let entry= &ModEntry {name: "crate1::mod1", log_level: level}; - let m = update_entry(dirs, transmute(entry)); - assert!(*entry.log_level == 2); - assert!(m == 1); - let entry= &ModEntry {name: "crate2::mod2", log_level: level}; - let m = update_entry(dirs, transmute(entry)); - assert!(*entry.log_level == 3); - assert!(m == 1); + let dirs = [LogDirective { name: Some("crate1::mod1"), level: 2 }, + LogDirective { name: None, level: 3 }]; + let mut level = 0; + { + let entry = &ModEntry { name: "crate1::mod1", log_level: &mut level }; + assert_eq!(update_entry(dirs, entry), 1); + } + assert_eq!(level, 2); + { + let entry = &ModEntry { name: "crate2::mod2", log_level: &mut level }; + assert_eq!(update_entry(dirs, entry), 1); } + assert_eq!(level, 3); } From 79e6ab54d13b8a5b8c17093186c39e493784bdc6 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 27 Feb 2014 20:37:40 -0800 Subject: [PATCH 3/4] std: Avoid using "{:?}" in format strings This removes all usage of Poly in format strings from libstd. This doesn't prevent more future strings from coming in, but it at least removes the ones for now. --- src/libnative/io/process.rs | 4 ++-- src/libstd/num/strconv.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libnative/io/process.rs b/src/libnative/io/process.rs index 0b833f473955e..6ac1f2b369244 100644 --- a/src/libnative/io/process.rs +++ b/src/libnative/io/process.rs @@ -470,7 +470,7 @@ fn spawn_process_os(config: p::ProcessConfig, Err(e) => { assert!(e.kind == io::BrokenPipe || e.kind == io::EndOfFile, - "unexpected error: {:?}", e); + "unexpected error: {}", e); Ok(SpawnProcessResult { pid: pid, handle: ptr::null() @@ -744,7 +744,7 @@ fn waitpid(pid: pid_t) -> p::ProcessExit { let mut status = 0 as c_int; match retry(|| unsafe { wait::waitpid(pid, &mut status, 0) }) { - -1 => fail!("unknown waitpid error: {:?}", super::last_error()), + -1 => fail!("unknown waitpid error: {}", super::last_error()), _ => { if imp::WIFEXITED(status) { p::ExitStatus(imp::WEXITSTATUS(status) as int) diff --git a/src/libstd/num/strconv.rs b/src/libstd/num/strconv.rs index 153c042c6a8c1..00497b6f0eaf0 100644 --- a/src/libstd/num/strconv.rs +++ b/src/libstd/num/strconv.rs @@ -532,19 +532,19 @@ pub fn from_str_bytes_common+ ) -> Option { match exponent { ExpDec if radix >= DIGIT_E_RADIX // decimal exponent 'e' - => fail!("from_str_bytes_common: radix {:?} incompatible with \ + => fail!("from_str_bytes_common: radix {} incompatible with \ use of 'e' as decimal exponent", radix), ExpBin if radix >= DIGIT_P_RADIX // binary exponent 'p' - => fail!("from_str_bytes_common: radix {:?} incompatible with \ + => fail!("from_str_bytes_common: radix {} incompatible with \ use of 'p' as binary exponent", radix), _ if special && radix >= DIGIT_I_RADIX // first digit of 'inf' - => fail!("from_str_bytes_common: radix {:?} incompatible with \ + => fail!("from_str_bytes_common: radix {} incompatible with \ special values 'inf' and 'NaN'", radix), _ if (radix as int) < 2 - => fail!("from_str_bytes_common: radix {:?} to low, \ + => fail!("from_str_bytes_common: radix {} to low, \ must lie in the range [2, 36]", radix), _ if (radix as int) > 36 - => fail!("from_str_bytes_common: radix {:?} to high, \ + => fail!("from_str_bytes_common: radix {} to high, \ must lie in the range [2, 36]", radix), _ => () } From ddc1c21264898f6a5d12cf03bba30f1f08b73665 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 27 Feb 2014 21:11:50 -0800 Subject: [PATCH 4/4] std: Flag run_fmt() as #[inline(always)] This function is a tiny wrapper that LLVM doesn't want to inline, and it ends up causing more bloat than necessary. The bloat is pretty small, but it's a win of at least 7k for small executables, and I imagine that the number goes up as there are more calls to fail!(). --- src/libstd/macros.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/libstd/macros.rs b/src/libstd/macros.rs index 490f2c9b198d2..a2e80a180ea3b 100644 --- a/src/libstd/macros.rs +++ b/src/libstd/macros.rs @@ -149,7 +149,14 @@ macro_rules! fail( // function to pass to format_args!, *and* we need the // file and line numbers right here; so an inner bare fn // is our only choice. - #[inline] + // + // LLVM doesn't tend to inline this, presumably because begin_unwind_fmt + // is #[cold] and #[inline(never)] and because this is flagged as cold + // as returning !. We really do want this to be inlined, however, + // because it's just a tiny wrapper. Small wins (156K to 149K in size) + // were seen when forcing this to be inlined, and that number just goes + // up with the number of calls to fail!() + #[inline(always)] fn run_fmt(fmt: &::std::fmt::Arguments) -> ! { ::std::rt::begin_unwind_fmt(fmt, file!(), line!()) }