Skip to content

Commit

Permalink
Memoize cwstring when used for env lookup / modification on Windows (
Browse files Browse the repository at this point in the history
…JuliaLang#51371)

~This is just me proposing a suggestion from @KristofferC in
https://discourse.julialang.org/t/debug-has-massive-performance-impact/103974/22,
it's all his code / idea, not mine.~

This PR makes it so that on windows, we pre-allocate an `IdDict` and
every time someone looks up environment variables (motivating example
here is `@debug` statements), we store the result of
`cwstring(::String)` in that `IdDict` so that it doesn't need to be
re-computed for future uses.

The idea behind this is that people have observed that [using `@debug`
is significantly more costly on Windows than other
platforms](https://discourse.julialang.org/t/debug-has-massive-performance-impact/103974),
even though we have documented in that manual that it should be a really
cheap operation. @KristofferC suggests this is due to the fact that
[checking environment variables in Windows is more
costly](https://discourse.julialang.org/t/debug-has-massive-performance-impact/103974/18).

~The idea here is that we preallocate a `Cwstring` on Windows that just
holds the text `"JULIA_DEBUG"`, so that if `access_env(f,
"JULIA_DEBUG")` gets called, we don't need to create a new `Cwstring`
and then throw it away each time.~

---------

Co-authored-by: Ian Butterworth <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
  • Loading branch information
3 people authored and mkitti committed Dec 9, 2023
1 parent 0d66879 commit 1e167ce
Showing 1 changed file with 21 additions and 4 deletions.
25 changes: 21 additions & 4 deletions base/env.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,29 @@
if Sys.iswindows()
const ERROR_ENVVAR_NOT_FOUND = UInt32(203)

const env_dict = IdDict{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)
end
end
var
end

_getenvlen(var::Vector{UInt16}) = ccall(:GetEnvironmentVariableW,stdcall,UInt32,(Ptr{UInt16},Ptr{UInt16},UInt32),var,C_NULL,0)
_hasenv(s::Vector{UInt16}) = _getenvlen(s) != 0 || Libc.GetLastError() != ERROR_ENVVAR_NOT_FOUND
_hasenv(s::AbstractString) = _hasenv(cwstring(s))
_hasenv(s::AbstractString) = _hasenv(memoized_env_lookup(s))

function access_env(onError::Function, str::AbstractString)
var = cwstring(str)
var = memoized_env_lookup(str)
len = _getenvlen(var)
if len == 0
return Libc.GetLastError() != ERROR_ENVVAR_NOT_FOUND ? "" : onError(str)
Expand All @@ -21,7 +38,7 @@ if Sys.iswindows()
end

function _setenv(svar::AbstractString, sval::AbstractString, overwrite::Bool=true)
var = cwstring(svar)
var = memoized_env_lookup(svar)
val = cwstring(sval)
if overwrite || !_hasenv(var)
ret = ccall(:SetEnvironmentVariableW,stdcall,Int32,(Ptr{UInt16},Ptr{UInt16}),var,val)
Expand All @@ -30,7 +47,7 @@ if Sys.iswindows()
end

function _unsetenv(svar::AbstractString)
var = cwstring(svar)
var = memoized_env_lookup(svar)
ret = ccall(:SetEnvironmentVariableW,stdcall,Int32,(Ptr{UInt16},Ptr{UInt16}),var,C_NULL)
windowserror(:setenv, ret == 0 && Libc.GetLastError() != ERROR_ENVVAR_NOT_FOUND)
end
Expand Down

0 comments on commit 1e167ce

Please sign in to comment.