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

libcore: Make fail handlers optional #18277

Closed
wants to merge 2 commits into from

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Oct 24, 2014

Some users, particularly those targeting embedded systems, would rather avoid depending on fmt due to its effect on code size. See #17886.

@alexcrichton
Copy link
Member

Could you expand a little more on how you see this working? I thought libcore wouldn't compile without this fake module?

Some users have no need for formatted fail messages and really can't
bear the code sizes that they require. See rust-lang#17886.
@bgamari
Copy link
Contributor Author

bgamari commented Oct 24, 2014

Whoops, yes, you are right. My flag apparently wasn't making it through the Zinc build system to rustc. Sorry for the noise.

I just pushed another attempt which this time I confirmed was built with --cfg=no_fail_fmt. This reduced text sizes by nearly 2 kB.

@alexcrichton
Copy link
Member

In the PR message you explained the motivation for this as "some users would rather define their own fail handlers," but as written this hard-codes all failure to intrinsics::abort which can't be overridden (unlike defining the fail_fmt lang item), was that intentional?

@bgamari
Copy link
Contributor Author

bgamari commented Oct 24, 2014

@alexcrichton it wasn't intentional but it happened ;) I neglected to update the PR text. It has since been fixed. That being said, it is a bit unfortunate that we now lose the ability to define custom fail handlers. I added a patch on top to instead just eliminate the arguments of format_args!. This preserves a good fraction of the savings in binary size that the first approach offered (1096 bytes vs. 916 bytes on a simple blink example with division) while retaining the ability to override fail_impl.

At this point I'm weakly in support of this second approach as I think we'll really want the ability to override fail_impl. If you agree I'll squash the commits and we can continue from there.

@alexcrichton
Copy link
Member

Interesting! With the goal of getting as small as possible here, I'm surprised that only removing the formatting messages from these failure functions is sufficient to fit your constraints. When you say 1096 bytes to 916 bytes, is that a before/after with respect to the entire patch or just the first/second strategy?

I'm curious about what's taking up the most space here which you you want to eliminate. The factors which contribute to the size of the code that I know of are:

  1. The file name of the failure invocation point
  2. The __STATIC_FMTSTR array which consists of pieces of string literals in format strings
  3. The code to generate fmt::Arguments
  4. The _FILE_LINE statics generated from failure locations.

Of these, the current PR only eliminates 2/3 for the two functions in core::failure, which I would have expected to be a practically miniscule contribution. Does it actually end up being much larger?

I suppose I could summarize my question as follows: Do you have some more data about the before/after picture here with respect to both the approaches? (forbidding overriding or just removing msgs)

Also, as a side note, while you're modifying these could you change the -> () in the closures to -> ! (now that we support it), that should also allow removing the calls to intrinsics::unreachable().

@bgamari
Copy link
Contributor Author

bgamari commented Oct 24, 2014

@alexcrichton here is a more thorough tabulation (looking at zinc with rake -j4 PLATFORM=k20 build_blink_k20),

branch .text .bss
no exceptions 836 bytes 8 bytes
neither patch 2568 bytes 8 bytes
first patch 916 bytes 8 bytes
both patches 1096 bytes 8 bytes

@alexcrichton
Copy link
Member

Wow, I would most certainly not expect the current patch to be under half the size of the original one. Could you past the before/after IR? I'm curious as to what's causing all the extra .text

@farcaller
Copy link
Contributor

I don't have IR at hand, but I remember that bloat is introduced by *fmt* symbols, error description strings and file path strings. Ideally we'd like to get rid of all those.

@bgamari
Copy link
Contributor Author

bgamari commented Oct 25, 2014

@alexcrichton I haven't yet generated IR but looking at objdump output is fairly enlightening. Comparing patch 2 against master a few obvious differences pop out; namely these symbols appear,

00000635 l     F .text  000002de _ZN3fmt11secret_show21h15661710013944803792E
00000915 l     F .text  0000014c _ZN3fmt22Formatter$LT$$x27a$GT$12with_padding20h568554db2fd92d406qzE
00000a61 l     F .text  00000022 _ZN3fmt22Formatter$LT$$x27a$GT$3pad13closure.22207E
00000c40 l     O .text  00000014 _ZN3str39_$BP$$x27a$x20str.StrSlice$LT$$x27a$GT$11slice_chars14_MSG_FILE_LINE20hf187f39f6e2e05067xrE
00000bf0 l     O .text  00000014 _ZN3str39_$BP$$x27a$x20str.StrSlice$LT$$x27a$GT$11slice_chars14_MSG_FILE_LINE20hf187f39f6e2e0506HxrE
00000ba0 l     O .text  00000014 _ZN3str3raw11slice_bytes14_MSG_FILE_LINE20hf187f39f6e2e050657qE
00000b60 l     O .text  00000014 _ZN3str3raw11slice_bytes14_MSG_FILE_LINE20hf187f39f6e2e0506A7qE

By my tally those constitute 1180 bytes of text.

These also appear,

00000b04 l     O .text  00000000 str22101
00000b10 l     O .text  00000027 str22153
00000b40 l     O .text  0000001e str22165
00000b80 l     O .text  00000020 str22166
00000bc0 l     O .text  0000002c str22180
00000c10 l     O .text  0000002a str22181

Which is another 187 bytes for a total of 1367 bytes.

@alexcrichton
Copy link
Member

The plot thickens! So to explain my impetus for these questions, I do find it unfortunate that we have a mode for libcore where failure produces blank failure messages where it otherwise has useful contextual information about what happened. The size does seem worrying, however, and I'd love to optimize it!

It looks like the largest contributor is the fmt::secret_show which is generated once per {}-formatted argument. In this case it's probably pulling in impl Show for &str as they're both used in core::failure. Some of the more expensive asserts or codegen bloats may be:

  • The Show impl for &str properly handles things like padding and such, so the Formatter::pad function is used.
  • This padding function isn't exactly small and contains a few vectors of failure itself:

Would it be possible to investigate optimizing the size of the padding functions before adding a switch to turn off the error messages entirely? One possible route to take is to define struct SimpleFormat<'a>(&'a str) inside of core::failure (privately) and the Show impl just calls write directly instead of padding (b/c it knows it has no padding). That should bypass all the fmt infrastructure by default (for at least the simple use case), and it should also preserve error messages.

The more difficult route (but more general) would be optimizing the padding function itself. The first step here would probably be removing as many points of "failure" as possible, with the next being optimizing the amount of code. This would apply more generally and would help keep all binaries a tad bit smaller perhaps?

How does that sound?

@farcaller
Copy link
Contributor

@alexcrichton, one of the things is that the system might run completely "headless" (no way to return any string of text to the user), or spend most of the boot time headless. Having any dependencies to pull in text strings and formatting is still dead weight, and in some cases each byte counts.

@alexcrichton
Copy link
Member

Taking that to the extreme then the no_fail_fmt cfg flag should rewrite fail!() and assert!() to not pass strings at all but simply just call intrinsics::abort(). It looked like @bgamari was saying that it was adequate to just remove the formatting from core::failure, so I was hoping that tweaking the design there would prevent pulling in lots of formatting by default.

@alexcrichton
Copy link
Member

Closing due to inactivity (and @bors's queue is quite full!). Feel free to reopen with rebase!

@bgamari
Copy link
Contributor Author

bgamari commented Nov 16, 2014

Alex Crichton notifications@github.com writes:

Closing due to inactivity (and @bors's queue is quite full!). Feel
free to reopen with rebase!

Sure, I've been meaning to do a more thorough investigation of the
contributions to the binary size but life has been getting in the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants