Reduce fmt's rodata usage by unzipping String and Argument arrays #16662
Conversation
|
Awesome wins! |
ExampleExpansion of old fn main() {
let x = 5i;
match (&x,) {
(__arg0,) => {
static __STATIC_FMTSTR: [Piece, ..3] = [
String("x is: "),
Argument(Argument {
position: ArgumentNext,
format: FormatSpec {
fill: ' ',
align: AlignUnknown,
flags: 0u,
precision: CountImplied,
width: CountImplied,
},
},
String(" foobar"),
];
let __args_vec = &[argument(secret_string, __arg0)];
let __args = unsafe { Arguments::new(__STATIC_FMTSTR, __args_vec) };
println_args(&__args)
}
};
}With this PR it becomes: fn main() {
let x = 5i;
match (&x,) {
(__arg0,) => {
static __STATIC_FMTSTR: [&'static str, ..2u] = [
"x is: ",
" foobar"
];
static __STATIC_FMTARGS: [rt::Argument<'static>, ..0u] = []; // optimized out
let __args_vec = &[argument(secret_show, __arg0)];
let __args = unsafe {
Arguments::new(__STATIC_FMTSTR, __STATIC_FMTARGS, __args_vec)
};
println_args(&__args)
}
};
}WinsDifference (negative is better)DSTs could further reduce code bloat, in a simple way and without harmful indirection: struct Pieces<'a> {
args: &'a [Argument],
strings: [&'a str]
}
static __STATIC_PIECES: Pieces = // ...
let __args = Arguments::new(__STATIC_PIECES, __args_vec);
// ...I'm wondering whether to use vtables or store |
| try!(formatter.run(arg)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Out of curiosity, have you measured a runtime improvement with these two paths?
There was a problem hiding this comment.
Not yet. By the way, these unwraps seem wrong, because formatting is used from fail!.
There was a problem hiding this comment.
Without a clear difference between them, could we unify the two paths? Part of the contract of calling this function is that Arguments is valid where the number of string pieces is very close to the number of argument pieces. This is always valid when generated by the syntax extension, so unwrap should be ok.
There was a problem hiding this comment.
One iterates over arguments, the other over placeholder specs.
As for the improvement:
#[bench]
fn show_hashmap(b: &mut Bencher) {
let mut map = HashMap::new();
for i in range(100u, 500) { map.insert(i, i + 1); }
let mut buf = [0, ..4096];
b.iter(|| {
let mut w = BufWriter::new(buf);
(write!(w, "{}", map)).unwrap()
})
}before
test show_hashmap ... bench: 49661 ns/iter (+/- 6403)
after
test show_hashmap ... bench: 46245 ns/iter (+/- 6816)
|
Awesome wins @pczarn, nice work! |
Really? Even |
|
@huonw, in these cases string pieces can be empty: Edit: or |
| @@ -54,6 +54,8 @@ struct Context<'a, 'b> { | |||
|
|
|||
| /// Collection of the compiled `rt::Piece` structures | |||
| pieces: Vec<Gc<ast::Expr>>, | |||
| str_pieces: Vec<Gc<ast::Expr>>, | |||
| all_pieces_simple: bool, | |||
There was a problem hiding this comment.
Can you add doc comments for these fields? (e.g. explaining what 'simple' means here.)
Can be? or are? I'm asking because this (adjacent formatting) seems like something that breaks your stated assumption and thus means this code is broken. E.g. |
|
They are, your example will print |
|
Ok, I don't quite see where the change to include the empty #[inline]
#[allow(dead_code)]
static __STATIC_FMTSTR: [::std::fmt::rt::Piece<'static>, ..4u] =
[::std::fmt::rt::String("foo"),
::std::fmt::rt::Argument(::std::fmt::rt::Argument{ ... }),
::std::fmt::rt::Argument(::std::fmt::rt::Argument{ ... }),
::std::fmt::rt::String("baz")];
let __args_vec =
&[::std::fmt::argument(::std::fmt::secret_show, __arg0),
::std::fmt::argument(::std::fmt::secret_show, __arg1)];
let __args =
unsafe {
::std::fmt::Arguments::new(__STATIC_FMTSTR, __args_vec)
};
::std::io::stdio::println_args(&__args) |
| secret_lower_hex::<uint>(&(*self as uint), f) | ||
| let res = secret_lower_hex::<uint>(&(*self as uint), f); | ||
| f.flags = 0; | ||
| res |
There was a problem hiding this comment.
This change worries me and makes me realize that this is a breaking change due to the differing semantics. It didn't look like there was much of a performance improvement over just calling fmt.arg, could there perhaps be a static instance of Argument representing the default formatting?
There was a problem hiding this comment.
Yes, should I make a static instance or rather an impl of Default for Argument? Another thing that worries me is the change of the signature of public-facing Arguments::new.
There was a problem hiding this comment.
Either way is fine, and it's ok to not worry about Arguments::new as it shouldn't be used/public anyway.
|
Looks great, thanks @pczarn! Just a few more minor comments, and then I think this is good to go with some squashing of the intermediate commits. |
pczarn
force-pushed
the
format-fmtstr-opt
branch
from
0b8cd02
to
11afef3
Compare
|
This is good to go |
Format specs are ignored and not stored in case they're all default. Restore default formatting parameters during iteration. Pass `None` instead of empty slices of format specs to take advantage of non-nullable pointer optimization. Generate a call to one of two functions of `fmt::Argument`.
pczarn
force-pushed
the
format-fmtstr-opt
branch
from
11afef3
to
fcf88b8
Compare
Based on an observation that strings and arguments are always interleaved, thanks to #15832. Additionally optimize invocations where formatting parameters are unspecified for all arguments, e.g. `"{} {:?} {:x}"`, by emptying the `__STATIC_FMTARGS` array. Next, `Arguments::new` replaces an empty slice with `None` so that passing empty `__STATIC_FMTARGS` generates slightly less machine code when `Arguments::new` is inlined. Furthermore, formatting itself treats these cases separately without making redundant copies of formatting parameters. All in all, this adds a single mov instruction per `write!` in most cases. That's why code size has increased.
Add test explorer This PR implements the vscode testing api similar to rust-lang#14589, this time using a set of lsp extensions in order to make it useful for clients other than vscode, and make the vscode client side logic simpler (its now around ~100 line of TS code) Fix rust-lang#3601
Some minor changes in the test explorer lsp extension followup rust-lang#16662 cc `@nemethf` `@ShuiRuTian`
Based on an observation that strings and arguments are always interleaved, thanks to #15832. Additionally optimize invocations where formatting parameters are unspecified for all arguments, e.g.
"{} {:?} {:x}", by emptying the__STATIC_FMTARGSarray. Next,Arguments::newreplaces an empty slice withNoneso that passing empty__STATIC_FMTARGSgenerates slightly less machine code whenArguments::newis inlined. Furthermore, formatting itself treats these cases separately without making redundant copies of formatting parameters.All in all, this adds a single mov instruction per
write!in most cases. That's why code size has increased.