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

core and std: Optimize write*!() and print*!() for a single argument #22335

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@erickt
Copy link
Contributor

erickt commented Feb 14, 2015

Before this patch, write!(wr, "foo") was not as fast as wr.write_all("foo".as_bytes()) because the underlying write_fmt has some overhead in order to work with multiple arguments. This PR speeds that up by optimizing this specific case.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 14, 2015

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@cmr

This comment has been minimized.

Copy link
Member

cmr commented Feb 14, 2015

@bors r+ bc7dd05 rollup

@erickt

This comment has been minimized.

Copy link
Contributor Author

erickt commented Feb 14, 2015

@bors r-

Ugh. This is actually a breaking change. Doing write!(wr, "{{") would now produce "{{" instead of "{". I'll have to tweak this

@erickt

This comment has been minimized.

Copy link
Contributor Author

erickt commented Feb 14, 2015

Turns out panic! right now also has this issue. panic!("{") produces a single "{", panic!("{{") produces "{{", panic!("{ {}", 5) produces a compile time error, and panic!("{{ {}", 5) produces "{ 5".

@pczarn

This comment has been minimized.

Copy link
Contributor

pczarn commented Feb 15, 2015

format_args! could expand to a single string literal.

@erickt

This comment has been minimized.

Copy link
Contributor Author

erickt commented Feb 16, 2015

@pczarn: Unfortunately then format_args!("foo") would be then returning a different type than format_args!("{}", 5), which would cause all sorts of havoc. I think we instead need a format_arg!(fmt) that uses the same syntax as format_args! but only allows for one argument. I have that implemented and I'll push it up in a few moments after I make sure it passes the tests locally first.

@erickt erickt force-pushed the erickt:single-write branch 2 times, most recently from c6ad5eb to a3e5cb3 Feb 16, 2015

///
/// ```
#[macro_export]
macro_rules! format_arg { ($fmt:expr) => ({

This comment has been minimized.

@huonw

huonw Feb 16, 2015

Member

I suspect the two cases need to be in the same macro definition to be rendered,

macro_rules! format_args {
     ($fmt: expr) => ({ /* compiler built-in */ });
     ($fmt: expr, $($args: tt)*) => ({ /* compiler built-in */ });
}

This comment has been minimized.

@huonw

huonw Feb 16, 2015

Member

@erickt pointed out my mistake on IRC (interpreted this just a variation of format_args rather than a new format_arg macro).

In any case, I think the doc string for this needs updating, since AFAICT, it doesn't return Arguments any more.

@erickt erickt force-pushed the erickt:single-write branch from a3e5cb3 to d69728b Feb 16, 2015

core and std: add format_arg!() to speed up write*!() and print*!()
Before this patch, `write!(wr, "foo")` was not as fast as
`wr.write_str("foo")` because the underlying write_fmt has some overhead
in order to work with multiple arguments.

@erickt erickt force-pushed the erickt:single-write branch from d69728b to ee7ab9a Feb 16, 2015

@erickt

This comment has been minimized.

Copy link
Contributor Author

erickt commented Feb 16, 2015

@huonw: I also have a branch that merges format_arg!() and format_args!() if we want to go that direction.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 16, 2015

If this is adding a new format_arg! macro then I think this needs an RFC behind it (as it's adding a new macro to the prelude).

#[stable(feature = "rust1", since = "1.0.0")]
macro_rules! format {
($fmt:expr) => {
format_arg!($fmt).to_string()

This comment has been minimized.

@pczarn

pczarn Feb 16, 2015

Contributor

How is this different from format_args!($fmt, $($arg)*).to_string()?

@pczarn

This comment has been minimized.

Copy link
Contributor

pczarn commented Feb 16, 2015

@erickt: you can add a method for extracting the only string piece out of fmt::Arguments and avoid adding a new macro.

@erickt

This comment has been minimized.

Copy link
Contributor Author

erickt commented Feb 17, 2015

@pczarn: doing that would be doing roughly the same amount of work Writer::write_fmt does, so at that point it'd be better to stick with what we've got right now. If we don't want this optimization, it's easy enough to call Writer::write_str. I'll close this for now in deference for an RFC, since I don't think I'd be able to do this without adding a new macro, or changing format_args to work differently for a single format string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.