Skip to content

Commit

Permalink
use a Dict instead of an IdDict for caching of the cwstring for Win…
Browse files Browse the repository at this point in the history
…dows env variables (JuliaLang#52758)

Should fix JuliaLang#52711.

My analysis of the invalidation is as follows:

We added code to cache the conversion to `cwstring` in env handling on
Windows (JuliaLang#51371):

```julia
const env_dict = IdDict{String, Vector{UInt16}}()

function memoized_env_lookup(str::AbstractString)
    ...
    env_dict[str] = cwstring(str)
    ...
end

function access_env(onError::Function, str::AbstractString)
    var = memoized_env_lookup(str)
    ...
end
```

Since `IdDict` has `@nospecialize` on `setindex!` we compile this
method:

```julia
setindex!(::IdDict{String, Vector{UInt16}}, ::Any, ::Any)
```

which has an edge to:

```julia
convert(Type{Vector{Int64}}, Any})
```

But then StaticArrays comes along and adds a method

```julia
convert(::Type{Array{T, N}}, ::StaticArray)
```

which invalidates the `setindex!` (due to the edge to `convert`) which
invalidates the whole env handling on Windows which causes 4k other
methods downstream to be invalidated, in particular, the artifact string
macro which causes a significant delay in the next jll package you load
after loading StaticArrays.

There should be no performance penalty to this since strings already
does a hash for their `objectid`.
  • Loading branch information
KristofferC committed Jan 8, 2024
1 parent 1701009 commit b7c24ed
Showing 1 changed file with 7 additions and 6 deletions.
13 changes: 7 additions & 6 deletions base/env.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,22 @@
if Sys.iswindows()
const ERROR_ENVVAR_NOT_FOUND = UInt32(203)

const env_dict = IdDict{String, Vector{Cwchar_t}}()
const env_dict = Dict{String, Vector{Cwchar_t}}()
const env_lock = ReentrantLock()

function memoized_env_lookup(str::AbstractString)
# Windows environment variables have a different format from Linux / MacOS, and previously
# incurred allocations because we had to convert a String to a Vector{Cwchar_t} each time
# an environment variable was looked up. This function memoizes that lookup process, storing
# the String => Vector{Cwchar_t} pairs in env_dict
var = get(env_dict, str, nothing)
if isnothing(var)
var = @lock env_lock begin
env_dict[str] = cwstring(str)
@lock env_lock begin
var = get(env_dict, str, nothing)
if isnothing(var)
var = cwstring(str)
env_dict[str] = var
end
return var
end
var
end

_getenvlen(var::Vector{UInt16}) = ccall(:GetEnvironmentVariableW,stdcall,UInt32,(Ptr{UInt16},Ptr{UInt16},UInt32),var,C_NULL,0)
Expand Down

0 comments on commit b7c24ed

Please sign in to comment.