Add new configuration mechanism for CoreCLR. #3896

adityamandaleeka · 2016-03-24T04:20:38Z

This change allows hosts using coreclr_initialize to specify configuration properties via key-value strings. The values of these properties can be retrieved at run time using a new mechanism that allows falling back on CLRConfig settings if desired.

Addresses #3884.

cc: @sergiy-k @jkotas @Maoni0

jkotas · 2016-03-24T04:52:53Z

src/vm/eeconfig.cpp

-    int forceGCconcurrent = CLRConfig::GetConfigValue(CLRConfig::UNSUPPORTED_gcConcurrent);
-    if ((forceGCconcurrent > 0) || (forceGCconcurrent == -1 && g_IGCconcurrent))
+    bool gcConcurrentWasForced = false;
+#ifdef FEATURE_CORECLR


I do not think we need this change and the related change in clrconfig.h.

What will not work if both these changes are reverted?

If we revert the change in clrconfigvalues.h, the startup flag for concurrent GC will not be enabled unless "System.GC.Concurrent" is explicitly set in the JSON configuration. We can change the code (in unixinterface.cpp) that sets the flag so that it uses the boolean argument fallback instead of the COMPlus one and always returns true if the value isn't explicitly set, but then you won't be able to use the COMPlus mechanism to configure the concurrent GC startup flag.

If we keep the change in clrconfigvalues.h, the existing code here that checks for -1 doesn't make sense anymore for CoreCLR.

We are looking up the CLRConfig::UNSUPPORTED_gcConcurrent and System.GC.Concurrent both here as well as in InitializeStartupFlags. It is redundant.

I think we should either:

Lookup both in InitializeStartupFlags and do nothing here (put this piece of code here under #ifndef FEATURE_CORECLR ifdef)

or

Lookup just System.GC.Concurrent in InitializeStartupFlags (use the overload that takes default value as constant) and UNSUPPORTED_gcConcurrent here - the existing code here should work just fine for it, no need to ifdef it.

We can't simply ifdef it out entirely because a bit further down, we need to know whether concurrent GC was forced. If it wasn't, we disable concurrent GC if GCStress is enabled.

However, doing the second option does mean that we won't treat the presence of "System.GC.Concurrent": true as the user forcing concurrent GC (in other words, we may turn it off even if they specify it).

If we're okay with that, I can undo the change in clrconfigvalues.h and here, and change InitializeStartupFlags to fallback to 'true' if "System.GC.Concurrent" isn't set.

I would prefer to keep the existing behavior. I think that we do want to be able run GCStress with Concurrent GC enabled.

We definitely should allow the option to run GCStress with concurrent GC enabled - at the very least, you should be able to turn on/off concurrent GC for GCStress 3.

what is the order of evaluation supposed to be here? complus overwrites startup flag which overwrites config?

what is the order of evaluation supposed to be here? complus overwrites startup flag which overwrites config?

With my changes, we will first determine whether the startup flag for concurrent GC should be set with this criteria:

If the COMPlus value is set explicitly, we will respect that (treating 1 as true and anything else as false).

If not, we will check whether the user has set "System.GC.Concurrent" explicitly to true or false in the config file (in the case of CLI this is the JSON configuration) and respect that.

If neither of them is set explicitly, we will use the default specified in the ConfigDWORDInfo, which for CoreCLR will be 1 (true).

If we go through all that and decide concurrent GC should be on, we will treat that as 'force' enabling. So with no special configuration set for concurrent GC and GCStress on, we will have both enabled. You can turn off concurrent GC by either setting "System.GC.Concurrent" to false in your JSON config or setting the COMPlus value to 0 (or anything other than 1).

treating 1 as true and anything else as false

Pretty much all existing COMPlus settings treat 0 as false, and everything else as true

Ah ok, I will change that in configuration.cpp in my next commit.

adityamandaleeka · 2016-03-25T18:56:54Z

I've updated the PR based on feedback and also updated the code reading ThreadPool_Force[Min|Max]WorkerThreads to use the new system.

Please let me know if there are any further comments.

jkotas · 2016-03-25T19:08:33Z

src/vm/win32threadpool.cpp

    // initialize Worker and CP thread settings
    DWORD forceMin;
-    forceMin = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_ThreadPool_ForceMinWorkerThreads);
+    forceMin = Configuration::GetKnobDWORDValue(W("System.Threading.ThreadPool.MinThreads"), CLRConfig::INTERNAL_ThreadPool_ForceMinWorkerThreads);


Could you please factor this out into a small helper method, so that the actual Configuration call is done in just single place?

Sure, done. I added helper functions for getting min/max. For now, the behavior will be the same though (each check results in a lookup). I think we should add a caching mechanism like we have for the old CLRConfig types, but that can be a separate change (not rushed for RC2).

jkotas · 2016-03-25T23:09:10Z

LGTM modulo comments

adityamandaleeka · 2016-03-25T23:09:48Z

Thanks @jkotas. Comments have been addressed.

sergiy-k · 2016-03-26T04:26:51Z

@dotnet-bot test Windows_NT x86 Release Legacy Backend Build and Test please

sergiy-k · 2016-03-26T06:59:15Z

LGTM

…lues4 Add new configuration mechanism for CoreCLR. Commit migrated from dotnet/coreclr@775003a

dnfclas added the cla-already-signed label Mar 24, 2016

dotnet-bot added the 2 - In Progress label Mar 24, 2016

jkotas reviewed Mar 24, 2016
View reviewed changes

adityamandaleeka force-pushed the configvalues4 branch from 7b1f030 to 922cd56 Compare March 25, 2016 18:53

jkotas reviewed Mar 25, 2016
View reviewed changes

adityamandaleeka force-pushed the configvalues4 branch from 4ea37db to 13e03b8 Compare March 25, 2016 23:09

Add new configuration mechanism for CoreCLR.

1aa1b8b

adityamandaleeka force-pushed the configvalues4 branch from 13e03b8 to 1aa1b8b Compare March 26, 2016 00:55

adityamandaleeka merged commit 775003a into dotnet:master Mar 26, 2016

dotnet-bot removed the 2 - In Progress label Mar 26, 2016

adityamandaleeka mentioned this pull request Mar 27, 2016

Add new configuration mechanism for CoreCLR [RC2] #3950

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#3896 from adityamandaleeka/configva…

3252a38

…lues4 Add new configuration mechanism for CoreCLR. Commit migrated from dotnet/coreclr@775003a

AntonLapounov mentioned this pull request Aug 10, 2022

Account for availability of multiple processor groups on Windows 11+ dotnet/runtime#68639

Merged

Add new configuration mechanism for CoreCLR. #3896

Add new configuration mechanism for CoreCLR. #3896

Uh oh!

Conversation

adityamandaleeka commented Mar 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityamandaleeka commented Mar 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Mar 25, 2016

Uh oh!

adityamandaleeka commented Mar 25, 2016

Uh oh!

sergiy-k commented Mar 26, 2016

Uh oh!

sergiy-k commented Mar 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants