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

debug_assert and debug logging shouldn't be enabled by default in optimized builds #17081

Closed
thestinger opened this issue Sep 7, 2014 · 5 comments
Labels
A-frontend Area: Compiler frontend (errors, parsing and HIR) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@thestinger
Copy link
Contributor

At the moment, using debug assertions is discouraged because it will hurt performance. In practice, no one disables assertions so they are not used: debug_assert! only has 11 uses in the Rust repository. It would have many more if I didn't feel I had to delete all of the assertions I used during development to avoid a 5-20% performance hit by default.

It would make a lot more sense to enable them by default only in non-optimized builds where the performance hit will be dwarfed by other issues. It would be overridden by passing an explicit --cfg switch, and Rust's build system could just use the default. Rust developers could easily override this, but users and packagers wouldn't be given a slow / bloated build by default.

This would make it acceptable to use debug_assert! for bounds checks in the unchecked indexing methods on slices and the very valuable jemalloc debugging assertions could also be enabled by default in an unoptimized build. Using the debug variant of mutexes rather than faster deadlocking ones is another example. There are bots building and testing with no optimizations, so there would be better testing coverage.

@thestinger thestinger added A-frontend Area: Compiler frontend (errors, parsing and HIR) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Sep 7, 2014
@gamazeps
Copy link
Contributor

gamazeps commented Sep 8, 2014

On it :)

@bluss
Copy link
Member

bluss commented Sep 9, 2014

cargo build --release does the right thing, so it is already quite viable to use debug_assert! in the users' own packages.

@sfackler
Copy link
Member

sfackler commented Sep 9, 2014

As @bluss noted, cargo already passes --cfg ndebug for release builds. The most straightforward fix here for rust itself would probably be to have --enable-optimize imply --disable-debug.

@thestinger
Copy link
Contributor Author

I think rustc's behaviour should be consistent with the higher-level Cargo semantics, not just Rust's own build system.

@thestinger thestinger changed the title debug_assert shouldn't be enabled by default in optimized builds debug_assert and debug logging shouldn't be enabled by default in optimized builds Sep 15, 2014
@thestinger
Copy link
Contributor Author

This will be the only major reason for #8859 after my latest heap pull request lands. Debug logging causes an enormous start-up time hit on Windows due to relocations from the global state.

Manishearth added a commit to Manishearth/rust that referenced this issue Apr 9, 2015
 This makes the default configuration fully optimized, with no debugging options, no llvm asserts, renames --enable-debug to --enable-debug-assertions, and adds --enable-debug as a blanket option that toggles various things, per rust-lang#17665. It does not add a `--enable-release` flag since that would be a no-op.

cc @nrc

Fixes rust-lang#22390
Fixes rust-lang#17081
Partially addresses rust-lang#17665
bors added a commit that referenced this issue Apr 9, 2015
This makes the default configuration fully optimized, with no debugging options, no llvm asserts, renames --enable-debug to --enable-debug-assertions, and adds --enable-debug as a blanket option that toggles various things, per #17665. It does not add a `--enable-release` flag since that would be a no-op.

cc @nrc

Fixes #22390
Fixes #17081
Partially addresses #17665
@bors bors closed this as completed in 2dffe78 Apr 9, 2015
lnicola pushed a commit to lnicola/rust that referenced this issue Apr 20, 2024
…kril

Revert rust-lang#17073: Better inline preview for postfix completion

See discussion on rust-lang/rust-analyzer#17077, but I strongly suspect that the changes to the `TextEdit` ranges caused VS Code's autocomplete to prefer the snippets over method completions. I explain why I think that [here](rust-lang/rust-analyzer#17077 (comment)).
lnicola pushed a commit to lnicola/rust that referenced this issue Apr 20, 2024
…kril

Revert rust-lang#17073: Better inline preview for postfix completion

See discussion on rust-lang/rust-analyzer#17077, but I strongly suspect that the changes to the `TextEdit` ranges caused VS Code's autocomplete to prefer the snippets over method completions. I explain why I think that [here](rust-lang/rust-analyzer#17077 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: Compiler frontend (errors, parsing and HIR) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants