Rewrite FormatArg::Display
#26
Conversation
It only had one useful function: `Display::fmt()`. Instead of that write a function that returns the `String` directly. I claim that this code is significantly more efficient.
There was a problem hiding this comment.
Thanks!
I think the reason i made it this way is because i wanted to also support other types that String. Ideally i would want the macro to return an impl Display or something like that.
But anyway, you're right that since it always creates a String, this is fine.
The only problem I see is the minor problem is the missing version in Cargo.toml
| pub fn display_string(format_str: &str, args: &[(&str, &dyn ::std::fmt::Display)]) -> String { | ||
| use ::std::fmt::Write; | ||
| let fmt_len = format_str.len(); | ||
| let mut res = String::with_capacity(2 * fmt_len); |
There was a problem hiding this comment.
Not sure about the capacity here. It is hard to know what's the require capacity since we don't know the size of the arguments. I would have let the default string growing algorithm do its job. But it's true that 2 times the format length is probably fine.
There was a problem hiding this comment.
Yeah, there is a trade-off here. I would set at least fmt_len, because the final string will almost never be shorter than the format one. If anything {} could get an empty string, but that is just two bytes less.
The thing is that if you write tr!("a {} b {} c {} d"), you will do 7 write operations, and I guess at least 3 allocations, depending on the length of the arguments.
I've run my benchmarks many times and 2 * fmt_len seems to be the sweet spot.
And since then I've been reading the std library; it uses this handy but undocumented Arguments::estimated_capacity: it does some smart heuristics, and... hey! it doubles some length 🤓!
If you agree, I'd leave it as is.
rodrigorc
force-pushed
the
RewriteFormatArgDisplay
branch
from
527772f to
9746aef
Compare
|
thanks! |
Currently, the only use of the internal
FormatArgtype is as the single argument to the callformat!("{}", ...)inside theruntime_format!macro.And the only reason
FormatArgimplementsfmt::Displayis to be able to be used in such a place.IMO, it would be easier and more efficient just to have a function that builds the final
Stringwithout all thefmtmechanisms. Except for the user-provided arguments, of course, that are still&dyn Display.Since now we are writing into a
Stringwe can pre-allocate its memory, further improving the performance. I'm currently allocating twice the size of the format string, that it looks to me a nice sweet spot.In this PR I've included two commits: the first one implements that change; the second one adds a benchmark (with crate
criterion) to confirm the runtime improvements. In my machine I've measured an improvement between 10% and 25%.I understand that you may prefer the old code over this hypothetical performance improvement. If so, feel free to disregard this PR, no worries. Also should you decide to merge it, you could remove the benchmark, if you think it is unnecessary, it is here mostly for show.
Usually performance in translations is not so important, but I'm using it in a Dear ImGui application, so I'm constantly translating strings, 60 whole UIs per second.
And with this one I reach the end of my PR spree. For now 😄.