specialize ToString for str - #32586
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Travis is unhappy: https://travis-ci.org/rust-lang/rust/builds/119328869 |
|
Thanks for the PR! @rust-lang/libs actually just started talking about whether we want to use specialization in the standard library. The conclusion so far is that we don't want want to make any strong commitments in terms of enabling new functionality just yet (as specialization isn't 100% on the path to stabilization yet), but small perf improvements like this seem reasonable for the standard library to do. I'm personally in favor of a change like this to test out specialization, but if we hit ICEs here and there we should be ready to revert relatively quickly. |
|
Does |
|
Nope - it should probably be change to stable. |
67fac0c to
fc8cf9c
Compare
|
Fixed stability attribute. |
|
The libs team discussed this during triage today and the decision was to merge! This does not expand the standard library's API surface area, provides a clear performance benefit, and it's very difficult to rely on specialization where code would break if we removed it for whatever reason. @aturon was also "very confident" that this would not ICE :) Thanks for the PR @seanmonstar! |
|
@alexcrichton ... I did suggest a crater run ;-) |
|
Oh oops I may have misinterpreted in that case. I'll schedule a crater run. @bors: r- |
|
Crater says zero regressions, yay! |
specialize ToString for str If there was some conditional compiling we could do, such that this impl only exists in nightly, and is turned off in beta/stable, I think that'd be an improvement here, as we could test specialization out without affecting stable builds.
|
Cool! 💀 to the ".to_string()" is slow memes! More procedural question: This type of change makes the blanket ToString impl downstream specializable. Do we want this? What do we need to think of when deciding whether to allow that with a |
|
@bluss Very good question. We talked about this some in a recent libs meeting -- ultimately, we're going to want a conventions RFC here, I think. But it's hard to get there without some experimentation first. |
RFC 1422 gives a general way to limit various permissions. The RFC talks only about permission to access ( In this particular case something like |
|
@petrochenkov You could take the approach in PR #32699 instead if specialization is not open to the outside (use an intermediate internal trait). |
bluss
added
the
relnotes
Three sections, all from notes that kept arriving with the same 2015 advice in them. The style verdict now carries its expiry date. The reasoning everybody repeats — to_string() is generic, goes through Display, may allocate more than once — was true when written and stopped being true in April 2016, when ToString was specialized for str (rust-lang/rust#32586). Measured here on 1.97.1 with -O, the four spellings are within a couple of nanoseconds; format! on a bare literal is the only real outlier at about 30%, and clippy flags that anyway. Also worth writing down: the most-linked benchmark of this question prints target/debug in its own transcript, where the spread is ~2% and cannot support the conclusion drawn from it. What survives is dtolnay's 2017 argument, which is about documentation rather than speed and therefore does not expire. "Can you implement it yourself?" — the two refusals, both reproduced: E0119 for anything Clone (which is nearly everything), and E0277 for a reference-like type, because Owned: Borrow<Self> has nowhere to lend a DataRef from. Hence every std impl being on an unsized referent. A Sized non-Clone type with type Owned = Self does compile, and the example now prints one — it is Clone under another name. The generic trap: <&str as ToOwned>::Owned is &str, not String (rust-lang/rust#31228). Same surprise as section 2b, one level up.
9 participants
If there was some conditional compiling we could do, such that this impl only exists in nightly, and is turned off in beta/stable, I think that'd be an improvement here, as we could test specialization out without affecting stable builds.