Skip to content

Conversation

ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Jul 27, 2018

Simple benchmarks suggest in some cases it can be faster by even 37%:

test converting_f64_long  ... bench:         339 ns/iter (+/- 199)
test converting_f64_short ... bench:         136 ns/iter (+/- 34)
test converting_i32_long  ... bench:          87 ns/iter (+/- 16)
test converting_i32_short ... bench:          87 ns/iter (+/- 49)
test converting_str       ... bench:          54 ns/iter (+/- 15)
test formatting_f64_long  ... bench:         349 ns/iter (+/- 176)
test formatting_f64_short ... bench:         145 ns/iter (+/- 14)
test formatting_i32_long  ... bench:          98 ns/iter (+/- 14)
test formatting_i32_short ... bench:          93 ns/iter (+/- 15)
test formatting_str       ... bench:          86 ns/iter (+/- 23)

@rust-highfive
Copy link
Contributor

r? @petrochenkov

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2018
@killercup
Copy link
Member

Most likely the wrong place to ask this: Can we special-case the format macro for the format!("{}", something) case and expand it to something.to_string() for some known types like &str, String, Cow<str>, and friends? Avoiding this performance trap might be worth the hassle.

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 28, 2018

@killercup the cases you are describing should already be covered by the useless_format clippy lint. I haven't seen one for non-string types implementing Display, but there already seems to be a related issue in the clippy repo.

@killercup
Copy link
Member

killercup commented Jul 28, 2018

@ljedrz a lint is not the same as making this macro do "the right thing" automatically. But this is a drive-by comment, at best. I'll look for/open a new issue.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 29, 2018

📌 Commit 57a5a9b has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2018
@bors
Copy link
Collaborator

bors commented Jul 29, 2018

⌛ Testing commit 57a5a9b with merge 023fd7e...

bors added a commit that referenced this pull request Jul 29, 2018
Prefer to_string() to format!()

Simple benchmarks suggest in some cases it can be faster by even 37%:
```
test converting_f64_long  ... bench:         339 ns/iter (+/- 199)
test converting_f64_short ... bench:         136 ns/iter (+/- 34)
test converting_i32_long  ... bench:          87 ns/iter (+/- 16)
test converting_i32_short ... bench:          87 ns/iter (+/- 49)
test converting_str       ... bench:          54 ns/iter (+/- 15)
test formatting_f64_long  ... bench:         349 ns/iter (+/- 176)
test formatting_f64_short ... bench:         145 ns/iter (+/- 14)
test formatting_i32_long  ... bench:          98 ns/iter (+/- 14)
test formatting_i32_short ... bench:          93 ns/iter (+/- 15)
test formatting_str       ... bench:          86 ns/iter (+/- 23)
```
@bors
Copy link
Collaborator

bors commented Jul 29, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 023fd7e to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants