Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[backports-release-1.11] Make String(::Memory) copy #56519

Conversation

nhz2
Copy link
Contributor

@nhz2 nhz2 commented Nov 10, 2024

Manual backport of #54457 to fix #54369 in 1.11

@giordano giordano force-pushed the nz/1.11-string-memory-backport branch from 621011d to 114953c Compare November 11, 2024 00:13
@giordano
Copy link
Contributor

https://buildkite.com/julialang/julia-release-1-dot-11/builds/325#01931891-b8f7-414d-8bce-0c0908b26d74/875-1652

The global RNG seed was 0xc55e57e524be7f332615c29ea9da899b.
Error in testset operators:
Test Failed at /cache/build/tester-amdci5-9/julialang/julia-release-1-dot-11/julia-114953cb48/share/julia/test/operators.jl:158
  Expression: +()
    Expected: MethodError
      Thrown: StackOverflowError
      StackOverflowError:
      Stacktrace:
       [1] +() (repeats 79984 times)
         @ Main.Test32Main_LinearAlgebra_matmul.TestMatmul /cache/build/tester-amdci5-9/julialang/julia-release-1-dot-11/julia-114953cb48/share/julia/stdlib/v1.11/LinearAlgebra/test/matmul.jl:1119

I seem to have seen this error before, maybe we need to backport some other patches. @jishnub perhaps you remember what was it? I thought it was #56184, which fixed #56136, but that was in a different testset.

@jishnub
Copy link
Contributor

jishnub commented Nov 11, 2024

This was caused by #56089, and was fixed subsequently by #56184.

@giordano
Copy link
Contributor

and was fixed subsequently by #56184.

But that's changing stdlib/LinearAlgebra/test/matmul.jl, the failure is in test/operators.jl

@jishnub
Copy link
Contributor

jishnub commented Nov 11, 2024

IIUC, the failure happens if the test/operators.jl is run in the same process after the LinearAlgebra matmul tests that introduce the stack-overflowing method. The offending method

Base.:+(t::Thing...) = +(getfield.(t, :data)...)
is committing type-piracy by defining +() = +(), so this would impact any subsequent invocation of +() that is run in the same process. This showed up in the test/errorshow.jl failure after the PR was merged, but apparently test/operators.jl also checks the same method.

@giordano
Copy link
Contributor

I see, tests weren't isolated enough. I pushed the backport to #56228, this is failing also there anyway

@KristofferC
Copy link
Member

Feels a bit too intrusive to backport to a patch release, or?

@nhz2
Copy link
Contributor Author

nhz2 commented Nov 11, 2024

This fixes code like String(str isa Vector{UInt8} ? copy(str) : str) in JuliaData/Parsers.jl#193

Since this is how the docs of String recommend copying an AbstractVector{UInt8} into a String there may be other existing packages that are fixed by this. This shouldn't break anything unless someone is using the internal Base.StringMemory to create a huge string, and there isn't enough memory left to make a copy.
Using String on a vector created with Base.StringVector(len) still avoids copying like before, since this only affects String(::Memory).

@KristofferC KristofferC deleted the branch JuliaLang:backports-release-1.11 November 21, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants