Avoid many allocations for CStrings during codegen. #53161
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
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
|
⌛ Trying commit 9ca04a1e0e052f3fa03248828c56b3148a643178 with merge 6d5276398f760025f61c4579a7c475fedd1b06bb... |
kennytm
added
the
S-waiting-on-perf
|
☀️ Test successful - status-travis |
|
@rust-timer build 6d5276398f760025f61c4579a7c475fedd1b06bb |
|
Success: Queued 6d5276398f760025f61c4579a7c475fedd1b06bb with parent 18925de, comparison URL. |
|
|
||
| let str_plus_nul = concat!($s, "\0"); | ||
|
|
||
| if cfg!(debug_assertion) { |
There was a problem hiding this comment.
I think this needs to be debug_assertions?
There was a problem hiding this comment.
It does. Good catch!
|
Results are in. Looks alright from the instructions view, not so sure otherwise. |
kennytm
removed
the
S-waiting-on-perf
O right, this is going to be 8 byte aligned, not 4. Yeah, doing either 30 or 38 seems good. All in all, I'm not sure that the "gains" are worth the churn. cc @rust-lang/wg-compiler-performance |
|
Overall it seems like a slight improvement to me. I agree with the |
|
I'd feel more comfortable with somebody from @rust-lang/wg-compiler-performance doing the final review. It seems to me that the only outstanding task is changing the |
wesleywiser
reviewed
Aug 8, 2018
|
|
||
| /// Declare a global value. | ||
| /// | ||
| /// If there’s a value with the same name already declared, the function will | ||
| /// return its Value instead. | ||
| pub fn declare_global(cx: &CodegenCx<'ll, '_>, name: &str, ty: &'ll Type) -> &'ll Value { | ||
| debug!("declare_global(name={:?})", name); | ||
| let namebuf = CString::new(name).unwrap_or_else(|_|{ | ||
| bug!("name {:?} contains an interior null byte", name) |
There was a problem hiding this comment.
I think we should try to retain this bug reporting code. It was introduced in f1dabed and caught an issue as recently as June 23. I'd recommend SmallCStr::new() returns an Option<SmallCStr> with its internal unwraps() moved back into the callsites.
There was a problem hiding this comment.
I'd like to avoid moving the unwraps() to the call site, since there are so many of them an you can't recover gracefully anyway. How about I change the unwrap() in SmallCStr::new() into a bug!() with more useful information? Together with the backtrace that should point you to the right spot pretty quickly.
| @@ -61,9 +59,7 @@ fn declare_raw_fn( | |||
| ty: &'ll Type, | |||
| ) -> &'ll Value { | |||
| debug!("declare_raw_fn(name={:?}, ty={:?})", name, ty); | |||
| let namebuf = CString::new(name).unwrap_or_else(|_|{ | |||
| bug!("name {:?} contains an interior null byte", name) | |||
| @@ -214,9 +210,7 @@ pub fn define_internal_fn( | |||
| /// Get declared value by name. | |||
| pub fn get_declared_value(cx: &CodegenCx<'ll, '_>, name: &str) -> Option<&'ll Value> { | |||
| debug!("get_declared_value(name={:?})", name); | |||
| let namebuf = CString::new(name).unwrap_or_else(|_|{ | |||
| bug!("name {:?} contains an interior null byte", name) | |||
| @@ -348,7 +349,7 @@ fn vec_slice_metadata( | |||
| let (pointer_size, pointer_align) = cx.size_and_align_of(data_ptr_type); | |||
| let (usize_size, usize_align) = cx.size_and_align_of(cx.tcx.types.usize); | |||
|
|
|||
| let member_descriptions = [ | |||
| let member_descriptions = vec
There was a problem hiding this comment.
If you want to go crazy, the LLVM C++ method we're using is called through a C wrapper that we define ourselves and uses a StringRef, which is a pointer/length pair and doesn't actually need a terminating nul byte. So you could get theoretically completely avoid CStr for the name (and possibly other places as well). Of course, if / when the DebugInfo stuff gets an official C API, we'd likely be back at using plain C strings.
There was a problem hiding this comment.
If you want to go crazy
I don't :P
nnethercote
reviewed
Aug 9, 2018
| // option. This file may not be copied, modified, or distributed | ||
| // except according to those terms. | ||
|
|
||
| #[macro_export] |
There was a problem hiding this comment.
Please add a comment explaining what this macro does.
DHAT is a good tool for finding out which heap allocations matter. |
Thanks for the hint, @nnethercote, I'll add it to my tool chest. |
michaelwoerister
force-pushed
the
cstrings
branch
from
9ca04a1 to
b8529c6
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
michaelwoerister
force-pushed
the
cstrings
branch
from
b8529c6 to
7e1faee
Compare
|
Let's see if this works: |
|
✌️ @wesleywiser can now approve this pull request |
|
I've never done this before. Do I just need to say? @bors r+ |
|
📌 Commit 7e1faee8b0945929a11c075d6e53664fc76b6ba4 has been approved by |
bors
added
S-waiting-on-bors
|
👍 |
|
@bors r- Merge conflict. (🤔 bors isn't detecting any merge conflict recently...) |
bors
added
S-waiting-on-author
michaelwoerister
force-pushed
the
cstrings
branch
from
7e1faee to
f2ede4d
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
michaelwoerister
force-pushed
the
cstrings
branch
from
f2ede4d to
88d84b3
Compare
|
Rebased. @bors r=wesleywiser |
|
📌 Commit 88d84b3 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 13, 2018
Avoid many allocations for CStrings during codegen. Giving in to my irrational fear of dynamic allocations. Let's see what perf says to this.
|
☀️ Test successful - status-appveyor, status-travis |
eddyb
reviewed
Aug 18, 2018
| unsafe { | ||
| CStr::from_bytes_with_nul_unchecked(str_plus_nul.as_bytes()) | ||
| } | ||
| } |
There was a problem hiding this comment.
cc @oli-obk this really wants to be a static assert, heh.
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.
Giving in to my irrational fear of dynamic allocations. Let's see what perf says to this.