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

Investigate the issue of Show bloat #15548

Closed
emberian opened this issue Jul 9, 2014 · 18 comments
Closed

Investigate the issue of Show bloat #15548

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

Comments

@emberian
Copy link
Member

emberian commented Jul 9, 2014

In rustc, many types derive Show purely for use with debug!. This may be negatively impacting compiletime, as we are generating and translating code that we really don't care about most of the time. Is this causing noticable impact, and do we want to do anything about it?

@emberian emberian added I-slow and removed A-codegen labels Jul 9, 2014
@emberian
Copy link
Member Author

emberian commented Jul 9, 2014

One possibility is add a #[deriving(DebugShow)] that is not emitted when --cfg ndebug (or today's equivalent) is used. It would then be the programmer's responsibility to ensure they are not using it outside of debug! logging.

@emberian
Copy link
Member Author

emberian commented Jul 9, 2014

cc @huonw

@huonw
Copy link
Member

huonw commented Jul 10, 2014

We could do this via (hypothetical) conditional attributes

#[cfg_attr(not(ndebug), deriving(Show))]
struct Foo { ... }

or even a more specific version: #[debug_only(deriving(Show))].

cfg_attr/debug_only can be done with a ItemModifier syntax extension, but it may be ugly to hard code it into libsyntax (it could be e.g. #[__cfg_attr(...)] for now, or just go via an RFC) and doing it as a rustc-specific thing may hit staging issues.

(Theoretically we should be bootstrapping/testing with ndebug anyway.)

@huonw
Copy link
Member

huonw commented Jul 10, 2014

I guess we could even have

#[cfg(not(ndebug))]
macro_rules! debug_show {
    ($($it: item)*) => {
        $(
          #[deriving(Show)] $it
        )*
    }
}
#[cfg(not(ndebug))]
macro_rules! debug_show {
    ($($it: item)*) => {
        $( $it )*
    }
}

debug_show! {
    struct Foo { ... }
    enum Bar { ... }
}

This would be easy to be specific to libsyntax/librustc and doesn't require an external plugin, but is also kinda ugly.

@lilyball
Copy link
Contributor

Any sort of debug-only Show impl means clients of your crate can't debug using your types. For example, clients of libsyntax won't be able to debug libsyntax-provided values if libsyntax adopted this sort of DebugShow.

Any sort of compile-time solution to this problem is very problematic. It involves allowing each client crate to re-derive its own Show instance and I can't see that working well.

Perhaps the best approach is to expand TyDesc to also provide info about specially-marked traits (which in this case would just be Show). If the debug reflection can construct a &Show at runtime for a type that implements it, then it could use this to print a derived-Show-like representation of a type at runtime without having to derive it at compile-time. A big advantage of this approach is it should work for any type without having to modify the type definition, which is a big help when debugging values from other crates (e.g. clients of libsyntax printing AST values).

We could also add a DebugShow trait that would be used instead of Show if implemented, thus allowing types to provide more information about themselves for debugging purposes.

@lilyball
Copy link
Contributor

Note: I have not actually looked at TyDesc or the runtime reflection in libdebug, so I'm making some assumptions here about the plausibility of adding the Show info.

@emberian
Copy link
Member Author

Sure they can, just don't pass --cfg ndebug when the crate is built. This
is not much different than building an asserts or debuginfo build.

On Fri, Jul 11, 2014 at 7:07 PM, Kevin Ballard notifications@github.com
wrote:

Note: I have not actually looked at TyDesc or the runtime reflection in
libdebug, so I'm making some assumptions here about the plausibility of
adding the Show info.


Reply to this email directly or view it on GitHub
#15548 (comment).

http://octayn.net/

@lilyball
Copy link
Contributor

@cmr If I have a no-debug version of Rust on my system (FWIW offhand I don't remember anymore if the default Rust configuration uses --cfg ndebug), and I want to write a program that uses libsyntax, and I need to debug some of the values libsyntax gave me, recompiling and reinstalling all of Rust is a bit of a serious hurdle.

@emberian
Copy link
Member Author

I don't really see that as a problem. That's how all C and C++ software
works in practice. I suppose whether regressing to C/C++'s level is worth
it or not depends on how much bloat this actually causes. Someone should
investigate ;)

On Fri, Jul 11, 2014 at 7:24 PM, Kevin Ballard notifications@github.com
wrote:

@cmr https://github.com/cmr If I have a no-debug version of Rust on my
system (FWIW offhand I don't remember anymore if the default Rust
configuration uses --cfg ndebug), and I want to write a program that uses
libsyntax, and I need to debug some of the values libsyntax gave me,
recompiling and reinstalling all of Rust is a bit of a serious hurdle.


Reply to this email directly or view it on GitHub
#15548 (comment).

http://octayn.net/

@lilyball
Copy link
Contributor

@cmr You don't typically have to recompile the language to turn on debugging functionality :P

@huonw
Copy link
Member

huonw commented Jul 12, 2014

You're not recompiling the language; you're just getting debug builds of the libraries you're using (which happen to be the compiler/parser). It's perfectly possible to build only those 2 crates in debugging mode directly, without a full bootstrap.

@bgamari
Copy link
Contributor

bgamari commented Jul 12, 2014

I'm currently characterizing the effect of my changes from #15585 on build time and sizes. Let's see what the data says.

@bgamari
Copy link
Contributor

bgamari commented Jul 14, 2014

#15585 derives 19 Show instances in librustc. To test the effect of these on build time, I manually compiled the librustc crate using rustc d24b828 and this script. In particular, I examined the build times before and after b99310b6d.

The effect on build time is within the (rather large) variance in build time on my test box (uncertainties are standard deviation over multiple builds),

(N=140)              commit     wall time (s)
without Show         98cd620    157.14 ± 6.67
with Show            b99310b    178.71 ± 15.09

The effect on the compiler's maximum RSS is negligible,

(N=140)              commit     maximum RSS (MB)
without Show         98cd620    1913.6 ± 0.60
with Show            b99310b    1917.5 ± 1.14

The instances add roughly 150 kilobytes to the resulting binary size,

(N=1)                commit     librustc.so (kB)
without Show         98cd620    66483.2
with Show            b99310b    66634.6

@bluss
Copy link
Member

bluss commented Jul 14, 2014

Is there any way to make the generated formatting code iself simpler?

An escaped curly always splits a String formatting piece for logical reasons, so Show for structs begins and ends with double fmt::rt::String pieces. Maybe these could be joined together in libsyntax/ext/format (seems to be the more convenient place).

Also would it be beneficial to replace the many FormatSpec struct literals with a reference to a single fmt::rt::FormatSpecDefault if all fields are default?

@alexcrichton
Copy link
Member

Certainly! The fmt::rt bits and pieces are basically all experimental, so we're free to frob their format as much as we like.

Both of those suggestions sound like excellent improvements.

@malbarbo
Copy link
Contributor

malbarbo commented Dec 2, 2016

@alexcrichton I think this issue can be closed.

@alexcrichton
Copy link
Member

Indeed!

@steveklabnik
Copy link
Member

Blast from the past 🚀

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 18, 2023
Restructure some modules in rust-analyzer crate
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
Development

No branches or pull requests

8 participants