Skip to content

Rewrite fmt! for performance #3571

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

Closed
brson opened this issue Sep 24, 2012 · 5 comments
Closed

Rewrite fmt! for performance #3571

brson opened this issue Sep 24, 2012 · 5 comments
Labels
A-frontend Area: Compiler frontend (errors, parsing and HIR) I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@brson
Copy link
Contributor

brson commented Sep 24, 2012

fmt was written in the stone ages and is not fast. It should be rewritten to use either an io::Writer or some kind of string builder, and it should use stack allocation everywhere it can. It should not emit lots of inline code to bloat up function bodies, instead delegating to core::extfmt functions marked with #[inline], leaving the amount of inlining up to LLVM.

@brson
Copy link
Contributor Author

brson commented Sep 24, 2012

It should also probably use traits to work with more types, per #1653

bors added a commit that referenced this issue Mar 22, 2013
This is a minor step towards #3571, although I'm sure there's still more work to be done. Previously, `fmt!` collected a bunch of strings in a vector and then called `str::concat`. This changes the behavior by maintaining only one buffer and appending directly into that buffer. This avoids doubly-allocating memory, and it has the added bonus of reducing some allocations in `core::unstable::extfmt`

One of the unfortunate side effects of this is that the `rt` module in `extfmt.rs` had to be duplicated to avoid `stage0` errors. Dealing with the change in conversion functions may require a bit of a dance when a snapshot happens, but I think it's doable.

If the second speedup commit isn't deemed necessary, I got about a 15% speedup with just the first patch which doesn't require any modification of `extfmt.rs`, so no snapshot weirdness.

Here's some other things I ran into when looking at `fmt!`:
* I don't think that #2249 is relevant any more except for maybe removing one of `%i` or `%d`
* I'm not sure what was in mind for using traits with #3571, but I thought that formatters like `%u` could invoke the `to_uint()` method on the `NumCast` trait, but I ran into some problems like those in #5462

I'm having trouble thinking of other wins for `fmt!`, but if there's some suggestions I'd be more than willing to look into if they'd work out or not.
@catamorphism
Copy link
Contributor

Not critical for 0.6, de-milestoning

@catamorphism
Copy link
Contributor

This doesn't seem to have been nominated, but had a milestone attached. Does this really need to be on production-ready? Is fmt! usually a huge performance bottleneck? Re-nominating so we can discuss.

@graydon
Copy link
Contributor

graydon commented Jun 20, 2013

accepted for backwards-compatible milestone

@alexcrichton
Copy link
Member

This is currently underway with the new ifmt! syntax extension. While not completely done, I think that the remaining progress of that extension is better tracked in the #2249 metabug.

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) I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

No branches or pull requests

4 participants