Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize vec.truncate and destructors for types with no dtor at -O0 #17633

Closed
bvssvni opened this issue Sep 29, 2014 · 9 comments
Closed

Optimize vec.truncate and destructors for types with no dtor at -O0 #17633

bvssvni opened this issue Sep 29, 2014 · 9 comments
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@bvssvni
Copy link

bvssvni commented Sep 29, 2014

The internal representation of String is Vec<u8>, but vec.truncate loops to drop members.

Perhaps setting the length directly?

@rust-slacker
Copy link

Maybe vec.truncate could use std::intrinsics::needs_drop::<T>() to decide if the loop is needed?

@thestinger
Copy link
Contributor

It's already optimized out when optimizations are enabled. These loops could use needs_drop to improve performance when optimizations are disabled, but I don't think it's very important.

@thestinger thestinger added the I-slow Issue: Problems and improvements with respect to performance of generated code. label Sep 29, 2014
@bvssvni
Copy link
Author

bvssvni commented Sep 30, 2014

vec.truncate leads to 70% decreased performance in some cases for game development, where compiling with optimization has negative impact on productivity.

@bvssvni bvssvni changed the title Optimize String truncate Optimize vec.truncate Sep 30, 2014
@nightpool
Copy link

@bvssvni Even just O1 isn't reasonable for development?

@bvssvni
Copy link
Author

bvssvni commented Oct 1, 2014

It would be nice if adding a profile to Cargo.toml was not required for a such simple case.

@rust-slacker
Copy link

Would it make sense to have cargo default to O1?

@thestinger thestinger changed the title Optimize vec.truncate Optimize vec.truncate and destructors for types with no dtor at -O0 Oct 16, 2014
@thestinger
Copy link
Contributor

@rust-slacker: LLVM only fully supports debugging at -O0, unlike GCC which supports it at all optimization levels via a lot of work integrating support in passes.

veddan added a commit to veddan/rust that referenced this issue Oct 16, 2014
This significantly improves the performance of `Vec::truncate` for types
without destructors when building with `--opt-level=0` or
`--opt-level=1`. Fixes rust-lang#17633.
@pcwalton
Copy link
Contributor

Closing; I don't think we necessarily want to do this.

@kmeisthax
Copy link

Fun fact: I may or may not have spent a few hours banging my head against performance issues that ultimately were resolved by replacing a truncate with a set_len. (See this commit)

Yeah, I know the optimizer is supposed to handle it, and in a perfect world that would be it, but...

  • The procedure for enabling the optimizer is non-obvious to programmers learning Rust and constitutes a DX hazard. It's technically mentioned in the book but as an option that can be easily skimmed over, not a critical part of Rust's performance strategy.
  • As mentioned elsewhere, LLVM doesn't support debugging optimized code. If you cannot debug optimized code, then the optimizer is not useful for developers!
  • You're moving the mental overhead of the check from core Rust to user code (like I had to do, replacing a truncate call with a set_len call).
    • Worse, someone might actually fork Vec and publish it on crates.io. Minor dependency proliferation isn't a good thing - do we want to become npm?
  • The whole damned point of Rust is low-overhead, memory-safe native code. Objecting to removing overhead for the sake of "the optimizer will take care of it" is extremely un-Rust-like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
6 participants