Read public GC options if available #86068

kant2002 · 2023-05-10T19:48:46Z

Also pass RuntimeHostConfigurationOption into --runtimeopt ILC parameter Fixes : #85961

ghost · 2023-05-10T19:49:00Z

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

Issue Details

Also pass RuntimeHostConfigurationOption into --runtimeopt ILC parameter Fixes : #85961

Author:	kant2002
Assignees:	-
Labels:	`area-NativeAOT-coreclr`
Milestone:	-

MichalStrehovsky · 2023-05-11T00:35:01Z

These configuration names come from different "namespaces". Like I wrote in #85961 (comment) "We need to either add another blob with the new values, or come up with some sort of mangling so that we can store both key/value pairs in the same blob. I don't have an opinion. Whichever looks better."

The approach in this PR will run into issues as soon as there's a conflict between a "Config" name and "Knob" name. Here's how CoreCLR implements this:

runtime/src/coreclr/vm/gcenv.ee.cpp

Lines 1158 to 1176 in 0b5f137

    
           if (CLRConfig::IsConfigOptionSpecified(configKey)) 
        
           { 
        
               CLRConfig::ConfigDWORDInfo info { configKey , 0, CLRConfig::LookupOptions::Default }; 
        
               *value = CLRConfig::GetConfigValue(info) != 0; 
        
               return true; 
        
           } 
        
           else if (publicKey != NULL) 
        
           { 
        
               if (MultiByteToWideChar(CP_ACP, 0, publicKey, -1 /* key is null-terminated */, configKey, MaxConfigKeyLength) == 0) 
        
               { 
        
                   // whatever this is... it's not something we care about. (It was too long, wasn't unicode, etc.) 
        
                   return false; 
        
               } 
        
               if (Configuration::GetKnobStringValue(configKey) != NULL) 
        
               { 
        
                   *value = Configuration::GetKnobBooleanValue(configKey, false); 
        
                   return true; 
        
               } 
        
           }

CLRConfig::GetConfigValue is the moral equivalent of RhConfig. It will try reading the value from embedded config or from a DOTNET_ prefixed environment variable with that name. Configuration::GetKnobStringValue reads the value from the JSON on the other hand. We miss the other thing. I don't think making CLRConfig::GetConfigValue do the other thing is the right approach.

MichalStrehovsky

Thanks for taking up the challenge!

MichalStrehovsky · 2023-05-23T07:34:20Z

src/coreclr/nativeaot/Runtime/RhConfig.cpp

@@ -82,6 +82,55 @@ bool RhConfig::ReadConfigValue(_In_z_ const TCHAR *wszName, uint64_t* pValue, bo
 }

 #ifdef FEATURE_EMBEDDED_CONFIG
+
+bool RhConfig::ReadKnobValue(_In_z_ const TCHAR *wszName, uint64_t* pValue, bool decimal)


We just switched ReadConfigValue from TCHAR to char*. Could you rebase against latest main and apply similar changes as in #86393? Cc @elinor-fung

MichalStrehovsky · 2023-05-23T07:40:03Z

src/coreclr/nativeaot/Runtime/RhConfig.h

 #endif // FEATURE_EMBEDDED_CONFIG

 public:

    bool ReadConfigValue(_In_z_ const TCHAR* wszName, uint64_t* pValue, bool decimal = false);
+#ifdef FEATURE_EMBEDDED_CONFIG
+    bool ReadKnobValue(_In_z_ const TCHAR* wszName, uint64_t* pValue, bool decimal = false);


decimal = false doesn't look right - we don't pass this as true for GC settings but the GC settings in runtimeconfig.json are documented to be decimal (as opposed to the GC settings set through environment variables that are hex).

In fact, do we need the hex support in knobs?

MichalStrehovsky · 2023-05-23T07:41:59Z

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/RuntimeKnobsRootProvider.cs

+
+            foreach (string option in _runtimeKnobs)
+            {
+                byte[] optionBytes = System.Text.Encoding.ASCII.GetBytes(option);


We should use UTF-8 - users can dump anything they want into RuntimeHostConfigurationOption ItemGroup.

Suggested change

byte[] optionBytes = System.Text.Encoding.ASCII.GetBytes(option);

byte[] optionBytes = System.Text.Encoding.UTF8.GetBytes(option);

kant2002 · 2023-05-23T09:41:55Z

What about honoring COMPlus_XXX environment variables while I'm in this location?

MichalStrehovsky · 2023-05-23T09:43:41Z

We should be already honoring the DOTNET_ prefixed ones. Reading from environment variables is the major difference between "knobs" and "switches".

kant2002 · 2023-05-23T12:04:24Z

Yes DOTNET_ is honored, but in the docs still exists old one COMPLUS_, and I thinking about adding that support for switches and not knobs. Just I see it is as supported (not sure for how long) and I suspect that after some time would be customer request for that.

jkotas · 2023-05-23T12:55:17Z

COMPlus_ prefix for env settings is deprecated. We do not need to give it a new life in native AOT. We can add stronger wording in the docs to use DOTNET_.

MichalStrehovsky · 2023-05-24T01:38:53Z

src/coreclr/nativeaot/Runtime/RhConfig.cpp

+
+    const ConfigPair* configPairs = (const ConfigPair*)g_embeddedKnobs;
+
+    // Find the first name which matches (case insensitive to be compat with environment variable counterpart)


TODO: compare with coreclr - is this case insensitive?

Whoops, that TODO was for me, but might as well dump it on you :). Can you compare how this is done in CoreCLR?

casesensetive comparison always. for knobs and configs.

MichalStrehovsky · 2023-05-24T01:41:17Z

src/coreclr/nativeaot/Runtime/RhConfig.cpp

+    const ConfigPair* configPairs = (const ConfigPair*)g_embeddedKnobs;
+
+    // Find the first name which matches (case insensitive to be compat with environment variable counterpart)
+    for (int iSettings = 0; iSettings < RCV_Count; iSettings++)


RCV_Count only make sense if we know the upper bound because this is not something that is user-configurable. RuntimeHostConfigurationOption is unbounded. We need to keep track of the real count. (Same comment applies to other uses of RCV_Count in this file)

MichalStrehovsky · 2023-05-24T01:44:45Z

src/coreclr/nativeaot/Runtime/RhConfig.cpp

@@ -179,6 +191,103 @@ void RhConfig::ReadEmbeddedSettings()
    return;
 }

+bool RhConfig::GetEmbeddedKnob(_In_z_ const char* configName, _Out_ const char** configValue)


The new methods look very similar to the existing ones. Can we share them?

If we share, we'd delete the concept of RCV_Count.

Note that there's another PR in flight making changes here so we might want to pause for a moment to avoid conflicts. #86656 cc @elinor-fung

I do not sure if sharing make things better, but please take a look.

MichalStrehovsky · 2023-05-24T01:46:26Z

src/coreclr/nativeaot/Runtime/gcrhenv.cpp

+        if (g_pRhConfig->ReadKnobValue(publicKey, &uiValue))
+        {
+            *value = uiValue != 0;
+            return true;
+        }


This won't work for settings that specify true/false. Could you mirror how this is handled in CoreCLR?

Also pass RuntimeHostConfigurationOption into --runtimeopt ILC parameter Fixes : dotnet#85961

MichalStrehovsky

Thank you!

src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets

src/coreclr/tools/aot/ILCompiler/ILCompilerRootCommand.cs

src/coreclr/nativeaot/Runtime/RhConfig.cpp

src/coreclr/nativeaot/Runtime/RhConfig.h

kant2002 · 2023-05-30T03:35:01Z

Looks like you are tired to babysit me 😄 thank you!

MichalStrehovsky · 2023-05-30T03:44:48Z

Looks like you are tired to babysit me 😄 thank you!

It was already close enough that we could avoid another round :). Thanks!

Previously AppContext switches were set by injecting a method that calls `AppContext.SetSwitch` at startup. Use the configuration blob added in dotnet#86068 instead. This makes startup a tiny bit faster and the outputs a tiny bit smaller. Fixes dotnet#77054.

Previously AppContext switches were set by injecting a method that calls `AppContext.SetSwitch` at startup. Use the configuration blob added in #86068 instead. This makes startup a tiny bit faster and the outputs a tiny bit smaller. Fixes #77054.

kant2002 requested a review from MichalStrehovsky as a code owner May 10, 2023 19:48

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label May 10, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label May 10, 2023

kant2002 force-pushed the kant/read-public-gc-options branch from 0edad47 to b4a4f08 Compare May 10, 2023 19:50

MichalStrehovsky reviewed May 23, 2023

View reviewed changes

kant2002 force-pushed the kant/read-public-gc-options branch from e4c84c5 to a0a1a65 Compare May 23, 2023 13:35

build-analysis bot mentioned this pull request May 23, 2023

Tracking issue for CI build timeouts #76454

Closed

MichalStrehovsky reviewed May 24, 2023

View reviewed changes

kant2002 added 5 commits May 26, 2023 17:41

Read public GC options if available

7e6f06d

Also pass RuntimeHostConfigurationOption into --runtimeopt ILC parameter Fixes : dotnet#85961

Add --runtimeknobs

d5cbc88

Remove hex suport when reading knobs

e560b6d

Fix stupid bug

7b9e61c

Cosolidate reading code

b128588

kant2002 force-pushed the kant/read-public-gc-options branch from d1a4693 to b128588 Compare May 26, 2023 15:30

This was referenced May 26, 2023

System.Net.Quic.Tests.QuicStreamTests.WriteCanceled_NextWriteThrows test failure #76831

Closed

System.Net.Http.WinHttpHandlerFunctional.Tests.BidirectionStreamingTest.AfterReadResponseServerError_ClientWrite failing #86262

Closed

MichalStrehovsky approved these changes May 30, 2023

View reviewed changes

Apply suggestions from code review

1b3622a

MichalStrehovsky merged commit 15f5f79 into dotnet:main May 30, 2023

kant2002 deleted the kant/read-public-gc-options branch May 30, 2023 04:22

MichalStrehovsky mentioned this pull request May 30, 2023

Use Knobs to populate AppContext switches #86883

Merged

ghost locked as resolved and limited conversation to collaborators Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read public GC options if available #86068

Read public GC options if available #86068

kant2002 commented May 10, 2023

ghost commented May 10, 2023

MichalStrehovsky commented May 11, 2023

MichalStrehovsky left a comment

MichalStrehovsky May 23, 2023

MichalStrehovsky May 23, 2023

MichalStrehovsky May 23, 2023

kant2002 commented May 23, 2023

MichalStrehovsky commented May 23, 2023

kant2002 commented May 23, 2023

jkotas commented May 23, 2023

MichalStrehovsky May 24, 2023

MichalStrehovsky May 24, 2023

kant2002 May 26, 2023

MichalStrehovsky May 24, 2023

MichalStrehovsky May 24, 2023

MichalStrehovsky May 24, 2023

MichalStrehovsky May 24, 2023

kant2002 May 26, 2023

MichalStrehovsky May 24, 2023

MichalStrehovsky left a comment

kant2002 commented May 30, 2023

MichalStrehovsky commented May 30, 2023

	byte[] optionBytes = System.Text.Encoding.ASCII.GetBytes(option);
	byte[] optionBytes = System.Text.Encoding.UTF8.GetBytes(option);


		const ConfigPair* configPairs = (const ConfigPair*)g_embeddedKnobs;

		// Find the first name which matches (case insensitive to be compat with environment variable counterpart)

Read public GC options if available #86068

Read public GC options if available #86068

Conversation

kant2002 commented May 10, 2023

ghost commented May 10, 2023

MichalStrehovsky commented May 11, 2023

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kant2002 commented May 23, 2023

MichalStrehovsky commented May 23, 2023

kant2002 commented May 23, 2023

jkotas commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichalStrehovsky left a comment

Choose a reason for hiding this comment

kant2002 commented May 30, 2023

MichalStrehovsky commented May 30, 2023