Skip to content

strings/cstring: transcode: prevent Windows sysimage invalidation #58038

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Apr 8, 2025

Should prevent all sysimage invalidation that happens on 64-bit Windows when running:

struct I <: Integer end
function Base.:(<<)(::I, ::Int) end
function Base.:(>>)(::I, ::Int) end

Should prevent some sysimage invalidation that happens on 64-bit
Windows when running:

```julia
struct I <: Integer end
function Base.:(<<)(::I, ::Int) end
```
@nsajko nsajko added system:windows Affects only Windows backport 1.11 Change should be backported to release-1.11 invalidations backport 1.12 Change should be backported to release-1.12 labels Apr 8, 2025
@nsajko nsajko changed the title strings/cstring: transcode: prevent some sysimage invalidation strings/cstring: transcode: prevent Windows sysimage invalidation Apr 8, 2025
@nsajko
Copy link
Contributor Author

nsajko commented Apr 8, 2025

The change prevents these invalidations, tested on Linux with Wine:

{
    "invalidation_count": 16,
    "trees": [
        {
            "method": "<<(::Main.RawInvalidations.I, ::Int64) @ Main.RawInvalidations \\home\\nsajko\\invalidations\\script\\reproducer.jl:19",
            "reason": "inserting",
            "mt_backedges": [
                {
                    "type": "Tuple{typeof(<<), Any, Int64}",
                    "tree": {
                        "method_instance": {
                            "method": "transcode(::Type{UInt16}, src::AbstractVector{UInt8}) @ Base strings\\cstring.jl:181",
                            "method_instance": "MethodInstance for transcode(::Type{UInt16}, ::Base.CodeUnits{UInt8})"
                        },
                        "children": [
                            {
                                "method_instance": {
                                    "method": "cwstring(s::AbstractString) @ Base strings\\cstring.jl:119",
                                    "method_instance": "MethodInstance for Base.cwstring(::AbstractString)"
                                },
                                "children": [
                                    {
                                        "method_instance": {
                                            "method": "memoized_env_lookup(str::AbstractString) @ Base env.jl:8",
                                            "method_instance": "MethodInstance for Base.memoized_env_lookup(::AbstractString)"
                                        },
                                        "children": [
                                            {
                                                "method_instance": {
                                                    "method": "_hasenv(s::AbstractString) @ Base env.jl:25",
                                                    "method_instance": "MethodInstance for Base._hasenv(::AbstractString)"
                                                },
                                                "children": [
                                                    {
                                                        "method_instance": {
                                                            "method": "in(k::AbstractString, ::Base.KeySet{String, Base.EnvDict}) @ Base env.jl:171",
                                                            "method_instance": "MethodInstance for in(::AbstractString, ::Base.KeySet{String, Base.EnvDict})"
                                                        },
                                                        "children": [
                                                            {
                                                                "method_instance": {
                                                                    "method": "haskey(d::AbstractDict, k) @ Base abstractdict.jl:19",
                                                                    "method_instance": "MethodInstance for haskey(::Base.EnvDict, ::Any)"
                                                                },
                                                                "children": [
                                                                    {
                                                                        "method_instance": {
                                                                            "method": "versioninfo(io::IO) @ LinearAlgebra Z:\\home\\nsajko\\tmp\\jl\\jl\\windows\\julia-ec424d47ff\\share\\julia\\stdlib\\v1.13\\LinearAlgebra\\src\\LinearAlgebra.jl:774",
                                                                            "method_instance": "MethodInstance for LinearAlgebra.versioninfo(::IO)"
                                                                        },
                                                                        "children": [
                                                                            {
                                                                                "method_instance": {
                                                                                    "method": "versioninfo() @ LinearAlgebra Z:\\home\\nsajko\\tmp\\jl\\jl\\windows\\julia-ec424d47ff\\share\\julia\\stdlib\\v1.13\\LinearAlgebra\\src\\LinearAlgebra.jl:774",
                                                                                    "method_instance": "MethodInstance for LinearAlgebra.versioninfo()"
                                                                                },
                                                                                "children": [
                                                                                ]
                                                                            },
                                                                            {
                                                                                "method_instance": {
                                                                                    "method": "versioninfo() @ LinearAlgebra Z:\\home\\nsajko\\tmp\\jl\\jl\\windows\\julia-ec424d47ff\\share\\julia\\stdlib\\v1.13\\LinearAlgebra\\src\\LinearAlgebra.jl:774",
                                                                                    "method_instance": "MethodInstance for LinearAlgebra.versioninfo()"
                                                                                },
                                                                                "children": [
                                                                                ]
                                                                            }
                                                                        ]
                                                                    }
                                                                ]
                                                            }
                                                        ]
                                                    }
                                                ]
                                            },
                                            {
                                                "method_instance": {
                                                    "method": "access_env(onError::Function, str::AbstractString) @ Base env.jl:27",
                                                    "method_instance": "MethodInstance for Base.access_env(::Base.var\"#getindex##11#getindex##12\", ::AbstractString)"
                                                },
                                                "children": [
                                                    {
                                                        "method_instance": {
                                                            "method": "getindex(::Base.EnvDict, k::AbstractString) @ Base env.jl:164",
                                                            "method_instance": "MethodInstance for getindex(::Base.EnvDict, ::AbstractString)"
                                                        },
                                                        "children": [
                                                            {
                                                                "method_instance": {
                                                                    "method": "(::LinearAlgebra.var\"#print_var#versioninfo##0\")(io, indent, name) @ LinearAlgebra Z:\\home\\nsajko\\tmp\\jl\\jl\\windows\\julia-ec424d47ff\\share\\julia\\stdlib\\v1.13\\LinearAlgebra\\src\\LinearAlgebra.jl:798",
                                                                    "method_instance": "MethodInstance for (::LinearAlgebra.var\"#print_var#versioninfo##0\")(::IO, ::String, ::AbstractString)"
                                                                },
                                                                "children": [
                                                                    {
                                                                        "method_instance": {
                                                                            "method": "versioninfo(io::IO) @ LinearAlgebra Z:\\home\\nsajko\\tmp\\jl\\jl\\windows\\julia-ec424d47ff\\share\\julia\\stdlib\\v1.13\\LinearAlgebra\\src\\LinearAlgebra.jl:774",
                                                                            "method_instance": "MethodInstance for LinearAlgebra.versioninfo(::IO)"
                                                                        },
                                                                        "children": [
                                                                        ]
                                                                    }
                                                                ]
                                                            }
                                                        ]
                                                    }
                                                ]
                                            },
                                            {
                                                "method_instance": {
                                                    "method": "access_env(onError::Function, str::AbstractString) @ Base env.jl:27",
                                                    "method_instance": "MethodInstance for Base.access_env(::Returns{Symbol}, ::AbstractString)"
                                                },
                                                "children": [
                                                    {
                                                        "method_instance": {
                                                            "method": "get(::Base.EnvDict, k::AbstractString, def) @ Base env.jl:165",
                                                            "method_instance": "MethodInstance for get(::Base.EnvDict, ::AbstractString, ::Symbol)"
                                                        },
                                                        "children": [
                                                            {
                                                                "method_instance": {
                                                                    "method": "in(k, v::Base.KeySet) @ Base abstractdict.jl:73",
                                                                    "method_instance": "MethodInstance for in(::Any, ::Base.KeySet{String, Base.EnvDict})"
                                                                },
                                                                "children": [
                                                                    {
                                                                        "method_instance": {
                                                                            "method": "haskey(d::AbstractDict, k) @ Base abstractdict.jl:19",
                                                                            "method_instance": "MethodInstance for haskey(::Base.EnvDict, ::Any)"
                                                                        },
                                                                        "children": [
                                                                        ]
                                                                    }
                                                                ]
                                                            }
                                                        ]
                                                    }
                                                ]
                                            }
                                        ]
                                    }
                                ]
                            }
                        ]
                    }
                }
            ],
            "backedges": [
                {
                    "method_instance": {
                        "method": "<<(x::Integer, c::Int64) @ Base operators.jl:720",
                        "method_instance": "MethodInstance for <<(::Integer, ::Int64)"
                    },
                    "children": [
                        {
                            "method_instance": {
                                "method": "transcode(::Type{UInt16}, src::AbstractVector{UInt8}) @ Base strings\\cstring.jl:181",
                                "method_instance": "MethodInstance for transcode(::Type{UInt16}, ::Base.CodeUnits{UInt8})"
                            },
                            "children": [
                            ]
                        }
                    ]
                }
            ],
            "mt_cache": [
            ],
            "mt_disable": [
            ]
        },
        {
            "method": ">>(::Main.RawInvalidations.I, ::Int64) @ Main.RawInvalidations \\home\\nsajko\\invalidations\\script\\reproducer.jl:19",
            "reason": "inserting",
            "mt_backedges": [
                {
                    "type": "Tuple{typeof(>>), Any, Int64}",
                    "tree": {
                        "method_instance": {
                            "method": "transcode(::Type{UInt16}, src::AbstractVector{UInt8}) @ Base strings\\cstring.jl:181",
                            "method_instance": "MethodInstance for transcode(::Type{UInt16}, ::Base.CodeUnits{UInt8})"
                        },
                        "children": [
                        ]
                    }
                }
            ],
            "backedges": [
                {
                    "method_instance": {
                        "method": ">>(x::Integer, c::Int64) @ Base operators.jl:761",
                        "method_instance": "MethodInstance for >>(::Integer, ::Int64)"
                    },
                    "children": [
                        {
                            "method_instance": {
                                "method": "transcode(::Type{UInt16}, src::AbstractVector{UInt8}) @ Base strings\\cstring.jl:181",
                                "method_instance": "MethodInstance for transcode(::Type{UInt16}, ::Base.CodeUnits{UInt8})"
                            },
                            "children": [
                            ]
                        }
                    ]
                }
            ],
            "mt_cache": [
            ],
            "mt_disable": [
            ]
        }
    ]
}

@KristofferC
Copy link
Member

KristofferC commented Apr 8, 2025

Is it possible to figure out why getindex on AbstractVector{T} cannot be inferred to T? It also seems possible to fix some of the type instabilitiea "higher up" the call chain. For example:

julia> @code_warntype LinearAlgebra.versioninfo(stdout)
MethodInstance for LinearAlgebra.versioninfo(::Base.TTY)
  from versioninfo(io::IO) @ LinearAlgebra ~/.julia/juliaup/julia-1.10.9+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/LinearAlgebra/src/LinearAlgebra.jl:614
...
  @_13::Union{Nothing, Tuple{Any, Int64}}
  name::Any
  nm::Any

We could fix there.

@nsajko
Copy link
Contributor Author

nsajko commented Apr 8, 2025

Is it possible to figure out why getindex on AbstractVector{T} cannot be inferred to T?

I looked into that already, but couldn't find anything to improve in that direction. It could be fixed by adding a typeassert to the CodeUnits getindex method, I guess, but that could cause other issues.

julia> methods(getindex, Tuple{Base.CodeUnits{UInt8}, Int})
# 1 method for generic function "getindex" from Base:
 [1] getindex(s::Base.CodeUnits, i::Int64)
     @ strings/basic.jl:803

julia> code_typed(getindex, Tuple{Base.CodeUnits{UInt8}, Int}; debuginfo=:source)
1-element Vector{Any}:
 CodeInfo(
    @ strings/basic.jl:803 within `getindex`
   ┌ @ Base_compiler.jl:55 within `getproperty`
1 ─│ %1 =   builtin Base.getfield(s, :s)::AbstractString
│  └
│   %2 =   dynamic Base.codeunit(%1, i)::Any
└──      return %2
) => Any

It also seems possible to fix some of the type instabilitiea "higher up" the call chain.

Done already, however I suppose it's better to be thorough and fix on multiple places on the call chain:

@KristofferC
Copy link
Member

KristofferC commented Apr 8, 2025

it's better to be thorough and fix on multiple places on the call chain:

Maybe, I mean the only reason you find these in particular is because they end up getting called by type unstable code that ends up in the sysimage. Otherwise, you would annotate every getindex on AbstractVector{T} in the whole code base?

@nsajko
Copy link
Contributor Author

nsajko commented Apr 8, 2025

the only reason you find these in particular is because they end up getting called by type unstable code that ends up in the sysimage

My reasoning is that in the future some other piece of type unstable code could be added, eventually calling transcode, so it seems good to make transcode as resistant to invalidation as reasonably possible. This is also presumably useful to prevent invalidation of precompile files of user packages.

Otherwise, you would annotate every getindex on AbstractVector{T} in the whole code base?

Not quite:

  • The reason we can typeassert here without hurting performance is that the element type,T, is known to be a specific concrete type at method definition time, that is, T is specified literally in the method signature, it's not a method static parameter. Otherwise we could still typeassert, improving inference in some cases, but at the cost of worse performance in cases where the method static parameter, T, is not known at compile time.
  • The reason the typeassert is helpful here is that the type of the collection is not restricted to a specific concrete type, for example if it was restricted to Vector{UInt8} there would be no need to typeassert.

@KristofferC KristofferC mentioned this pull request Apr 9, 2025
43 tasks
@nsajko
Copy link
Contributor Author

nsajko commented Apr 13, 2025

What exactly is the issue here?

If it's code duplication, I could introduce an additional local function like getsrc(i) = src[i]::UInt8, then the typeasserts wouldn't need to be duplicated, I guess.

If this seems useless after the LinearAlgebra.jl PR is merged, as far as I understand it should still be useful for released versions of Julia, v1.10 and v1.11, because those don't have upgradeable LinearAlgebra, I think.

@KristofferC
Copy link
Member

What exactly is the issue here?

It doesn't to me feel like his function does anything wrong. The only reason you are changing these is because of some badly written code in stdlibs, or?

it should still be useful for released versions of Julia,

You can just make the same fix here in stdlib/LinearAlgebra?

@nsajko
Copy link
Contributor Author

nsajko commented Apr 13, 2025

You can just make the same fix here in stdlib/LinearAlgebra?

Not sure what are you suggesting? AFAIK LinearAlgebra is baked into the sysimage, so it can't be upgraded, at least on released versions of Julia? EDIT: figured it out, see comment below.

@nsajko
Copy link
Contributor Author

nsajko commented Apr 13, 2025

Oh, you mean to make a PR to the v1.11 backports branch, modifying the LinearAlgebra code there directly?

@nsajko
Copy link
Contributor Author

nsajko commented Apr 13, 2025

It doesn't to me feel like his function does anything wrong.

Perhaps not wrong, however I guess it can do better, to be more resistant to invalidation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 invalidations system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants