use a Dict instead of an IdDict for caching of the cwstring for Windows env variables

[KristofferC]

Should fix #52711.

My analysis of the invalidation is as follows:

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

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:

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

which has an edge to:

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

But then StaticArrays comes along and adds a method

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.

cc @jaakkor2, could you try this PR out?

cc @MasonProtter

[vtjnash]

Unlike IdDict, the Dict code is not thread-safe for concurrent lookups, so will need to move those lookup access behind a lock

[MasonProtter]

moving the accesses behind a lock could slow things down a bit right? Choosing dict types based just on opaque non-local invalidation effects seems like a kinda unhappy situation. I wonder if there's a way to keep the IdDict but avoid the invalidation

[KristofferC]

the Dict code is not thread-safe for concurrent lookups

For my learning, what causes this unsafeness?

Choosing dict types based just on opaque non-local invalidation effects seems like a kinda unhappy situation.

I agree, but the current situation is also unhappy.

I wonder if there's a way to keep the IdDict but avoid the invalidation

The memoized_env_lookup could be moved behind an inference barrier but that also has some performance implications. However, the speed of the caching makes lookups faster than on 1.9 anyway so perhaps that is still ok?

[KristofferC]

#52760 for example.

[IanButterworth]

What about going back to the non-Dict original form of #51371 and just store the cwstring for JULIA_DEBUG? That seems like a safe fix for a 1.10.1 ?

[vtjnash]

The lock performance should be fine. The function you call here (the environment variables lookup) also already always needed to take a lock internally on all platforms, so the added overhead should be slight.

[jaakkor2]

Lock-version does not compile, output var is not in the scope.
https://github.com/JuliaLang/julia/blob/kc/dict_fdsfds/base/env.jl#L20

I defined var = nothing before the @lock, and then it does fix the invalidation issue.

Fixed