Enable dynamic adaptive GC by default in Web SDK when PublishAot==true #33309
Conversation
ghost
added
|
FYI @agocke @vitek-karas @MichalStrehovsky @jkotas for awareness. |
|
|
||
| responseFileContents.Should().Contain("--feature:System.Text.Json.JsonSerializer.IsReflectionEnabledByDefault=false"); | ||
| responseFileContents.Should().Contain("--feature:System.Diagnostics.Tracing.EventSource.IsSupported=true"); | ||
| responseFileContents.Should().Contain("--runtimeknob:System.GC.DynamicAdaptationMode=1"); |
There was a problem hiding this comment.
it's literally called --runtimeknob? 😆
There was a problem hiding this comment.
public Option<string[]> RuntimeKnobs { get; } =
new(new[] { "--runtimeknob" }, Array.Empty<string>, "Runtime knobs to set");😆
There was a problem hiding this comment.
In CoreCLR lingo, the settings one can set through environment variables are "config values" (e.g. DOTNET_gcServer=1). The values in runtimeconfig.json are "knob values" (e.g. System.GC.Server=true).
I can't say I'm thrilled to see the existence/name/contents of the RSP file interpreted outside of the dotnet/runtime repo though.
There was a problem hiding this comment.
I can't say I'm thrilled to see the existence/name/contents of the RSP file interpreted outside of the dotnet/runtime repo though.
Agreed this isn't ideal. But I can't really think of a better way to ensure the properties are flown correctly. Running the app, giving it load, and inspecting its memory usage would be the "real" way to test it. But that's too heavy handed.
The good news is that this is limited to a very small subset of tests (namely 1) to ensure we have the right defaulting logic in place. If we ever change these ilc command line argument names, it is simple to fix it here.
There was a problem hiding this comment.
Agreed this isn't ideal. But I can't really think of a better way to ensure the properties are flown correctly. Running the app, giving it load, and inspecting its memory usage would be the "real" way to test it. But that's too heavy handed.
If we're allowed to run the app, we also inspect AppContext (these also flow to AppContext), or there might be a GC API we can query directly. Similarly for the feature switches.
I'm not saying we need to do this because digging into RSP also works and this code is already written, but maybe for next time.
src/WebSdk/ProjectSystem/Targets/Microsoft.NET.Sdk.Web.ProjectSystem.targets
Outdated
Show resolved
Hide resolved
DamianEdwards
left a comment
DamianEdwards
left a comment
There was a problem hiding this comment.
Only the one comment nit
src/WebSdk/ProjectSystem/Targets/Microsoft.NET.Sdk.Web.ProjectSystem.targets
Show resolved
Hide resolved
…System.targets Co-authored-by: Damian Edwards <[email protected]>
dotnet/sdk#33309 enables GarbageCollectionAdaptationMode=1 by default for ASP.NET apps that use ServerGarbageCollection. Our Server GC benchmarks want to use the "normal" Server GC, so explicitly set the environment variable so they aren't using the adaptive mode.
dotnet/sdk#33309 enables GarbageCollectionAdaptationMode=1 by default for ASP.NET NativeAOT apps that use ServerGarbageCollection. Our Server GC benchmarks want to use the "normal" Server GC, so explicitly set the environment variable so they aren't using the adaptive mode.
5 participants
Fix #33298
cc @mangod9 @Maoni0