Allow preinitializing AppContext.TryGetSwitch

[MichalStrehovsky]

Interpret this in the static constructor interpreter. Allows compile-time initializing ~30 extra types in the Stage1 app. 99% of them are the SR class.

To actually preinitialize the SR class I had to pass --feature:System.Resources.UseSystemResourceKeys=false on the ILC command line. This will get overwritten by --feature:System.Resources.UseSystemResourceKeys=true if it shows up on the command line after this.

I considered doing this in the substitution IL rewriter (so that RyuJIT-produced code can also benefit), but the API shape (returning bool and taking ref to a bool) makes this really awkward to rewrite in IL. Much easier to interpret.

Cc @dotnet/ilc-contrib

[ghost]

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

Issue Details

---

Interpret this in the static constructor interpreter. Allows compile-time initializing ~30 extra types in the Stage1 app. 99% of them are the SR class.

To actually preinitialize the SR class I had to pass --feature:System.Resources.UseSystemResourceKeys=false on the ILC command line. This will get overwritten by --feature:System.Resources.UseSystemResourceKeys=true if it shows up on the command line after this.

I considered doing this in the substitution IL rewriter (so that RyuJIT-produced code can also benefit), but the API shape (returning bool and taking ref to a bool) makes this really awkward to rewrite in IL. Much easier to interpret.

Cc @dotnet/ilc-contrib

Author:	MichalStrehovsky
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[jkotas]

AppContext key/value pairs are mutable at runtime. Can this lead to odd behaviors when the AppContext is mutated at runtime?

[vitek-karas]

AppContext key/value pairs are mutable at runtime. Can this lead to odd behaviors when the AppContext is mutated at runtime?

Both trimmer and AOT already "inline" some of the feature switches anyway. The fact that AppContext can mutate was discussed around that behavior already, and the outcome was that we're OK ignoring the mutation for these cases. This change expands the set of calls to AppContext which are effectively considered immutable, but it's not a new behavior really. And this also only applies to known trimmer feature switches - which are specifically marked in the runtime config collection for trimming.

[jkotas]

Both trimmer and AOT already "inline" some of the feature switches anyway

The existing inlining behavior has same observable effect as caching of feature switch values.

Inlining of AppContext.TryGetSwitch calls brings it to a different level. For example, consider this program:

using System;

class Program
{
   static readonly bool v = AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", out bool tmp) && tmp;

   // static readonly bool v = AppContext.TryGetSwitch(string.Concat("System.Resources.", "UseSystemResourceKeys"), out bool tmp) && tmp;

   static void Main()
   {
      AppContext.SetSwitch("System.Resources.UseSystemResourceKeys", true);
      Console.WriteLine(v);
   }
}

It prints false when the first AppContext.TryGetSwitch line is used and it prints true when the second AppContext.TryGetSwitch line is used. How do we explain this in the docs?

Should we mark AppContext.SetSwitch as trim/AOT incompatible to make it clear that it can have surprising behaviors with trimming? Or throw an exception when AppContext.SetSwitch is used on a value that was inlined at build time?

[filipnavara]

Or throw an exception when AppContext.SetSwitch is used on a value that was inlined at build time?

FWIW I would find that useful to do that.

[tannergooding]

Or throw an exception when AppContext.SetSwitch is used on a value that was inlined at build time?

I would think this is quiet dangerous. There are many places where similar situations can occur from static readonly and other places where DEBUG vs RELEASE can subtly differ. Having the compiler throw because of one type of mutation might be highly undesirable.

We have a way already to force static readonly values to be a constant result (via embedding an ILLink.Substitutions.xml file as a resource) and this PR just seems like a simpler way to opt into that for bool in particular. It'd be nice if it could also work for string and int (or other primitive types), but that could always be a "future" thing as well.

I'm not really convinced this is trim or AOT incompatible either, you can see many of the same behaviors at JIT time. I'd think it would only make sense to warn if the analyzer detects you're forcing the switch such that mutation is no longer valid.

[MichalStrehovsky]

I look at this as the specific string. This will not kick in for anything but the feature switch strings (no other RuntimeHostConfiguration options get inlined). System.Resources.UseSystemResourceKeys is not documented anywhere. If someone digs it out of the source code in the runtime repo and tries to set it at runtime they'll still get weird behaviors because we could have latched it into a static in the past. We explicitly don't support setting these at runtime.

Like Vitek said we wanted to have a read-only mechanism to query these at runtime (not AppContext) but it was an explicit decision that we're okay with people shooting themselves in the foot and we don't want to build a new mechanism just for that. Other features (like serialization guard) also wanted a read-only AppContext. It was also shot down. If we want to reopen that conversation, I'm up for that. These must be read-only.

[MichalStrehovsky]

After sleeping on this, I see two options:

1. Go with what's here

Consider "What if someone calls SetData using undocumented strings to affect runtime behaviors" the same how we consider "what if someone reflection-invokes a runtime implementation detail". Someone can absolutely reflection-invoke e.g. RuntimeType.ActivatorCache to get a warning-free way to activate arbitrary types (we'll not warn for this because these private APIs are not annotated for trim safety). If someone files a bug that they get a runtime behavioral difference, we're absolutely going to won't fix it without bothering to annotate the API. The problem is that they dug out some unsupported type using reflection, not that we don't generate a warning.

2. Reopen the conversation about a read-only appcontext.

What we probably want here is:

class AppContext
{
    public static bool GetRuntimeConfigurationSwitch(string name, bool defaultValue) { }
}

This would:

1. Purely just read from the compile-time created runtime configuration (e.g. runtimeconfig.json, or the embedded blob on native AOT side). No way to override at runtime, besides hacking those files.
2. Give us an API shape that is easy to do substitution with (no more of the "place the result in a byref and return a bool")

This would allow us to get rid of substitution XML garbage for most scenarios because the IL Linker could also see and optimize this without having to worry about being inconsistent if someone reverse engineers a feature switch name and tries to SetData it later.

@vitek-karas @sbomer what do you think?

[sbomer]

@jkotas I'd like to clarify which specific aspect of the inlining behavior you find problematic.

The existing inlining behavior has same observable effect as caching of feature switch values.

Inlining of AppContext.TryGetSwitch calls brings it to a different level.

The existing inlining behavior can show observable differences similar to your example:

using System;

class Program
{
   static bool V => AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", out bool tmp) && tmp;

   static void Main()
   {
      AppContext.SetSwitch("System.Resources.UseSystemResourceKeys", true);
      Console.WriteLine(V);
   }
}

This prints false when there is an XML substitution (<method signature="System.Boolean get_V()" body="stub" value="false" />) and true otherwise.

Do you consider this similarly problematic, or is your point that inlining based on whether the string argument is a constant is less explainable than an explicit substitution?

[sbomer]

Personally I think a read-only AppContext would be nice, but it seems orthogonal to this change. It would still have the problem that AppContext.GetRuntimeConfigurationSwitch("System.Resources.UseSystemResourceKeys", false) can get inlined to support preinit, while AppContext.GetRuntimeConfigurationSwitch(string.Concat("System.Resources.", "UseSystemResourceKeys"), false) cannot.

I would like to establish a pattern for feature switches that reliably has the desired semantics and allowed optimizations. We should either decide that the supported pattern requires a constant string (for both preinit inlining and branch removal), or use the substitutions for both.

[jkotas]

Do you consider this similarly problematic, or is your point that inlining based on whether the string argument is a constant is less explainable than an explicit substitution?

I do not have a problem with Xml substitution. The Xml substitutions are not an optimization, they are guaranteed to happen.

I have a problem with optimizations that are not guaranteed to happen and that change observable functional behavior when they do. For example, Roslyn can decide to store the constant string in a local variable or confuse the pattern matching in some other way - it should not result into an observable functional behavior difference.

[sbomer]

Thanks! I tend to agree. I would additionally like it if the inlining for preinit could rely on the substitutions, even if that's not an observable functional behavior. We rely on the substitutions for branch removal already. It would be nice to use this as the source of truth for both optimizations.

Could we detect that since UsingResourceKeys is substituted, s_usingResourceKeys is unused, and eliminate the type initializer altogether?

[MichalStrehovsky]

I have a problem with optimizations that are not guaranteed to happen and that change observable functional behavior when they do. For example, Roslyn can decide to store the constant string in a local variable or confuse the pattern matching in some other way - it should not result into an observable functional behavior difference.

The point I'm trying to make is that this is only "observable functional behavior difference" if we consider someone doing SetData with these switches at runtime a supported thing in the first place.

Suppose someone puts <UseSystemResourceKeys>true</UseSystemResourceKeys> (a supported switch) to their project file and observe resource strings got stripped. Then they decide to put AppContext.SetData("System.Resources.UseSystemResourceKeys", false); in their main for whatever reason (this is a supported API, so whatever). Now resource strings are not stripped at runtime. But wait, if they publish their app with trimming or AOT, the resource strings are stripped. And also there was no warning that we're producing a behavioral difference. @jkotas is this a trimming bug?

[MichalStrehovsky]

Could we detect that since UsingResourceKeys is substituted, s_usingResourceKeys is unused, and eliminate the type initializer altogether?

We don't have capability to trim fields, but we can eliminate entire static bases and I think in this case it's going to be eliminated once the static constructor runs at compile time.

[jkotas]

if they publish their app with trimming or AOT, the resource strings are stripped. And also there was no warning that we're producing a behavioral difference. @jkotas is this a trimming bug?

Observable effects of this case are similar to observable effects of the cached appcontext value. I have tried to explain the distinction in #95948 (comment).

[jkotas]

We use IL pattern matching in number of places today. If the IL pattern matching changes observable behavior, we typically produce warnings when we fail to pattern match the IL. For example, failure to pattern match IL for reflection annotations will produce warning. Do we have any existing situations where failure to pattern match IL changes behavior without producing a warning?

[MichalStrehovsky]

Do we have any existing situations where failure to pattern match IL changes behavior without producing a warning?

RuntimeHelpers.InitializeArray for example. But note that this PR is not a pattern match; this is running the cctor at compile time. If the cctor can be interpreted, the effect is that the cctor executed before someone managed to call SetData. If it cannot be interpreted because it's too complex, it will run at a later time, which may or may not be after someone called SetData.

Observable effects of this case are similar to observable effects of the cached appcontext value

To me it sounds like the same timing issue - the SetData might be called after someone already read the data and latched it and trimming makes it so it gets "latched" at trimming time.

[jkotas]

Ok, I have been thinking about what a more robust implementation of this optimization can look like: Scan the app for AppDomain.SetData and AppContext.SetSwitch calls. If there are none, enable this optimizations. The optimization can kick in even for unknown keys. The hack that sets UseSystemResourceKeys=false to trigger the optimization is no longer necessary.

This leads to a problem with needing to restart the scanning, or at least invalidate and recompute some of the scanning results.

Are there other situations where we can do an optimization once we know that certain rarely used API is not used anywhere in the app? Would it be worth it to generalize infrastructure for this pattern?

[MichalStrehovsky]

Ok, I have been thinking about what a more robust implementation of this optimization can look like: Scan the app for AppDomain.SetData and AppContext.SetSwitch calls. If there are none, enable this optimizations. The optimization can kick in even for unknown keys. The hack that sets UseSystemResourceKeys=false to trigger the optimization is no longer necessary.

I think the applicability of this would be fairly limited because we only do it in the cctor and doing this extra analysis will more than double the amount of code we need to write for this feature. Most other places within the framework don't actually read this in cctors but have their own lazy scheme.

This has a really simple fix - add this to ILLink.Substitutions.Resources.template:

  <assembly fullname="{AssemblyName}" feature="System.Resources.UseSystemResourceKeys" featurevalue="false" featuredefault="true">
    <type fullname="System.SR">
      <method signature="System.Boolean UsingResourceKeys()" body="stub" value="false" />
      <method signature="System.Boolean GetUsingResourceKeysSwitchValue()" body="stub" value="false" />
    </type>

And then fix up SR.cs to do this:

-        private static readonly bool s_usingResourceKeys = AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false;
+        private static readonly bool s_usingResourceKeys = GetUsingResourceKeysSwitchValue();
+
+        // This method is a target of ILLink substitution.
+        private static bool GetUsingResourceKeysSwitchValue() => AppContext.TryGetSwitch("System.Resources.UseSystemResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false;

Unfortunately that runs into #96539 - ILLink will do this substitution at the time we build the runtime repo. The refcount on that issue is starting to be pretty high.

[vitek-karas]

I moved the trimmer issue into the runtime repo - for better visibility.