Skip to content

Commit 9dcedaa

Browse files
MasonProtterIanButterworthvtjnash
authored
Memoize cwstring when used for env lookup / modification on Windows (#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 <i.r.butterworth@gmail.com> Co-authored-by: Jameson Nash <vtjnash@gmail.com>
1 parent 4a18886 commit 9dcedaa

File tree

1 file changed

+21
-4
lines changed

1 file changed

+21
-4
lines changed

base/env.jl

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,29 @@
33
if Sys.iswindows()
44
const ERROR_ENVVAR_NOT_FOUND = UInt32(203)
55

6+
const env_dict = IdDict{String, Vector{Cwchar_t}}()
7+
const env_lock = ReentrantLock()
8+
9+
function memoized_env_lookup(str::AbstractString)
10+
# Windows environment variables have a different format from Linux / MacOS, and previously
11+
# incurred allocations because we had to convert a String to a Vector{Cwchar_t} each time
12+
# an environment variable was looked up. This function memoizes that lookup process, storing
13+
# the String => Vector{Cwchar_t} pairs in env_dict
14+
var = get(env_dict, str, nothing)
15+
if isnothing(var)
16+
var = @lock env_lock begin
17+
env_dict[str] = cwstring(str)
18+
end
19+
end
20+
var
21+
end
22+
623
_getenvlen(var::Vector{UInt16}) = ccall(:GetEnvironmentVariableW,stdcall,UInt32,(Ptr{UInt16},Ptr{UInt16},UInt32),var,C_NULL,0)
724
_hasenv(s::Vector{UInt16}) = _getenvlen(s) != 0 || Libc.GetLastError() != ERROR_ENVVAR_NOT_FOUND
8-
_hasenv(s::AbstractString) = _hasenv(cwstring(s))
25+
_hasenv(s::AbstractString) = _hasenv(memoized_env_lookup(s))
926

1027
function access_env(onError::Function, str::AbstractString)
11-
var = cwstring(str)
28+
var = memoized_env_lookup(str)
1229
len = _getenvlen(var)
1330
if len == 0
1431
return Libc.GetLastError() != ERROR_ENVVAR_NOT_FOUND ? "" : onError(str)
@@ -21,7 +38,7 @@ if Sys.iswindows()
2138
end
2239

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

3249
function _unsetenv(svar::AbstractString)
33-
var = cwstring(svar)
50+
var = memoized_env_lookup(svar)
3451
ret = ccall(:SetEnvironmentVariableW,stdcall,Int32,(Ptr{UInt16},Ptr{UInt16}),var,C_NULL)
3552
windowserror(:setenv, ret == 0 && Libc.GetLastError() != ERROR_ENVVAR_NOT_FOUND)
3653
end

0 commit comments

Comments
 (0)