Implement RFC 640 by sfackler · Pull Request #23162 · rust-lang/rust

sfackler · 2015-03-08T03:38:51Z

I've made some minor changes from the implementation attached to the RFC to try to minimize codegen. The methods now take &Debug trait objects rather than being parameterized and there are inlined stub methods that call to non-inlined methods to do the work.

r? @alexcrichton

cc @huonw for the derive(Debug) changes.

brson · 2015-03-08T05:16:18Z

re https://github.com/rust-lang/rfcs/blob/master/text/0640-debug-improvements.md

alexcrichton · 2015-03-08T17:49:46Z

I'll take a closer look at this tomorrow, but I'm curious why you applied #[inline] attributes at all? It looks like none of these methods require inlining (e.g. none have type parameters) and if you avoid the #[inline] attribute entirely then they'll all just naturally not be inlined because they're across the crate boundary.

sfackler · 2015-03-08T18:22:46Z

The public API takes self by value so that we can return the fmt::Result at the call to finish. But if we didn't inline it at all there'd be overhead from repeatedly copying the builder struct around. The current setup has inlineable stub functions that take self by value and do nothing but call a private non-inlineable method that takes &mut self and then returns self. The idea is that the end result is as if the API took &mut self at the assembly level while behaving as if it takes self.

alexcrichton · 2015-03-10T02:37:59Z

But if we didn't inline it at all there'd be overhead from repeatedly copying the builder struct around.

Ah ok. If we're trying to minimize codegen though I would probably recommend a &mut self-based builder as it would involve far fewer moves. The finish method could replace the inner result with Ok(()), for example, so it doesn't necessarily have to consume.

In general I'm not so sure about the application of #[inline] throughout these methods. I would be much more convinced with some data though! Without some statistics in favor of the #[inline], I think I'd personally prefer they'd all be removed for now. One interesting statistic would be the before/after size of librustdoc. I know that the lib has probably hundreds of #[derive(Debug)] and if this cuts down on codegen it should really help that size.

Otherwise, my only other concern is the usage in #[derive]. I'm a little unsure about how we lint stability in #[derive]-based implementations, but I just wanted to confirm that this does not require the core feature gate? I personally don't necessarily like the compiler being able to call unstable internals (e.g. the expanded AST won't actually compiled b/c it calls unstable code) where other code cannot, but it's also something we're at liberty to change so I'd be fine giving this a whirl to see what happens!

sfackler · 2015-03-10T02:42:44Z

I will collect some statistics.

The deriving implementation uses the functionality @huonw added to be able to call unstable APIs without triggering feature gates.

alexcrichton · 2015-03-10T03:03:23Z

Ah excellent. cc @huonw on the #[derive] changes then, but I think they're all pretty ok

huonw · 2015-03-10T03:34:44Z

Did you find that this was necessary? I'm under the impression that the macro infrastructure manages re-spanning everything after a macro expansion?

It did not appear to respan - I think only the macro_rules! infrastructure may do that.

huonw · 2015-03-10T03:36:30Z

Looks great. Is it worth putting these under a different feature gate to core?

Switching from generic bounds to trait objects and having un-inlined inner methods should cut down on the size of Debug impls, since we care about the speed of a Debug implementation way less than binary bloat.

Turns out it's basically a wash, codegen wise.

sfackler · 2015-03-11T04:33:35Z

So it turns out that no-inline silliness actually ends up a tiny bit smaller than inline silliness: https://gist.github.com/sfackler/a9300bce13ad166cd35b. Looks like the builder structs are small enough that the copies don't matter.

I pushed a commit that cuts out all the inline annotations.

sfackler · 2015-03-11T04:36:27Z

Oh, and there doesn't appear to be much of a difference one way or another for overall binary sizes before and after the derive(Debug) change.

alexcrichton · 2015-03-11T18:31:03Z

So it turns out that no-inline silliness actually ends up a tiny bit smaller than inline silliness

Thanks for investigating!

Oh, and there doesn't appear to be much of a difference one way or another for overall binary sizes before and after the derive(Debug) change.

Ah well, it probably at least paves the way in the future for such changes though.

alexcrichton · 2015-03-11T18:32:14Z

This looks great to me! r=me with a feature name that's not core (e.g. debug_builders or something like that)

sfackler · 2015-03-11T19:59:36Z

@bors r=alexcrichton 905a611

bors · 2015-03-12T02:31:33Z

⌛ Testing commit 905a611 with merge eef7330...

@alexcrichton

I've made some minor changes from the implementation attached to the RFC to try to minimize codegen. The methods now take `&Debug` trait objects rather than being parameterized and there are inlined stub methods that call to non-inlined methods to do the work. r? @alexcrichton cc @huonw for the `derive(Debug)` changes.

bors · 2015-03-12T04:30:46Z

💔 Test failed - auto-linux-32-nopt-t

sfackler · 2015-03-12T04:33:51Z

@bors retry

Seems like a spurious error??

bors · 2015-03-12T04:48:28Z

⚡ Previous build results for auto-linux-32-opt, auto-linux-64-nopt-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt...

bors · 2015-03-12T04:51:32Z

⛄ The build was interrupted to prioritize another pull request.

bors · 2015-03-12T04:51:34Z

⚡ Previous build results for auto-linux-32-opt, auto-linux-64-nopt-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt...

bors · 2015-03-12T04:53:29Z

⛄ The build was interrupted to prioritize another pull request.

@alexcrichton

I've made some minor changes from the implementation attached to the RFC to try to minimize codegen. The methods now take `&Debug` trait objects rather than being parameterized and there are inlined stub methods that call to non-inlined methods to do the work. r? @alexcrichton cc @huonw for the `derive(Debug)` changes.

bors · 2015-03-12T07:30:45Z

⌛ Testing commit 905a611 with merge 49f7550...

bors · 2015-03-12T10:05:12Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

SimonSapin · 2015-04-02T07:43:34Z

Should assert_eq! default to using this? (I just found out about this, it looks great!)

liigo · 2015-07-08T03:36:20Z

keywords: pretty debug builders
rfc: rust-lang/rfcs#640
(to help people finding this rfc implementation)

sunjay · 2016-06-04T04:24:34Z

@SimonSapin It would be great is assert_eq! used this. I don't know what a good way to integrate it in my tests right now without that.

rust-highfive assigned alexcrichton Mar 8, 2015

sfackler force-pushed the debug-builders branch from adb6db6 to f61cd26Compare March 8, 2015 05:19

huonw reviewed Mar 10, 2015
View reviewed changes

sfackler added 4 commits March 9, 2015 23:24

Implement RFC 640
e3656bd

Restructure debug builders to minimize codegen
8121cf0
Switching from generic bounds to trait objects and having un-inlined inner methods should cut down on the size of Debug impls, since we care about the speed of a Debug implementation way less than binary bloat.

Switch derive(Debug) to use the debug builders
bd6ed22

Fix unstable span checks
3181213

sfackler force-pushed the debug-builders branch from f61cd26 to 3181213Compare March 11, 2015 02:48

Remove inline silliness from debug builders
4181d43
Turns out it's basically a wash, codegen wise.

Switch to a specific feature
905a611

bors merged commit 905a611 into rust-lang:masterMar 12, 2015

bors mentioned this pull request Mar 12, 2015
[RFC] Split Copy trait in two traits: Pod and Copy#23016
Closed

alexcrichton mentioned this pull request Mar 17, 2015
Implement Debug Improvments #23083
Closed

SimonSapin mentioned this pull request Apr 2, 2015
Pretty formating #10068
Closed

liigo mentioned this pull request Jul 8, 2015
Debug improvements rust-lang/rfcs#640
Merged

sfackler deleted the debug-builders branch June 4, 2016 04:40

Uh oh!

Uh oh!

Conversation

sfackler commented Mar 8, 2015

Uh oh!

brson commented Mar 8, 2015

Uh oh!

alexcrichton commented Mar 8, 2015

Uh oh!

sfackler commented Mar 8, 2015

Uh oh!

alexcrichton commented Mar 10, 2015

Uh oh!

sfackler commented Mar 10, 2015

Uh oh!

alexcrichton commented Mar 10, 2015

Uh oh!

huonwMar 10, 2015

Choose a reason for hiding this comment

Uh oh!

sfacklerMar 10, 2015

Choose a reason for hiding this comment

Uh oh!

huonw commented Mar 10, 2015

Uh oh!

sfackler commented Mar 11, 2015

Uh oh!

sfackler commented Mar 11, 2015

Uh oh!

alexcrichton commented Mar 11, 2015

Uh oh!

alexcrichton commented Mar 11, 2015

Uh oh!

sfackler commented Mar 11, 2015

Uh oh!

bors commented Mar 12, 2015

Uh oh!

bors commented Mar 12, 2015

Uh oh!

sfackler commented Mar 12, 2015

Uh oh!

bors commented Mar 12, 2015

Uh oh!

bors commented Mar 12, 2015

Uh oh!

bors commented Mar 12, 2015

Uh oh!

bors commented Mar 12, 2015

Uh oh!

bors commented Mar 12, 2015

Uh oh!

bors commented Mar 12, 2015

Uh oh!

SimonSapin commented Apr 2, 2015

Uh oh!

liigo commented Jul 8, 2015

Uh oh!

sunjay commented Jun 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants