auto merge of #9352 : erickt/rust/master, r=huonw

bors · bors · commit d6774f8c3992 · 2013-09-26T23:51:13.000-07:00
One of the downsides with `c_str` is that it always forces an allocation, and so this could add unnecessary overhead to various calls. This PR implements one of the suggestions @graydon made in #8296 for `vec.with_c_str` in that for a short string can use a small stack array instead of a malloced array for our temporary c string. This ends up being twice as fast for small strings. There are two things to consider before landing this commit though. First off, I arbitrarily picked the stack array as 32 bytes, and I'm not sure if this a reasonable amount or not. Second, there is a risk that someone can keep a reference to the interior stack pointer, which could cause mayhem if someone were to dereference the pointer. Since we also can easily grab and return interior pointers to vecs though, I don't think this is that much of an issue.
diff --git a/src/librustc/middle/trans/base.rs b/src/librustc/middle/trans/base.rs
@@ -524,13 +524,13 @@ pub fn set_always_inline(f: ValueRef) {
 }
 
 pub fn set_fixed_stack_segment(f: ValueRef) {
-    do "fixed-stack-segment".to_c_str().with_ref |buf| {
+    do "fixed-stack-segment".with_c_str |buf| {
         unsafe { llvm::LLVMAddFunctionAttrString(f, buf); }
     }
 }
 
 pub fn set_no_split_stack(f: ValueRef) {
-    do "no-split-stack".to_c_str().with_ref |buf| {
+    do "no-split-stack".with_c_str |buf| {
         unsafe { llvm::LLVMAddFunctionAttrString(f, buf); }
     }
 }
diff --git a/src/librustc/middle/trans/debuginfo.rs b/src/librustc/middle/trans/debuginfo.rs
@@ -781,7 +781,7 @@ pub fn create_function_debug_context(cx: &mut CrateContext,
 
                 let ident = special_idents::type_self;
 
-                let param_metadata = do token::ident_to_str(&ident).to_c_str().with_ref |name| {
+                let param_metadata = do token::ident_to_str(&ident).with_c_str |name| {
                     unsafe {
                         llvm::LLVMDIBuilderCreateTemplateTypeParameter(
                             DIB(cx),
@@ -819,7 +819,7 @@ pub fn create_function_debug_context(cx: &mut CrateContext,
             // Again, only create type information if extra_debuginfo is enabled
             if cx.sess.opts.extra_debuginfo {
                 let actual_type_metadata = type_metadata(cx, actual_type, codemap::dummy_sp());
-                let param_metadata = do token::ident_to_str(&ident).to_c_str().with_ref |name| {
+                let param_metadata = do token::ident_to_str(&ident).with_c_str |name| {
                     unsafe {
                         llvm::LLVMDIBuilderCreateTemplateTypeParameter(
                             DIB(cx),
diff --git a/src/librustc/middle/trans/foreign.rs b/src/librustc/middle/trans/foreign.rs
@@ -465,7 +465,7 @@ pub fn trans_rust_fn_with_foreign_abi(ccx: @mut CrateContext,
         //     }
 
         let the_block =
-            "the block".to_c_str().with_ref(
+            "the block".with_c_str(
                 |s| llvm::LLVMAppendBasicBlockInContext(ccx.llcx, llwrapfn, s));
 
         let builder = ccx.builder.B;
@@ -519,7 +519,7 @@ pub fn trans_rust_fn_with_foreign_abi(ccx: @mut CrateContext,
 
                 None => {
                     let slot = {
-                        "return_alloca".to_c_str().with_ref(
+                        "return_alloca".with_c_str(
                             |s| llvm::LLVMBuildAlloca(builder,
                                                       llrust_ret_ty.to_ref(),
                                                       s))
diff --git a/src/libstd/c_str.rs b/src/libstd/c_str.rs
@@ -61,16 +61,18 @@ do my_string.with_c_str |c_buffer| {
 */
 
 use cast;
+use container::Container;
 use iter::{Iterator, range};
 use libc;
 use ops::Drop;
 use option::{Option, Some, None};
 use ptr::RawPtr;
 use ptr;
-use str;
 use str::StrSlice;
-use vec::{ImmutableVector, CopyableVector};
-use container::Container;
+use str;
+use vec::{CopyableVector, ImmutableVector, MutableVector};
+use vec;
+use unstable::intrinsics;
 
 /// Resolution options for the `null_byte` condition
 pub enum NullByteResolution {
@@ -152,8 +154,7 @@ impl CString {
     pub fn as_bytes<'a>(&'a self) -> &'a [u8] {
         if self.buf.is_null() { fail!("CString is null!"); }
         unsafe {
-            let len = ptr::position(self.buf, |c| *c == 0);
-            cast::transmute((self.buf, len + 1))
+            cast::transmute((self.buf, self.len() + 1))
         }
     }
 
@@ -187,6 +188,15 @@ impl Drop for CString {
     }
 }
 
+impl Container for CString {
+    #[inline]
+    fn len(&self) -> uint {
+        unsafe {
+            ptr::position(self.buf, |c| *c == 0)
+        }
+    }
+}
+
 /// A generic trait for converting a value to a CString.
 pub trait ToCStr {
     /// Copy the receiver into a CString.
@@ -233,24 +243,27 @@ impl<'self> ToCStr for &'self str {
     unsafe fn to_c_str_unchecked(&self) -> CString {
         self.as_bytes().to_c_str_unchecked()
     }
+
+    #[inline]
+    fn with_c_str<T>(&self, f: &fn(*libc::c_char) -> T) -> T {
+        self.as_bytes().with_c_str(f)
+    }
+
+    #[inline]
+    unsafe fn with_c_str_unchecked<T>(&self, f: &fn(*libc::c_char) -> T) -> T {
+        self.as_bytes().with_c_str_unchecked(f)
+    }
 }
 
+// The length of the stack allocated buffer for `vec.with_c_str()`
+static BUF_LEN: uint = 128;
+
 impl<'self> ToCStr for &'self [u8] {
     fn to_c_str(&self) -> CString {
         #[fixed_stack_segment]; #[inline(never)];
         let mut cs = unsafe { self.to_c_str_unchecked() };
         do cs.with_mut_ref |buf| {
-            for i in range(0, self.len()) {
-                unsafe {
-                    let p = buf.offset(i as int);
-                    if *p == 0 {
-                        match null_byte::cond.raise(self.to_owned()) {
-                            Truncate => break,
-                            ReplaceWith(c) => *p = c
-                        }
-                    }
-                }
-            }
+            check_for_null(*self, buf);
         }
         cs
     }
@@ -269,6 +282,50 @@ impl<'self> ToCStr for &'self [u8] {
             CString::new(buf as *libc::c_char, true)
         }
     }
+
+    fn with_c_str<T>(&self, f: &fn(*libc::c_char) -> T) -> T {
+        unsafe { with_c_str(*self, true, f) }
+    }
+
+    unsafe fn with_c_str_unchecked<T>(&self, f: &fn(*libc::c_char) -> T) -> T {
+        with_c_str(*self, false, f)
+    }
+}
+
+// Unsafe function that handles possibly copying the &[u8] into a stack array.
+unsafe fn with_c_str<T>(v: &[u8], checked: bool, f: &fn(*libc::c_char) -> T) -> T {
+    if v.len() < BUF_LEN {
+        let mut buf: [u8, .. BUF_LEN] = intrinsics::uninit();
+        vec::bytes::copy_memory(buf, v, v.len());
+        buf[v.len()] = 0;
+
+        do buf.as_mut_buf |buf, _| {
+            if checked {
+                check_for_null(v, buf as *mut libc::c_char);
+            }
+
+            f(buf as *libc::c_char)
+        }
+    } else if checked {
+        v.to_c_str().with_ref(f)
+    } else {
+        v.to_c_str_unchecked().with_ref(f)
+    }
+}
+
+#[inline]
+fn check_for_null(v: &[u8], buf: *mut libc::c_char) {
+    for i in range(0, v.len()) {
+        unsafe {
+            let p = buf.offset(i as int);
+            if *p == 0 {
+                match null_byte::cond.raise(v.to_owned()) {
+                    Truncate => break,
+                    ReplaceWith(c) => *p = c
+                }
+            }
+        }
+    }
 }
 
 /// External iterator for a CString's bytes.
@@ -471,3 +528,131 @@ mod tests {
         assert_eq!(c_str.as_str(), None);
     }
 }
+
+#[cfg(test)]
+mod bench {
+    use iter::range;
+    use libc;
+    use option::Some;
+    use ptr;
+    use extra::test::BenchHarness;
+
+    #[inline]
+    fn check(s: &str, c_str: *libc::c_char) {
+        do s.as_imm_buf |s_buf, s_len| {
+            for i in range(0, s_len) {
+                unsafe {
+                    assert_eq!(
+                        *ptr::offset(s_buf, i as int) as libc::c_char,
+                        *ptr::offset(c_str, i as int));
+                }
+            }
+        }
+    }
+
+    static s_short: &'static str = "Mary";
+    static s_medium: &'static str = "Mary had a little lamb";
+    static s_long: &'static str = "\
+        Mary had a little lamb, Little lamb
+        Mary had a little lamb, Little lamb
+        Mary had a little lamb, Little lamb
+        Mary had a little lamb, Little lamb
+        Mary had a little lamb, Little lamb
+        Mary had a little lamb, Little lamb";
+
+    fn bench_to_str(bh: &mut BenchHarness, s: &str) {
+        do bh.iter {
+            let c_str = s.to_c_str();
+            do c_str.with_ref |c_str_buf| {
+                check(s, c_str_buf)
+            }
+        }
+    }
+
+    #[bench]
+    fn bench_to_c_str_short(bh: &mut BenchHarness) {
+        bench_to_str(bh, s_short)
+    }
+
+    #[bench]
+    fn bench_to_c_str_medium(bh: &mut BenchHarness) {
+        bench_to_str(bh, s_medium)
+    }
+
+    #[bench]
+    fn bench_to_c_str_long(bh: &mut BenchHarness) {
+        bench_to_str(bh, s_long)
+    }
+
+    fn bench_to_c_str_unchecked(bh: &mut BenchHarness, s: &str) {
+        do bh.iter {
+            let c_str = unsafe { s.to_c_str_unchecked() };
+            do c_str.with_ref |c_str_buf| {
+                check(s, c_str_buf)
+            }
+        }
+    }
+
+    #[bench]
+    fn bench_to_c_str_unchecked_short(bh: &mut BenchHarness) {
+        bench_to_c_str_unchecked(bh, s_short)
+    }
+
+    #[bench]
+    fn bench_to_c_str_unchecked_medium(bh: &mut BenchHarness) {
+        bench_to_c_str_unchecked(bh, s_medium)
+    }
+
+    #[bench]
+    fn bench_to_c_str_unchecked_long(bh: &mut BenchHarness) {
+        bench_to_c_str_unchecked(bh, s_long)
+    }
+
+    fn bench_with_c_str(bh: &mut BenchHarness, s: &str) {
+        do bh.iter {
+            do s.with_c_str |c_str_buf| {
+                check(s, c_str_buf)
+            }
+        }
+    }
+
+    #[bench]
+    fn bench_with_c_str_short(bh: &mut BenchHarness) {
+        bench_with_c_str(bh, s_short)
+    }
+
+    #[bench]
+    fn bench_with_c_str_medium(bh: &mut BenchHarness) {
+        bench_with_c_str(bh, s_medium)
+    }
+
+    #[bench]
+    fn bench_with_c_str_long(bh: &mut BenchHarness) {
+        bench_with_c_str(bh, s_long)
+    }
+
+    fn bench_with_c_str_unchecked(bh: &mut BenchHarness, s: &str) {
+        do bh.iter {
+            unsafe {
+                do s.with_c_str_unchecked |c_str_buf| {
+                    check(s, c_str_buf)
+                }
+            }
+        }
+    }
+
+    #[bench]
+    fn bench_with_c_str_unchecked_short(bh: &mut BenchHarness) {
+        bench_with_c_str_unchecked(bh, s_short)
+    }
+
+    #[bench]
+    fn bench_with_c_str_unchecked_medium(bh: &mut BenchHarness) {
+        bench_with_c_str_unchecked(bh, s_medium)
+    }
+
+    #[bench]
+    fn bench_with_c_str_unchecked_long(bh: &mut BenchHarness) {
+        bench_with_c_str_unchecked(bh, s_long)
+    }
+}
diff --git a/src/libstd/rt/crate_map.rs b/src/libstd/rt/crate_map.rs
@@ -209,7 +209,7 @@ fn iter_crate_map_follow_children() {
         let child_crate1 = CrateMapT2 {
             version: 1,
             entries: vec::raw::to_ptr([
-                ModEntry { name: "t::f1".to_c_str().with_ref(|buf| buf), log_level: &mut 1},
+                ModEntry { name: "t::f1".with_c_str(|buf| buf), log_level: &mut 1},
                 ModEntry { name: ptr::null(), log_level: ptr::mut_null()}
             ]),
             children: [&child_crate2 as *CrateMap, ptr::null()]
@@ -219,7 +219,7 @@ fn iter_crate_map_follow_children() {
         let root_crate = CrateMapT2 {
             version: 1,
             entries: vec::raw::to_ptr([
-                ModEntry { name: "t::f1".to_c_str().with_ref(|buf| buf), log_level: &mut 0},
+                ModEntry { name: "t::f1".with_c_str(|buf| buf), log_level: &mut 0},
                 ModEntry { name: ptr::null(), log_level: ptr::mut_null()}
             ]),
             children: [child_crate1_ptr, ptr::null()]
diff --git a/src/libstd/rt/logging.rs b/src/libstd/rt/logging.rs
@@ -294,7 +294,7 @@ fn update_entry_match_full_path() {
                  LogDirective {name: Some(~"crate2"), level: 3}];
     let level = &mut 0;
     unsafe {
-        do "crate1::mod1".to_c_str().with_ref |ptr| {
+        do "crate1::mod1".with_c_str |ptr| {
             let entry= &ModEntry {name: ptr, log_level: level};
             let m = update_entry(dirs, transmute(entry));
             assert!(*entry.log_level == 2);
@@ -310,7 +310,7 @@ fn update_entry_no_match() {
                  LogDirective {name: Some(~"crate2"), level: 3}];
     let level = &mut 0;
     unsafe {
-        do "crate3::mod1".to_c_str().with_ref |ptr| {
+        do "crate3::mod1".with_c_str |ptr| {
             let entry= &ModEntry {name: ptr, log_level: level};
             let m = update_entry(dirs, transmute(entry));
             assert!(*entry.log_level == DEFAULT_LOG_LEVEL);
@@ -326,7 +326,7 @@ fn update_entry_match_beginning() {
                  LogDirective {name: Some(~"crate2"), level: 3}];
     let level = &mut 0;
     unsafe {
-        do "crate2::mod1".to_c_str().with_ref |ptr| {
+        do "crate2::mod1".with_c_str |ptr| {
             let entry= &ModEntry {name: ptr, log_level: level};
             let m = update_entry(dirs, transmute(entry));
             assert!(*entry.log_level == 3);
@@ -343,7 +343,7 @@ fn update_entry_match_beginning_longest_match() {
                  LogDirective {name: Some(~"crate2::mod"), level: 4}];
     let level = &mut 0;
     unsafe {
-        do "crate2::mod1".to_c_str().with_ref |ptr| {
+        do "crate2::mod1".with_c_str |ptr| {
             let entry = &ModEntry {name: ptr, log_level: level};
             let m = update_entry(dirs, transmute(entry));
             assert!(*entry.log_level == 4);
@@ -360,13 +360,13 @@ fn update_entry_match_default() {
                 ];
     let level = &mut 0;
     unsafe {
-        do "crate1::mod1".to_c_str().with_ref |ptr| {
+        do "crate1::mod1".with_c_str |ptr| {
             let entry= &ModEntry {name: ptr, log_level: level};
             let m = update_entry(dirs, transmute(entry));
             assert!(*entry.log_level == 2);
             assert!(m == 1);
         }
-        do "crate2::mod2".to_c_str().with_ref |ptr| {
+        do "crate2::mod2".with_c_str |ptr| {
             let entry= &ModEntry {name: ptr, log_level: level};
             let m = update_entry(dirs, transmute(entry));
             assert!(*entry.log_level == 3);
diff --git a/src/libstd/rt/uv/file.rs b/src/libstd/rt/uv/file.rs
diff --git a/src/libstd/str.rs b/src/libstd/str.rs

Original file line number	Diff line number	Diff line change
`@@ -524,13 +524,13 @@ pub fn set_always_inline(f: ValueRef) {`
`524`	`524`	`}`
`525`	`525`
`526`	`526`	`pub fn set_fixed_stack_segment(f: ValueRef) {`
`527`		`- do "fixed-stack-segment".to_c_str().with_ref \|buf\| {`
	`527`	`+ do "fixed-stack-segment".with_c_str \|buf\| {`
`528`	`528`	`unsafe { llvm::LLVMAddFunctionAttrString(f, buf); }`
`529`	`529`	`}`
`530`	`530`	`}`
`531`	`531`
`532`	`532`	`pub fn set_no_split_stack(f: ValueRef) {`
`533`		`- do "no-split-stack".to_c_str().with_ref \|buf\| {`
	`533`	`+ do "no-split-stack".with_c_str \|buf\| {`
`534`	`534`	`unsafe { llvm::LLVMAddFunctionAttrString(f, buf); }`
`535`	`535`	`}`
`536`	`536`	`}`