WIP: Getting GC settings from runtimeconfig #25469

ghost · 2019-06-28T02:14:43Z

No description provided.

jkotas · 2019-06-28T06:48:29Z

src/System.Private.CoreLib/shared/System/GCMemoryInfo.cs

+    // TODO: own file
+    public readonly struct GCConfigInfo
+    {
+        public bool AllowVeryLargeObjects { get; }


As public API, it would be more interesting to expose maximum size of array of given type instead. It is hard-coded as constant in number of places currently that is less than ideal.

The API is internal now -- just needed it for testing.

It needs comments to say so.

jkotas · 2019-06-28T06:58:24Z

src/System.Private.CoreLib/shared/System/GCMemoryInfo.cs

+    public readonly struct GCConfigInfo
+    {
+        public bool AllowVeryLargeObjects { get; }
+        public bool CpuGroup { get; }


Many of these look very specialized and platform specific., e.g. CpuGroup is a Windows-specific, LargePages can have multiple values on Unix, ... . They do not feel like good durable APIs.

Is the purpose of this API to return the config for diagnostic purposes? I am wondering whether it would better to have an API to return all overriden runtime config settings as Dictionary or something. It may be useful to have API proposal issue in CoreFX to discuss.

Maoni0 · 2019-07-02T05:35:55Z

@jkotas I asked Andy to change this to an internal API that returns whatever config you are asking for. please take another look. yes this is for diagnostics purpose.

jkotas · 2019-07-02T05:43:55Z

src/utilcode/util.cpp


 #if !defined(FEATURE_REDHAWK) && (defined(_TARGET_AMD64_) || defined(_TARGET_ARM64_))
-    BOOL enableGCCPUGroups     = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_GCCpuGroup) != 0;
+    BOOL enableGCCPUGroups     = Configuration::GetKnobBooleanValue(W("System.GC.CpuGroup"), CLRConfig::EXTERNAL_GCCpuGroup);


This is adding a new official .runtime.json switch. Is it intentional?

yes, we want to allow all public facing GC configs to be specifiable via runtimeconfig.json.

It may be useful to get the list of the new public facing configs and make sure that they are designed well and have good names.

The new public facing GC configs are a new feature that should be approved for 3.0 by @MeiChin-Tsai at this point.

jkotas · 2019-07-02T05:44:51Z

src/vm/comutilnative.cpp

+    if (value != nullptr)
+    {
+        result.Set(value);
+        delete value;


This will leak memory when result.Set throws. Use Holder - we use holders everywhere else in the runtime.

jkotas · 2019-07-02T05:47:58Z

src/inc/configuration.h

    //    - The value of the ConfigurationKnob (searched by name) if it's set (performs a wcstoul)
    //    - The default value passed in
-    static DWORD GetKnobDWORDValue(LPCWSTR name, DWORD defaultValue);
+    static DWORD GetKnobDWORDValueWithDefault(LPCWSTR name, DWORD defaultValue);


What was wrong with the old name?

jkotas · 2019-07-02T05:50:03Z

src/vm/eeconfig.cpp


 #ifdef _WIN64
-    iGCAllowVeryLargeObjects = (CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_gcAllowVeryLargeObjects) != 0);
+    iGCAllowVeryLargeObjects = Configuration::GetKnobBooleanValue(W("System.GC.AllowVeryLargeObjects"), CLRConfig::EXTERNAL_gcAllowVeryLargeObjects);


I do not think we need official switch for AllowVeryLargeObjects. It is left over compat quirk from desktop.

jkotas · 2019-07-02T05:52:03Z

yes this is for diagnostics purpose.

There is a difference between internal testing purposes (internal API is ok) and diagnostic purposes (we tell people out there to call this API via reflection). It is just internal testing purposes, correct?

jkotas · 2019-07-02T05:55:37Z

Also, the change contains both exposing the internal testing API as well as a bunch of new config surface. Should that be split into multiple PRs?

jkotas · 2019-07-02T05:56:53Z

src/vm/eeconfig.cpp

+iGCCpuGroup = Configuration::GetKnobBooleanValue(W("System.GC.CpuGroup"), CLRConfig::EXTERNAL_GCCpuGroup);
+    iGCHighMemPercent = Configuration::GetKnobDWORDValueWithDefault(W("System.GC.HighMemPercent"), 0);
+    iGCHeapHardLimit = Configuration::GetKnobULONGLONGValue(W("System.GC.HeapHardLimit"));
+    fGCLargePages = Configuration::GetKnobBooleanValue(W("System.GC.LargePages"), CLRConfig::EXTERNAL_GCLargePages);


The large pages come in multiple sizes. Do we need to design for that?

I would think we just let the users specify whether they want to use large pages or not and if so we will use as many larger large pages as we can instead of users having to worry about what size to specify

In reply to: 299312301 [](ancestors = 299312301)

jkotas · 2019-07-02T06:00:57Z

src/vm/eeconfig.cpp

 #endif //_WIN64

+iGCCpuGroup = Configuration::GetKnobBooleanValue(W("System.GC.CpuGroup"), CLRConfig::EXTERNAL_GCCpuGroup);
+    iGCHighMemPercent = Configuration::GetKnobDWORDValueWithDefault(W("System.GC.HighMemPercent"), 0);


The idea behind the official .runtime.json config surface is that we will hold it to the same naming standard as public APIs. HighMemPercent does not look like a good name of a public API.

MeiChin-Tsai · 2019-07-02T21:27:30Z

per our discussion, please minimize the PR to .NET Core 3.0 necessity and only expose configuration that is completed in design.

ghost · 2019-07-05T23:42:30Z

A much smaller version of this is #25574

maryamariyan · 2019-11-06T21:04:39Z

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

In your coreclr repository clone, create patch by running git format-patch origin
In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

Maoni0 · 2019-11-08T20:02:40Z

src/vm/eeconfig.cpp

    iGCHeapHardLimitPercent = Configuration::GetKnobDWORDValue(W("System.GC.HeapHardLimitPercent"), 0);

+    iGCCpuGroup = Configuration::GetKnobBooleanValue(W("System.GC.CpuGroup"), CLRConfig::EXTERNAL_GCCpuGroup);
+    iGCHighMemoryPercent = Configuration::GetKnobDWORDValue(W("System.GC.HighMemoryPercent"), 0);


0 [](start = 93, length = 2)

why is this not reading from the complus variable "GCHighMemPercent"?

Maoni0 · 2019-11-08T20:15:30Z

src/gc/gcconfig.h

 // Each one of these keys produces a method on GCConfig with the name "Get{name}", where {name}
 // is the first parameter of the *_CONFIG macros below.
 #define GC_CONFIGURATION_KEYS \
+    BOOL_CONFIG(AllowVeryLargeObjects, "gcAllowVeryLargeObjects", true, "Allow allocation of 2GB+ objects on GC heap") \


AllowVeryLargeObjects [](start = 16, length = 21)

why do we need this? GC does not use it.

It would be best to delete this (in this or separate change). gcAllowVeryLargeObjects was a desktop compact quirk that serves no real purpose in .NET Core.

Maoni0 · 2019-11-08T20:24:08Z

src/System.Private.CoreLib/src/System/GC.cs

+        internal static extern void GetGCConfigValue(string name, StringHandleOnStack retString);
+
+        // internal for now, for testing that configuration works
+        internal static string GetConfigValue(string name)


GetConfigValue [](start = 31, length = 14)

Andy, can you please file an API proposal for this? for the next release we should provide this as a public API

dotnet/corefx#42499

… non-Windows

ghost · 2019-11-08T21:17:12Z

src/vm/comutilnative.cpp

 }
 FCIMPLEND

+void QCALLTYPE GCInterface::GetGCConfigValue(const wchar_t* key, QCall::StringHandleOnStack result)


@jkotas Is this the best way to return a string from a function? GCHeap::GetGCConfigValue has to always return a newly allocated string, which is then put in to a string holder here, result.Set is called, then it's deleted. (If we don't always return a newly allocated one, how would we know whether it should be deleted or not?) Alternately we could try to pass the StringHandleOnStack down to gc.cpp, but I don't think it has the necessary headers to deal with those.

GCHeap::GetGCConfigValue has to always return a newly allocated string, which is then put in to a string holder here, result.Set is called, then it's deleted

Yes, that's the way to do it.

ghost · 2019-11-20T23:55:23Z

Moved to dotnet/runtime#162.

wip

5a13307

jkotas reviewed Jun 28, 2019

View reviewed changes

Andy Hanson added 2 commits July 1, 2019 19:24

wip

357a9d5

add comment

ade7175

jkotas reviewed Jul 2, 2019

View reviewed changes

Code review

c80d58c

fix bug

ac7cd74

ghost mentioned this pull request Jul 26, 2019

Handle more configs in GCToEEInterface #25653

Closed

jkotas added the area-GC label Nov 2, 2019

Merge branch 'master' into runtimeconfig

67a1189

Andy Hanson added 3 commits November 7, 2019 10:24

Fix type declarations to work on non-Windows

ccb84ec

Remove duplicate set for iGCHeapHardLimit and iGCHeapHardLimitPercent

c839049

Use _snprintf_s instead of snprintf

4cf016e

Maoni0 reviewed Nov 8, 2019

View reviewed changes

Use char instead of wchar_t so it will be the same on Windows and…

d8b473c

… non-Windows

ghost commented Nov 8, 2019

View reviewed changes

Andy Hanson added 2 commits November 8, 2019 17:21

Remove new code for allowVeryLargeObjects

b9553ea

Support env var COMPlus_GCHighMemPercent

61f945f

sergiy-k added the post-consolidation PRs which will be hand ported to dotnet/runtime label Nov 12, 2019

Change to public GetConfigurationVariables API and add tests

e280411

ghost closed this Nov 20, 2019

ghost mentioned this pull request Feb 1, 2020

Proposal: GC.GetConfig* API for getting GC config values dotnet/runtime#31450

Closed

This pull request was closed.

WIP: Getting GC settings from runtimeconfig #25469

WIP: Getting GC settings from runtimeconfig #25469

Uh oh!

Conversation

ghost commented Jun 28, 2019

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!

Maoni0 commented Jul 2, 2019

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!

jkotas commented Jul 2, 2019

Uh oh!

jkotas commented Jul 2, 2019

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!

MeiChin-Tsai commented Jul 2, 2019

Uh oh!

ghost commented Jul 5, 2019

Uh oh!

maryamariyan commented Nov 6, 2019

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!

ghost commented Nov 20, 2019

Uh oh!

Uh oh!