Micro-optimize vec.with_c_str for short vectors #9352
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -217,6 +224,45 @@ impl<'self> ToCStr for &'self [u8] { | |||
| CString::new(buf as *libc::c_char, true) | |||
| } | |||
| } | |||
|
|
|||
| /// WARNING: This function uses an optimization to only malloc a temporary | |||
| /// CString when the source string is small. Do not save a reference to | |||
There was a problem hiding this comment.
You write small but I think you mean large, but does the doc have to spill the guts here? Either the small or the large string case, the CString or buffer is just a temporary and the pointer will be invalid after the closure exits, that's all the user should need to know
|
This is ready for review again. It also adds a couple other things. First, it bumps the size of the buffer to 128 bytes. Next, it adds the micro-optimization to |
| @@ -145,7 +154,7 @@ pub trait ToCStr { | |||
| fn to_c_str(&self) -> CString; | |||
|
|
|||
| /// Unsafe variant of `to_c_str()` that doesn't check for nulls. | |||
There was a problem hiding this comment.
This still mentions Unsafe. Also, could you expand the documentation to explain what happens when there is actually an interior null?
|
@cmr: I'm not yet ready to agree, but I'll factor out the |
|
|
||
| fn with_c_str<T>(&self, f: &fn(*libc::c_char) -> T) -> T { | ||
| if self.len() < BUF_LEN { | ||
| do self.as_imm_buf |self_buf, self_len| { |
There was a problem hiding this comment.
Can this avoid unsafe code and use:
let mut buf = ...;
vec::bytes::copy_memory(buf, self.as_bytes());
buf[self.len()] = 0;
do buf.as_imm_buf |buf, _| {
// etc
}?
This now makes it unsafe to save the pointer returned by .with_c_str as that pointer now may be pointing at a stack allocated array. I arbitrarily chose 32 bytes as the length of the stack vector, and so it might not be the most optimal size. before: test c_str::bench::bench_with_c_str_long ... bench: 539 ns/iter (+/- 91) test c_str::bench::bench_with_c_str_medium ... bench: 97 ns/iter (+/- 2) test c_str::bench::bench_with_c_str_short ... bench: 70 ns/iter (+/- 5) after: test c_str::bench::bench_with_c_str_long ... bench: 542 ns/iter (+/- 13) test c_str::bench::bench_with_c_str_medium ... bench: 53 ns/iter (+/- 6) test c_str::bench::bench_with_c_str_short ... bench: 19 ns/iter (+/- 0)
The documentation for `.with_c_str()` already says that the pointer will be deallocated before returning from the function.
This matches @graydon's recommendation.
before: test c_str::bench::bench_with_c_str_unchecked_long ... bench: 361 ns/iter (+/- 9) test c_str::bench::bench_with_c_str_unchecked_medium ... bench: 75 ns/iter (+/- 2) test c_str::bench::bench_with_c_str_unchecked_short ... bench: 60 ns/iter (+/- 9) after: test c_str::bench::bench_with_c_str_unchecked_long ... bench: 362 ns/iter (+/- test c_str::bench::bench_with_c_str_unchecked_medium ... bench: 30 ns/iter (+/- 7) test c_str::bench::bench_with_c_str_unchecked_short ... bench: 12 ns/iter (+/- 4)
This also fixes a bug in `vec.with_c_str_unchecked` where we were not calling `.to_c_str_unchecked()` for long strings.
bors
added a commit
that referenced
this pull request
Sep 27, 2013
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One of the downsides with
c_stris 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 forvec.with_c_strin 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.