Always resize input to zero length in String(::Vector{UInt8}) #26093

nalimilan · 2018-02-17T14:01:59Z

This makes the behavior more predictable than only resizing Vector{UInt8} inputs when they have been allocated via StringVector, as the caller may have obtained them from other code without knowing how they were created. This ensures code will not rely on the fact that a copy is made in many common cases. The behavior is also simpler to document.

This is an alternative to @stevengj's #25846 (I included a few changes from there), as discussed on the #strings Slack channel with @JeffBezanson and @StefanKarpinski.

The second commit fixes a bug I discovered when preparing the first one. It's not actually needed for the first one so better avoid squashing it.

stevengj · 2018-02-17T14:17:17Z

The docstring should not give the misleading implication that String(vector) is zero-copy (as it used to be) except for special cases, however.

nalimilan · 2018-02-17T14:42:26Z

Indeed. I've rephrased the docstring, let me know what you think.

stevengj · 2018-02-17T16:29:52Z

base/strings/string.jl

+
+When possible, the data in `v` will be used directly rather than making
+a copy. This is the case for `Vector{UInt8}` arrays returned by [`take!`](@ref)
+on a writable [`IOBuffer`](@ref) or by [`read(io, nb)`](@ref). Else, a copy is made.


Else -> Otherwise

stevengj · 2018-02-17T16:30:28Z

base/strings/string.jl

+When possible, the data in `v` will be used directly rather than making
+a copy. This is the case for `Vector{UInt8}` arrays returned by [`take!`](@ref)
+on a writable [`IOBuffer`](@ref) or by [`read(io, nb)`](@ref). Else, a copy is made.
+In all cases, one should assume that the function takes "ownership" of the array


"In all cases" seems overly broad here. Why not just say that this is the case for a Vector and leave it at that?

JeffBezanson · 2018-02-17T17:50:08Z

test/arrayops.jl

+@testset "check that resize! is a no-op when size does not change" for T in (Int, UInt8, Int8)
+    x = Vector{T}(uninitialized, 1)
+    resize!(x, 0)
+    unsafe_store!(pointer(x), 1, 1)


I don't think this test is valid --- it does not need to be possible to store an element to an empty array. In the future the array's data pointer could be NULL, or tooling could flag this as a memory error, etc.

Yeah, it's pretty hacky. I guess we can leave this one out and just rely on the String-based test below. I'm not sure whether we can find other cases where this bug would have user-visible consequences we could test.

I've removed the test and squashed the commits.

StefanKarpinski · 2018-02-17T17:57:09Z

The text about resizing to zero seems a bit misleading to me. If all it did was keep using the same buffer but truncate it to zero length, then you could still mutate the String's memory by pushing elements to the truncated array. The real point is that the memory buffer of the array is "stolen" by the String object and the array object is given a a new memory buffer, which happens to be empty since that's the most efficient thing to do. In the case of an immutable byte vector, the vector is not truncated afterwards. The real point is that if the vector is mutable, its buffer is taken and the vector is left with a new, empty buffer, preventing modification of the String through the buffer.

Incidentally, this is one of the reasons that having String(v, copy=true) didn't quite sit right with me since in the immutable case, you're not making a copy. What you're really asking for is "please don't empty the argument vector" – whatever it needs to do to accomplish that. Thus, String(v, empty=false) which would make a copy in the case of a mutable vector or just do the usual thing (jut share the memory) in the case of an immutable vector. Of course, if one writes String(copy(v)) and copy(v) is the identity for immutable v then it "just works" and the only issue is avoiding a second copy in the case where copy(v) makes a copy of a mutable vector and String then steals the buffer of that copy. That seems like a better API to me since the optimization is desirable in general.

nalimilan · 2018-02-17T18:04:24Z

Can you suggest a wording? It's hard to describe the behavior in a simple way while still giving enough details for people who are interested in the performance and copying characteristics.

EDIT: feel free to commit changes directly on GitHub if you like.

stevengj · 2018-02-17T18:50:23Z

base/strings/string.jl

-
-If you need to subsequently modify `v`, use `String(copy(v))` instead.
+Create a new `String` from a byte vector `v` containing UTF-8 encoded
+characters. If `v` is a mutable vector, it will be truncated and any future


truncated to zero length

Also, this statement seems false. It only truncates Vector, not arbitrary mutable subtypes of AbstractVector.

The behavior for other types should be implemented to match, but yes, at the moment this may not be 100% so so I guess this docstring is aspirational.

JeffBezanson · 2018-02-17T18:54:13Z

I agree String(copy(v)) is a good API, but we're not going to be able to optimize it any time soon. String(copy(v)) => String(v) is of course not valid, but nor does String(copy(v)) => String_in_place(copy(v)) work, since copy(v) might not have the right memory layout to be able to support that. We would basically need a special case to replace String(copy(v)) with String(copyto!(StringVector(length(v)), v)).

stevengj · 2018-02-17T21:22:50Z

Maybe we should have String(v) always make a copy, and have a new copy-free truncating constructor String!(v) that throws an exception for non-StringVector arrays.

String(v) could print a deprecation message in 0.7 if is called on a StringVector.

JeffBezanson · 2018-02-17T23:11:49Z

I don't like that so much, since (1) there's no type called String!, (2) the vast majority (pretty much 100%) of uses of String(v) do not use v afterwards, and (3) this is not necessarily a mutating operation. In practice, we would have to tell people "never use String, always use String! instead", because that's the function that does what you want.

One place where something like String! or string! would be valuable though is to replace String(take!(iobuffer)). Perhaps take!(iobuffer, String). That would allow us to avoid an extra allocation since we know the caller doesn't want the vector, just the string underneath, so the buffer can reuse the vector object (just not its storage).

JeffBezanson · 2018-02-19T19:59:12Z

NEWS.md

+  * `String(array)` now accepts an arbitrary `AbstractVector{UInt8}` and "steals" the
+    memory buffer of mutable arrays, leaving the byte vector with an empty buffer which
+    is guaranteed not to be shared with the `String` object; if the byte vector is
+    immutable, it simply shares memory with the string and is not truncated ([#26093]).


This promises too much --- we don't have the ability to share arbitrary immutable vector memory with strings. It also doesn't steal the memory of arbitrary mutable arrays, only Vector{UInt8}.

Can you suggest a wording?

I've proposed something, let me know what you think.

JeffBezanson · 2018-02-19T19:59:57Z

test/arrayops.jl

@@ -2342,4 +2342,4 @@ Base.view(::T25958, args...) = args
    @test t[end,1,end]   == @view(t[end,1,end])   == @views t[end,1,end]
    @test t[end,end,1]   == @view(t[end,end,1])   == @views t[end,end,1]
    @test t[end,end,end] == @view(t[end,end,end]) == @views t[end,end,end]
-end
+end


This should be reverted.

Woops, those damned ending newlines... Fixed.

stevengj · 2018-02-19T21:41:15Z

The basic problem here is that sometimes String makes a copy and sometimes not, and it is hard to determine which occurs when you look at a piece of code. If you had a String! that never makes a copy, and throws an exception if not, while String always makes a copy, it would be a lot easier to understand.

Another problem is that if you want to make a copy, then String(copy(v)) actually makes two copies of the data, since copy doesn't use a StringVector. Whereas if String(v) always made a copy, we could ensure that it only made a single copy.

JeffBezanson · 2018-02-19T23:03:54Z

In theory that seems nice, but in practice 99% of String(v) calls want the memory-stealing behavior.

stevengj · 2018-02-20T13:16:56Z

99% of String(v) calls may want the memory-stealing behavior, but probably less than 99% of them get it (because they allocated v the wrong way). The current inconsistency in the behavior of String (even for v::Vector{UInt8}) is sure to lead to unexpected allocation behavior.

(With String(v) changed to truncate v, that is another reason to rename it String!.)

nalimilan · 2018-02-20T13:42:44Z

(With String(v) changed to truncate v, that is another reason to rename it String!.)

See also discussion about Hermitian! at #17367.

StefanKarpinski · 2018-03-14T19:34:12Z

I suspect that a very large portion of cases where people write String(v) are already getting the no-copy behavior that they want. Moreover, with this PR, if we find many cases where String(v) is forced to copy, we can fix that problem without breaking anything.

JeffBezanson · 2018-03-14T19:35:09Z

At this point I think this should just be rebased and merged.

This makes the behavior more predictable than only resizing Vector{UInt8} inputs when they have been allocated via StringVector, as the caller may have obtained them from other code without knowing how they were created. This ensures code will not rely on the fact that a copy is made in many common cases. The behavior is also simpler to document.

Else we set the last element to zero for one-byte element types like UInt8, which means that resizing a vector allocated with StringVector corrupts it. Also add redundant checks in Julia code to avoid a function call.

quinnj · 2018-03-22T04:04:03Z

Well, this led to some fun behavior because MbedTLS was calling string on an input Vector{UInt8} (for no reason really); poor web apps that were just trying to hmac their request bodies.....

Hopefully there's not too many cases where this change causes such subtle changes in behavior.

Currently because `String` steals the buffer allocated, we create a copy before conversion. This may be suboptimal. See JuliaLang/julia#26093

kindlychung · 2019-07-07T17:48:34Z

Imagine you are just printing some debug info with String(v)...
Assuming this kind of things only happen 1% of time, it can lead to a lot of frustration.

StefanKarpinski · 2019-07-08T14:56:32Z

I think the way to get change here is not to comment on this long-closed issue but to open a new issue to discuss the possibility of migrating away from the current behavior.

nalimilan added domain:docs This change adds or pertains to documentation domain:strings "Strings!" labels Feb 17, 2018

nalimilan force-pushed the nl/String branch from 851a892 to de014c9 Compare February 17, 2018 14:40

stevengj reviewed Feb 17, 2018

View reviewed changes

JeffBezanson reviewed Feb 17, 2018

View reviewed changes

StefanKarpinski force-pushed the nl/String branch from 88b1248 to 6c58d10 Compare February 17, 2018 18:32

stevengj reviewed Feb 17, 2018

View reviewed changes

StefanKarpinski force-pushed the nl/String branch from 6c58d10 to 369820d Compare February 17, 2018 19:16

nalimilan force-pushed the nl/String branch 2 times, most recently from e491edc to cabe8ad Compare February 18, 2018 11:47

JeffBezanson reviewed Feb 19, 2018

View reviewed changes

nalimilan force-pushed the nl/String branch 2 times, most recently from 9d64ae9 to 2154ea6 Compare February 20, 2018 10:03

JeffBezanson added the status:triage This should be discussed on a triage call label Mar 10, 2018

JeffBezanson self-assigned this Mar 13, 2018

JeffBezanson added this to the 1.0 milestone Mar 14, 2018

StefanKarpinski removed the status:triage This should be discussed on a triage call label Mar 14, 2018

nalimilan added 2 commits March 14, 2018 21:32

Make resize! a no-op when size does not change

63a72d1

Else we set the last element to zero for one-byte element types like UInt8, which means that resizing a vector allocated with StringVector corrupts it. Also add redundant checks in Julia code to avoid a function call.

nalimilan force-pushed the nl/String branch from 2154ea6 to 63a72d1 Compare March 14, 2018 20:32

JeffBezanson merged commit 4be51b7 into master Mar 14, 2018

JeffBezanson deleted the nl/String branch March 14, 2018 22:57

rejuvyesh added a commit to rejuvyesh/MuJoCo.jl that referenced this pull request Aug 10, 2018

All tests pass

65efb46

Currently because `String` steals the buffer allocated, we create a copy before conversion. This may be suboptimal. See JuliaLang/julia#26093

MasonProtter mentioned this pull request Jul 8, 2019

String constructor truncates data. #32528

Open

KristofferC mentioned this pull request May 13, 2024

New copystring function #54424

Open

jakobnissen mentioned this pull request May 14, 2024

Add takestring! and make String(::Memory) not truncate #54372

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always resize input to zero length in String(::Vector{UInt8}) #26093

Always resize input to zero length in String(::Vector{UInt8}) #26093

nalimilan commented Feb 17, 2018

stevengj commented Feb 17, 2018 •

edited

Loading

nalimilan commented Feb 17, 2018

stevengj Feb 17, 2018

stevengj Feb 17, 2018

JeffBezanson Feb 17, 2018

nalimilan Feb 17, 2018

nalimilan Feb 18, 2018

StefanKarpinski commented Feb 17, 2018 •

edited

Loading

nalimilan commented Feb 17, 2018 •

edited

Loading

stevengj Feb 17, 2018

stevengj Feb 17, 2018

StefanKarpinski Feb 17, 2018

JeffBezanson commented Feb 17, 2018

stevengj commented Feb 17, 2018

JeffBezanson commented Feb 17, 2018

JeffBezanson Feb 19, 2018

nalimilan Feb 19, 2018

nalimilan Feb 20, 2018

JeffBezanson Feb 19, 2018

nalimilan Feb 20, 2018

stevengj commented Feb 19, 2018 •

edited

Loading

JeffBezanson commented Feb 19, 2018

stevengj commented Feb 20, 2018

nalimilan commented Feb 20, 2018

StefanKarpinski commented Mar 14, 2018

JeffBezanson commented Mar 14, 2018

quinnj commented Mar 22, 2018

kindlychung commented Jul 7, 2019

StefanKarpinski commented Jul 8, 2019

Always resize input to zero length in String(::Vector{UInt8}) #26093

Always resize input to zero length in String(::Vector{UInt8}) #26093

Conversation

nalimilan commented Feb 17, 2018

stevengj commented Feb 17, 2018 • edited Loading

nalimilan commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Feb 17, 2018 • edited Loading

nalimilan commented Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Feb 17, 2018

stevengj commented Feb 17, 2018

JeffBezanson commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Feb 19, 2018 • edited Loading

JeffBezanson commented Feb 19, 2018

stevengj commented Feb 20, 2018

nalimilan commented Feb 20, 2018

StefanKarpinski commented Mar 14, 2018

JeffBezanson commented Mar 14, 2018

quinnj commented Mar 22, 2018

kindlychung commented Jul 7, 2019

StefanKarpinski commented Jul 8, 2019

stevengj commented Feb 17, 2018 •

edited

Loading

StefanKarpinski commented Feb 17, 2018 •

edited

Loading

nalimilan commented Feb 17, 2018 •

edited

Loading

stevengj commented Feb 19, 2018 •

edited

Loading