Do not drop_in_place elements of Vec<T> if T doesn't need dropping #28531
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
With -O2, LLVM's inliner can remove this code, but this does not happen with -O1 and lower. As a result, dropping Vec<u8> was linear with length, resulting in abysmal performance for large buffers.
| if unsafe { needs_drop::<T>() } { | ||
| for x in self.iter_mut() { | ||
| unsafe { drop_in_place(x); } | ||
| } |
There was a problem hiding this comment.
Can you change the unsafe block to wrap the whole if needs_drop block here? This is the preferred style.
|
We have in the past been pretty hesitant to add any form of contortion to optimize for O0 or O1 because it can easily make code obfuscated or the optimizations are quite brittle and don't survive well over time. That being said I think it has become more important over time to scrutinize O0 and O1 performance at least a little, so I'm more comfortable with this now. I would personally want to require at least a comment in every location that does this indicating that the extra code is not necessary, but this is an O0 or O1 performance improvement. I would like to talk about this a little more broadly, however, so I'm going to bring this up at the next libs triage meeting we get a chance to. I am concretely worried about providing any guarantees about O0 or O1 performance, and I don't think we're ready for a large influx of "improve O1 performance" PRs. This may be the right time, but it's something we should decide first rather than letting a trickle start out first. |
alexcrichton
added
the
T-libs-api
|
Updated to address review. |
|
style 👍 |
|
@alexcrichton Perhaps a line could be drawn somewhere along algorithmic complexity vs. constant factor improvements? |
|
Yeah this seems like a seriously catastrophic problem for debug perf, and I can dream to dream that this will put a bit less stress on the optimizer. @bors r+ |
|
📌 Commit 77f5da7 has been approved by |
With -O2, LLVM's inliner can remove this code, but this does not happen with -O1 and lower. As a result, dropping Vec<u8> was linear with length, resulting in abysmal performance for large buffers. See issue #24280.
brson
added
the
relnotes
With -O2, LLVM's inliner can remove this code, but this does not happen
with -O1 and lower. As a result, dropping Vec was linear with length,
resulting in abysmal performance for large buffers.
See issue #24280.