search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

Why do we have to have a lock on Environment.GetEnvironmentVariables? #106810

Closed vasicvuk closed 2 months ago

vasicvuk commented 2 months ago

Why do we have to have a lock on Environment.GetEnvironmentVariables?

https://github.com/dotnet/runtime/blob/6177a9f920861900681cfda2b6cc66ac3557e93b/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Unix.cs#L64

We encountered the hot path in the function called Environment.GetEnvironmentVariables and find out that there is this lock inside of a function. Can this be optimized somehow? Maybe the whole Hashtable can be cached or we can return the value if exists before the lock?

dotnet-policy-service[bot] commented 2 months ago

Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.

vcsjones commented 2 months ago

Maybe the whole Hashtable can be cached

The values are cached - that's what s_environment is. The method GetEnvironmentVariables is creating a copy to return because returning s_environment would expose mutability. Every call to the method needs its own copy.

The lock is needed because SetEnvironmentVariable can mutate s_environment, and that collection type is not safe to mutate from different threads. So the lock is taken while the copy is made to return.

If you need better performance out of it, I don't see why you couldn't cache it yourself. This may be more appropriate as your own code understands the thread safety needs of your own code, whereas the runtime itself cannot make any assumptions or expectations about callers from multiple threads - especially for static APIs.

julealgon commented 2 months ago

We encountered the hot path in the function called Environment.GetEnvironmentVariables

Could you elaborate why you need to call this method so much? What is the design of your application? Can't you use the higher-level IConfiguration-based mechanism (and IOptions) instead?

vasicvuk commented 2 months ago

Currently, we are using the Workflow Core which uses this method directly from the persistence layer causing CPU to hot path there :) I was just debugging when I found this. I don't have direct access to code so i cannot optimize this. I don't know if some kind of a Write only lock could help here?

jkotas commented 2 months ago

Workflow Core

Is that https://github.com/danielgerlag/workflow-core ?

vasicvuk commented 2 months ago

Yes

julealgon commented 2 months ago

@vasicvuk it might be worth it opening an issue on the workflow-core repo for them to look into this. The fact that there are this many GetEnvironmentVariables calls happening to the point of it becoming a bottleneck seems to be an oversight on the framework part IMHO.

vasicvuk commented 2 months ago

Thanks i will open an issue there. Should I close this one or you feel that we can improve this part also?

julealgon commented 2 months ago

@vasicvuk

Should I close this one or you feel that we can improve this part also?

I won't comment on that myself as I'm not part of the team. Maybe there is something that could still be optimized? Up to you.

jkotas commented 2 months ago

There is not much to optimize here. GetEnvironmentVariables is a relatively expensive API that should not be called repeatedly.