[proof of concept] give String support for CoW wrapping of literals #46993

Gankra · 2017-12-25T03:10:50Z

This is a proof of concept implementation for people who want to test this out. It isn't fully tested, and the RawVec changes probably need more work. In addition the implementation isn't as optimized as it could be (see FIXME's).

This adds a String::literally constructor that wraps up a literal without allocating. It in turn makes Vec partially support this (with more robust capacity checking) and RawVec partially support this (by copying the source if len > cap). String also gains some make_unique guards for methods which don't naturally do this themselves (ones which make mutable views or mutate the middle of the buffer).

rust-highfive · 2017-12-25T03:11:01Z

r? @sfackler

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

Gankra · 2017-12-25T03:11:33Z

cc @withoutboats @aturon

Gankra · 2017-12-25T03:16:17Z

How to test this: replace instances of:

String::from("...")
"...".to_string()
"...".to_owned()

with String::literally("...")

ghost · 2017-12-25T05:11:17Z

src/liballoc/raw_vec.rs

-                        Ok(ptr) => (new_cap, ptr),
-                        Err(e) => self.a.oom(e),
+                    if used_cap == 0 {
+                        // skip to 4 because tiny Vec's are dumb; but not if that


Isn't it "Vecs", not "Vec's"?

You only need the apostrophe if the "Vec" is possessing something.

this is a pre-existing comment, but yes

ghost · 2017-12-25T05:17:11Z

src/liballoc/string.rs

@@ -384,6 +384,38 @@ impl String {
        String { vec: Vec::new() }
    }

+    #[inline]
+    #[unstable(feature = "lit_strings", reason = "its TOO lit", issue = "42069")]


You might want to change this to something more informative.

This isn't intended to be landed any time soon, so I opted for a joke :)

This isn't intended to be landed any time soon

Do you say this because there is more implementation work to be done, or because it hasn't been decided that this is a good thing? (I'm pretty out of the loop.) If the former, cool. If the latter, has it been discussed somewhere?

There's a thread on internals about landing this feature as a coercion (rather than a method): https://internals.rust-lang.org/t/pre-rfc-allowing-string-literals-to-be-either-static-str-or-string-similar-to-numeric-literals/5029

Oh hey, that's pretty cool! Thanks.

Manishearth · 2017-12-25T05:33:50Z

https://github.com/manishearth/rust/tree/literally-sed has it all sed-ed. The sed commands are in each commit (gsed is GNU sed, which is default on most distros but not on OSX)

There's a ^\([^\/]*\)" in it so that we don't do replacements on doc comments.

shepmaster · 2018-01-06T02:38:20Z

I don't really know how to triage this. You uh... hmm. Uh, what do you think we should do?

aturon · 2018-01-06T03:54:17Z

@shepmaster I'm gonna mark as waiting on (libs) team. cc @rust-lang/libs

aturon · 2018-01-09T22:00:53Z

@gankro

The libs team discussed this PR some today, and the following points came up:

What happened to concerns around unsafe code making assumptions about the capacity? Wasn't that the killer last time around?
People are generally fine with the DerefMut implementation potentially allocating, but this would mean that String would not implement DerefPure, if that becomes a thing. It was unclear exactly what the implications would be.
More generally, are we concerned about code assuming a stable address?

Modulo these concerns, no one saw a blocking objection, and we'd love to see this proceed to an RFC.

Gankra · 2018-01-09T22:17:08Z

We only found examples of people assuming vec's capacity. We halted progress because it was possible to use String in the same way. It still is. That's one of the reasons this PR exists: so we can test this change out on other crates.

Sadly it's hard to cargo-bomb this since nothing in the ecosystem uses it, so nothing would trip it!

I am vaguely considered about stable address assumptions, but also we could reasonably just phase it out of the ecosystem, like we've done for other implementation-defined assumptions that we've broken (heap::EMPTY).

ghost · 2018-01-09T23:13:47Z

What happened to concerns around unsafe code making assumptions about the capacity? Wasn't that the killer last time around?

Unless I misunderstood something, the assumption in question is s.len() <= s.capacity().

This PR breaks the assumption, but I believe it's easy to avoid breaking it. The trick is to implement String::capacity as:

fn capacity(&self) -> usize {
    cmp::max(self.vec.capacity(), self.vec.len())
}

This way the assumption is still valid, and we can still internally disambiguate owned and literal strings by checking self.vec.capacity() == 0.

Gankra · 2018-01-09T23:21:24Z

No, this will break anyone using the _raw apis.

withoutboats · 2018-01-09T23:22:08Z

More generally, are we concerned about code assuming a stable address?

Rental assumes that string's derefmut derefs to a stable address. I spoke briefly with the author once; rental has a StableDeref trait that would just need to gain a StableDerefMut pair that isn't implemented for String. This would break rental's current assumptions - but its only relevant for the ascii mutation in-place methods, because they're the only methods using &mut str. In other words, there's probably no code in existence which actually makes use of rental's broken assumption about String.

I'm really eager to see this change, but I think it needs to be epoch gated.

hanna-kruppe · 2018-01-09T23:45:23Z

I thought library changes can't be epoch-specific, because crates of all epochs share the same standard library (and other libraries, for that matter)?

withoutboats · 2018-01-09T23:46:58Z

I thought library changes can't be epoch-specific, because crates of all epochs share the same standard library (and other libraries, for that matter)?

We can introduce new APIs only in a new epoch; in this case String::literally. So it wouldn't be possible to construct a String that violates the current assumptions on the 2015 epoch (even though the internal implementation would still support that possibility).

CryZe · 2018-01-09T23:48:31Z

Can't such a String get moved into a library that is written against the 2015 epoch though?

withoutboats · 2018-01-09T23:57:32Z

Can't such a String get moved into a library that is written against the 2015 epoch though?

Absolutely.

To be clearer, I don't think we need an epoch to change this (from a legalistic perspective). Rental is currently making an assumption that String upholds a particular invariant that we haven't really guaranteed. However, rental - and possibly other unsafe code - is making that assumption, and we need to decide what to do about it.

Often, we will say our hands are tied by the unsafe code that exists and is relying on this thing, so we now guarantee this thing we didn't originally say we would. I don't think that needs to be the case here, since the guarantee failing can only be observed in very particular edge cases - assuming that calling to_ascii_(upper|lower)case will not change the address of the string from what it was before. Rental can make a breaking change for this little detail and all will be well.

However, I'd like to very clearly and loudly message to users about the fact that these assumptions about String are now invalidated. One way to do that is have it be one of the changes we're making in the epoch transition. So I'd like to use the epoch as a social force in amplifying information about this potential incorrect assumption that as far as we know no one is actually relying on.

bors · 2018-01-20T15:08:22Z

☔ The latest upstream changes (presumably #46952) made this pull request unmergeable. Please resolve the merge conflicts.

shepmaster · 2018-01-26T18:17:13Z

I'm going to unilaterally move this to be nominated for discussion by the team, otherwise it seems like this will just languish forever.

(or you know, what @aturon said in response to me earlier that I forgot about...)

aturon · 2018-01-27T17:17:12Z

@shepmaster I'm going to go a step further and close this PR -- not because we don't want it, but because the next step is an RFC.

I don't have the bandwidth to write such an RFC myself, but I'd really like to see this at least attempted. Is there anyone who would like to take on an RFC with me mentoring/serving as backup?

Manishearth · 2018-01-28T01:18:35Z

I can do an RFC, if Gankro doesn't want to. But if someone else wants to be mentored it's better if they do it (I can help mentor). On Jan 27, 2018 10:47 PM, "Aaron Turon" <[email protected]> wrote: @shepmaster <https://github.com/shepmaster> I'm going to go a step further and *close* this PR -- not because we don't want it, but because the next step is an RFC. I don't have the bandwidth to write such an RFC myself, but I'd really like to see this at least attempted. Is there anyone who would like to take on an RFC with me mentoring/serving as backup? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#46993 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSC4dRuAXJLARGdZrb8f-0I0JImLfks5tO1o1gaJpZM4RMFN5> .

Gankra · 2018-01-28T02:28:21Z

I don't have any interest in working on an RFC. I made this PoC so ergonomics people couldn't complain to me about implementation issues anymore. Yer problem now!

withoutboats · 2018-01-28T04:53:06Z

I'm interested in shepherding (but not writing) an RFC that adds this as a coercion (rather than the String::literally constructor), gated on the 2018 epoch.

SimonSapin · 2018-01-28T08:30:18Z

Why not both a coercion and an explicit contructor? Coercion sounds like it’d work from any &'static str value, is that what you had in mind or only from literals like integers? Why epoch-gated?

withoutboats · 2018-01-28T09:30:28Z

I don't really have an objection to having a constructor. I laid out an argument in favor of epoch gating a few posts up. More generally these seem like good questions for an RFC thread 😉.

Gankra added 3 commits December 24, 2017 22:01

Make Vec/RawVec partially support CoW usage

b57af26

Make String support CoW for literals

50e2cfe

test CoW Strings

eef8810

rust-highfive assigned sfackler Dec 25, 2017

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 25, 2017

ghost reviewed Dec 25, 2017

View reviewed changes

fixup doc comment

bb26fd0

carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jan 22, 2018

shepmaster added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 26, 2018

shepmaster added I-nominated and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 26, 2018

aturon closed this Jan 27, 2018

zbraniecki mentioned this pull request Jan 7, 2019

Refactor of the code, 0.8 compatibility, zero-copy AST/parsing, Rust 2018 goodies projectfluent/fluent-rs#76

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[proof of concept] give String support for CoW wrapping of literals #46993

[proof of concept] give String support for CoW wrapping of literals #46993

Gankra commented Dec 25, 2017

rust-highfive commented Dec 25, 2017

Gankra commented Dec 25, 2017

Gankra commented Dec 25, 2017 •

edited

Loading

ghost Dec 25, 2017

Gankra Dec 25, 2017

ghost Dec 25, 2017

Gankra Dec 25, 2017

BurntSushi Jan 6, 2018

withoutboats Jan 6, 2018

BurntSushi Jan 6, 2018

Manishearth commented Dec 25, 2017 •

edited

Loading

shepmaster commented Jan 6, 2018

aturon commented Jan 6, 2018

aturon commented Jan 9, 2018

Gankra commented Jan 9, 2018

ghost commented Jan 9, 2018

Gankra commented Jan 9, 2018

withoutboats commented Jan 9, 2018 •

edited

Loading

hanna-kruppe commented Jan 9, 2018

withoutboats commented Jan 9, 2018 •

edited

Loading

CryZe commented Jan 9, 2018

withoutboats commented Jan 9, 2018

bors commented Jan 20, 2018

shepmaster commented Jan 26, 2018 •

edited

Loading

aturon commented Jan 27, 2018

Manishearth commented Jan 28, 2018 via email

Gankra commented Jan 28, 2018

withoutboats commented Jan 28, 2018

SimonSapin commented Jan 28, 2018

withoutboats commented Jan 28, 2018

[proof of concept] give String support for CoW wrapping of literals #46993

[proof of concept] give String support for CoW wrapping of literals #46993

Conversation

Gankra commented Dec 25, 2017

rust-highfive commented Dec 25, 2017

Gankra commented Dec 25, 2017

Gankra commented Dec 25, 2017 • edited Loading

ghost Dec 25, 2017

Choose a reason for hiding this comment

Gankra Dec 25, 2017

Choose a reason for hiding this comment

ghost Dec 25, 2017

Choose a reason for hiding this comment

Gankra Dec 25, 2017

Choose a reason for hiding this comment

BurntSushi Jan 6, 2018

Choose a reason for hiding this comment

withoutboats Jan 6, 2018

Choose a reason for hiding this comment

BurntSushi Jan 6, 2018

Choose a reason for hiding this comment

Manishearth commented Dec 25, 2017 • edited Loading

shepmaster commented Jan 6, 2018

aturon commented Jan 6, 2018

aturon commented Jan 9, 2018

Gankra commented Jan 9, 2018

ghost commented Jan 9, 2018

Gankra commented Jan 9, 2018

withoutboats commented Jan 9, 2018 • edited Loading

hanna-kruppe commented Jan 9, 2018

withoutboats commented Jan 9, 2018 • edited Loading

CryZe commented Jan 9, 2018

withoutboats commented Jan 9, 2018

bors commented Jan 20, 2018

shepmaster commented Jan 26, 2018 • edited Loading

aturon commented Jan 27, 2018

Manishearth commented Jan 28, 2018 via email

Gankra commented Jan 28, 2018

withoutboats commented Jan 28, 2018

SimonSapin commented Jan 28, 2018

withoutboats commented Jan 28, 2018

Gankra commented Dec 25, 2017 •

edited

Loading

Manishearth commented Dec 25, 2017 •

edited

Loading

withoutboats commented Jan 9, 2018 •

edited

Loading

withoutboats commented Jan 9, 2018 •

edited

Loading

shepmaster commented Jan 26, 2018 •

edited

Loading