Reduce Environment.GetEnvironmentVariables calls on start-up

[benaadams]

Caching it privately here is safer than caching in Environment as the IDictionary is writable and at this level we know its not being modified.

Then throw the cache away at Gen2 as shouldn't hold onto it forever as its only looked at at start-up; but a WeakReference<T> would have the risk of discarding too early as there are likely many allocations at startup.

Before

After

And the allocated IDictionary (HashTable) is collected

As well as the Gen2Callback

Resolves #44733
Contributes to #44598

[ghost]

Tagging subscribers to this area: @maryamariyan, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Resolves #44733

Author:	benaadams
Assignees:	-
Labels:	area-Extensions-Configuration
Milestone:	-

[marek-safar]

This will misbehave if someone adds/sets an environment variable before Gen2 runs (which can be anytime). I think a better fix would be to cache it inside Environment class for Windows too.

[benaadams]

I think a better fix would be to cache it inside Environment class for Windows too.

If it was in Environment would it need to do a defensive copy on return since its returning an IDictionary that is writable; but unlinked from environment vars? That would mean if it was called only once it would create 2 IDictionarys which would be a regression; could alternatively make it a live IDictionary so changes are reflected in Environment, but that might be breaking?

This will misbehave if someone adds/sets an environment variable before Gen2 runs

Might be bigger issues there?

• The ConfigurationProvider has a Set method; however EnvironmentVariablesConfigurationProvider doesn't call Environment.SetEnvironmentVariable if that set method is called but just updates that instance's copy.

• The ConfigurationProvider also has an IChangeToken which says to update and reload the configuration however if Environment.SetEnvironmentVariable is called it doesn't reload and fire that event (unlike with file config), nor is there currently an Environment event that it can listen to trigger that reload of configuration.

[jkotas]

Does this actually improve startup performance that is the primary metric that we care about?

This is replacing one cost (redundant short term allocation that will get collected quickly) with a different cost (dragging the allocation all the way to Gen2 and only allowing it to be collected then).

[benaadams]

This is replacing one cost (redundant short term allocation that will get collected quickly)

I assume its also the reducing the multiple copies of the keys and value environment strings that get stashed in the configuration forever; the p/invokes to get them and the parsing of the environment block to produce them? /cc @stephentoub

runtime/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs

Lines 93 to 108 in c373952

	// Find the = separating the key and value. We skip entries that begin with =. We also skip entries that don't
	// have =, which can happen on some older OSes when the environment block gets corrupted.
	int i = variable.IndexOf('=');
	if (i > 0)
	{
	// Add the key and value.
	string key = new string(variable.Slice(0, i));
	string value = new string(variable.Slice(i + 1));
	try
	{
	// Add may throw if the environment block was corrupted leading to duplicate entries.
	// We allow such throws and eat them (rather than proactively checking for duplication)
	// to provide a non-fatal notification about the corruption.
	results.Add(key, value);
	}
	catch (ArgumentException) { }

Mono/Linux takes a different approach; which may be what @marek-safar #49856 (comment) was referring to and caches everything in a Dictionary<string, string> using that also for getting a single variable; and then returns a copy for all

runtime/src/mono/System.Private.CoreLib/src/System/Environment.Unix.Mono.cs

Lines 88 to 94 in c373952

	private static void EnsureEnvironmentCached()
	{
	if (s_environment == null)
	{
	Interlocked.CompareExchange(ref s_environment, GetSystemEnvironmentVariables(), null);
	}
	}

[jkotas]

Mono/Linux takes a different approach
caches everything in a Dictionary<string, string>

CoreCLR on Linux uses similar approach, except that the cache is in the Win32 PAL. It should be possible to switch CoreCLR to a similar plan on Linux, but not on Windows.

[benaadams]

but not on Windows.

Why wouldn't it work on Windows? (For process level environment vars)

[jkotas]

Code out there depends on environment changes done via SetEnvironmentVariable Win32 API to be visible via managed APIs, and vice versa.

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Caching it privately here is safer than caching in Environment as the IDictionary is writable and at this level we know its not being modified.

Then throw the cache away at Gen2 as shouldn't hold onto it forever as its only looked at at start-up; but a WeakReference<T> would have the risk of discarding too early as there are likely many allocations at startup.

Before

After

And the allocated IDictionary (HashTable) is collected

As well as the Gen2Callback

Resolves #44733
Contributes to #44598

Author:	benaadams
Assignees:	-
Labels:	area-Extensions-Hosting
Milestone:	-

[benaadams]

Code out there depends on environment changes done via SetEnvironmentVariable Win32 API to be visible via managed APIs, and vice versa.

So wouldn't be able to cache in Environment itself to address this.

This is replacing one cost (redundant short term allocation that will get collected quickly) with a different cost (dragging the allocation all the way to Gen2 and only allowing it to be collected then).

Locally (for me) one call to Environment.GetEnvironmentVariables parses and creates about 15kB of strings; the configuration will hold them forever so its about a 30kB saving, and that's without passing connection strings in Env vars which are very chunky and one of the common ways to pass them:

[benaadams]

Oh the 15kb might be 3 calls; so is only a saving of 10kB (and 10Kb less parsing)

[tarekgh]

@benaadams @jkotas what you think is the next step here with this PR?

[jkotas]

I think that this PR can be closed. It does not save that much if anything, and it comes with other issues.

There may be an opportunity to move the caching from Win32 PAL to C# on non-Windows, but that should be a separate PR.

[jkotas]

@benaadams Thank you anyway for giving it a shot! Keep these coming :-)