Change the format_args! macro expansion for temporaries

[edwardw]

Currently, the format_args! macro and its downstream macros in turn
expand to series of let statements, one for each of its arguments, and
then the invocation of the macro function. If one or more of the
arguments are RefCell's, the enclosing statement for the temporary of
the let is the let itself, which leads to scope problem. This patch
changes let's to a match expression.

Closes #12239.

[alexcrichton]

This looks like it translates where the right-most expression being formatted is evaluated first and then the left-most expression is evaluated last? I think we should keep the order of evaluation from left-to-right.

[edwardw]

Good catch.

[alexcrichton]

In the past I've heard of codegen blowup whenever we're dealing with match statements. Do you have any numbers showing the codegen impact of this strategy vs the let strategy?

When I was first writing the format! code I had a test with 100k calls to format!("foo {}", "bar") and I timed the compilation time and file size both optimized and unoptimized. It would be nice to see the impact of this approach.

Additionally, I'm worried about the codegen time with lots of nested match statements, so could you test the impact of 5 or more arguments as well?

[edwardw]

Also, what you were looking for in the original ticket is:

static PRECOMPILED_FORMAT_STRING = ...;
match foo {
    ref foo_addr => {
        let arguments: &fmt::Arguments = unsafe { /* do things with foo_addr */ };
        fmt::writeln(logger, arguments)
    }
}

But the patch translated the macro to:

match foo {
    ref foo_addr => {
        static PRECOMPILED_FORMAT_STRING = ...;
        let arguments: &fmt::Arguments = unsafe { /* do things with foo_addr */ };
        fmt::writeln(logger, arguments)
    }
}

I had some mysterious errors when compiling librustdoc to the first format. Hope this is okay.

[alexcrichton]

Ah indeed that's fine, it doesn't matter so much where the format string goes as long as it's in its own scope.

[edwardw]

Alas, 10k lines of format! blew my 4G ram macbook away easily and sent it into crazy swap, let alone 100k. So I settled on 5k lines simple format! and 2k lines of complex one.

                time      file size(bytes)
5k lines of format!("foo {}", "bar")
before w/o opt  1m3.6s    4853288
after w/o opt   1m9.1s    4853296
before w/t -O   1m23.3s   3231128
after w/t -O    1m24.2s   3231136

2k lines of format! with 10 arguments, 5 unnamed and 5 named
before w/o opt  3m28.2s   7659744
after w/o opt   4m47.5s   8079328
before w/t -O   3m48.6s   3613024
after w/t -O    5m10s     3613024

[edwardw]

Will matching against tuples be a improvement?

match (x, y, z) {
    (ref x_addr, ref y_addr, ref z_addr) => {
        ...
    }
}

[huonw]

That changes the semantics (it moves x, y and z): it would have to be match (&x, &y, &z) { ... }.

[edwardw]

Ah, of course. I guess watching out for the move semantics of rust still isn't a knee jerk reflex for me, yet.

[alexcrichton]

Those codegen times are worrying, although they're not quite as bad as I expected. I expect build times to increase as a result of this patch, but perhaps the semantic differences are better such that the build time increase is worth it for now.

[edwardw]

I'm fiddling with the match unrolling idea. Hope it'll achieve something.

[edwardw]

I tried to construct the following snippet: match &[&x, &y, &z] => { [xx, yy, zz] => ... } with

let (idents, args) = get id and expression list somehow;
let pat = self.ecx.pat(self.fmtsp, ast::PatVec(
                    idents.map(|&x| self.ecx.pat_ident(self.fmtsp, x)),
                    None, ~[]));
let arm = self.ecx.arm(self.fmtsp, ~[pat], body);
let exp = self.ecx.expr_vec_slice(
                    self.fmtsp,
                    args.map(|&x| self.ecx.expr_addr_of(self.fmtsp, x)));
self.ecx.expr_match(self.fmtsp, exp, ~[arm])

It seems to work only for match &[&x] => { [xx] => ... } but not others. What might be wrong?

[edwardw]

Never mind, I think I figured it out. Testing locally and benchmarking now.

[huonw]

Why not just all of them at once?

[edwardw]

Can I? Isn't a tuple able to have 12 elements at most?

[huonw]

No, they're arbitrarily large, but the stdlib only has traits for some convenient methods up to 12 (because if you're using larger tuples you've got bigger problems than the sugar that those traits provide).

[edwardw]

Then I shall just shovel them together.

[edwardw]

The compiling time certainly improved.

after-1 = naive match expression
after-2 = one match nested level every 8 arguments

                time      file size(bytes)
5k lines of format!("foo {}", "bar")
before w/o opt  1m3.6s    4853288
after-1 w/o opt 1m9.1s    4853296
after-2 w/o opt 1m10s     5041808
before w/t -O   1m23.3s   3231128
after-1 w/t -O  1m24.2s   3231136
after-2 w/t -O  1m26s     3231136

2k lines of format! with 10 arguments, 5 unnamed and 5 named
before w/o opt  3m28.2s   7659744
after-1 w/o opt 4m47.5s   8079328
after-2 w/o opt 3m36s     8349816
before w/t -O   3m48.6s   3613024
after-1 w/t -O  5m10s     3613024
after-2 w/t -O  3m49s     3613024

[alexcrichton]

I'm a little surprised that the tuple method worked, but nice thinking! Those numbers don't look too bad to me at all!

[edwardw]

Comments addressed, commits squashed and rebased. It should be ready for merging.

[alexcrichton]

amazing results, nice work!

[alexcrichton]

Looks like this needs a pub fn main on the tests (for the check-fast runner)

[edwardw]

It is now pub fn main. r?

[huonw]

Can you put a comment about what and why is going on here? (i.e. why we need a match, and why that match needs to be one big tuple.)

[edwardw]

Certainly.

[edwardw]

Done. Is it a little bit verbose?

[edwardw]

r? @huonw