Skip to content

Speed up fmt! by 20% #5463

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

Merged
merged 3 commits into from
Mar 22, 2013
Merged

Speed up fmt! by 20% #5463

merged 3 commits into from
Mar 22, 2013

Conversation

alexcrichton
Copy link
Member

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'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.

@pcwalton
Copy link
Contributor

The biggest win for fmt! is just going to be to add functions that also write directly to io::Writers instead of building up intermediate strings.

meth: ast::ident, +args: ~[@ast::expr]) -> @ast::expr {
return mk_expr(cx, sp, ast::expr_method_call(expr, meth, ~[], args,
ast::NoSugar));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this function? I'm not seeing it called from anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh whoops, I added that in for a previous scheme I was doing, but you're right that it's no longer necessary so I'll remove it.

@alexcrichton
Copy link
Member Author

I tried to do something like that by passing the string buffer to as many functions as possible, but to account for things like padding and numbers being generated from right-to-left, I'm not sure how many more intermediate strings can be eliminated.

Right now the only intermediate string is actually when formatting numbers, and I found it difficult to change that because of the way the numbers are generated, and then also having to deal with padding. For other things, strings always have to copy memory, bools copy from static memory, and poly conversions have the same problem as numbers where to do padding the intermediate form is necessary.

One thing I can try is a sort of "fast path" for conversions which don't have any padding, because those can mostly just be emitted directly into the string buffer. That would probably require a lot of changes though, so I'll open another pull request later with that if this goes through.

@pcwalton
Copy link
Contributor

Sure, that's fine if some things have to build intermediate strings (although it might be nice to have a SmallString optimization so that small strings can be stored on the stack and only spill to the heap if necessary). What I was referring to is that instead of building up a buffer the extfmt functions should take an &io::Writer.

@alexcrichton
Copy link
Member Author

Oh I thought it was only possible to create @ instances of traits to pass around, I'm not even aware of how it's possible to create an &io::Writer

I just tried doing something like

fn foo(a: &'b io::Writer) {      
}                                

fn bar(b: &'b io::BytesWriter) { 
  foo(b as &'b io::Writer);      
}                                

fn main() {                      
}                                

But I got an ICE from it. Is there a way to create traits as borrowed pointers?

@catamorphism
Copy link
Contributor

@alexcrichton The intent is that it should be possible, but currently there are some open issues about objects with storage other than @.

@alexcrichton
Copy link
Member Author

Oh ok, I'll try to keep an eye out for when that comes around and see if this can be modified a bit. I know that conv_poly would benefit immediately from passing an io::Writer instance instead of using a string buffer, and perhaps numbers could be worked on as well. Thanks for the clarification!

@bstrie
Copy link
Contributor

bstrie commented Mar 22, 2013

Has bors already been over this pull request twice? If there are still changes yet to be merged, I think it would be better to rebase and open a new pull request rather than continue pushing new commits to this one. I'm honestly not sure if bors is smart enough to understand the latter.

@alexcrichton
Copy link
Member Author

Bors takes care of this just fine as far as I can tell. I just takes this series of commits (whatever is current) and plants it on top of incoming, naming the result 'auto'. I've had a couple pull requests in the past which you just have to continually rebase to keep up with merge conflicts.

In this case, bors dealt with it twice, but both had build failures (which should be fixed now). This should be ready for another go at this point.

@alexcrichton
Copy link
Member Author

Fourth time's the charm, right?

bors added a commit that referenced this pull request 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.
@bors bors closed this Mar 22, 2013
@bors bors merged commit e93654c into rust-lang:incoming Mar 22, 2013
@alexcrichton alexcrichton deleted the faster-fmt branch March 22, 2013 18:21
oli-obk pushed a commit to oli-obk/rust that referenced this pull request May 2, 2020
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.

6 participants