-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Knobs to populate AppContext switches #86883
Conversation
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.
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsPreviously AppContext switches were set by injecting a method that calls Fixes #77054. Cc @dotnet/ilc-contrib
|
@@ -204,19 +239,19 @@ void RhConfig::ReadEmbeddedSettings(void *volatile * embeddedSettings, void* com | |||
{ | |||
if (*embeddedSettings == NULL) | |||
{ | |||
uint32_t size = ((CompilerEmbeddedSettingsBlob*)compilerEmbeddedSettingsBlob)->Size; | |||
uint32_t count = ((CompilerEmbeddedSettingsBlob*)compilerEmbeddedSettingsBlob)->Count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're treating the size as count incorrectly in rest of the code (missed that in the code review). So this is also a bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up deleting all of this.
It ran into an issue that we had a hard limit on how long a key or value can be and instead of writing more C code, I just deleted it and update the compiler to emit it in a format that we don't need to parse at all and just use as-is.
The less string handling code in C we have to maintain, the better.
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
src/coreclr/nativeaot/System.Private.CoreLib/src/System/AppContext.NativeAot.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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.
Cc @dotnet/ilc-contrib