-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add support for DOTNET prefix on ICorProfiler env vars (#117902) #121646
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
base: main
Are you sure you want to change the base?
Conversation
Add DOTNET_* variations to existing CORECLR_* profiler environment variables.
|
Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag |
|
@dotnet-policy-service agree company="Microsoft" |
noahfalk
left a comment
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.
Congrats on the first PR! Everything here looks like it works so adding a few suggestions to refine it :)
| @@ -1,23 +1,39 @@ | |||
| **Note:** | |||
| The profiler environment variables now support both `DOTNET` and `CORECLR` prefixes. The `DOTNET` prefix is the new standard, while `CORECLR` is maintained for backwards compatibility and may be removed in the future. **Do not mix both conventions in a project** as this will result in errors. New projects should use the `DOTNET` prefix. | |||
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.
| The profiler environment variables now support both `DOTNET` and `CORECLR` prefixes. The `DOTNET` prefix is the new standard, while `CORECLR` is maintained for backwards compatibility and may be removed in the future. **Do not mix both conventions in a project** as this will result in errors. New projects should use the `DOTNET` prefix. | |
| Starting in .NET 11, profiler environment variables now support both `DOTNET` and `CORECLR` prefixes. The `DOTNET` prefix is the new standard, while `CORECLR` is maintained for backwards compatibility and may be removed in the future. **Do not mix both conventions in a project** as this will result in errors. New projects should use the `DOTNET` prefix. |
| RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_CORECLR_NOTIFICATION_PROFILERS_64, W("CORECLR_NOTIFICATION_PROFILERS_64"), "A semi-colon separated list of notification profilers to load into currently running 64 process in the form \"path={guid}\"", CLRConfig::LookupOptions::DontPrependPrefix) | ||
| RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_CORECLR_NOTIFICATION_PROFILERS_ARM32, W("CORECLR_NOTIFICATION_PROFILERS_ARM32"), "A semi-colon separated list of notification profilers to load into currently running ARM32 process in the form \"path={guid}\"", CLRConfig::LookupOptions::DontPrependPrefix) | ||
| RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_CORECLR_NOTIFICATION_PROFILERS_ARM64, W("CORECLR_NOTIFICATION_PROFILERS_ARM64"), "A semi-colon separated list of notification profilers to load into currently running ARM64 process in the form \"path={guid}\"", CLRConfig::LookupOptions::DontPrependPrefix) | ||
| RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_DOTNET_ENABLE_PROFILING, W("DOTNET_ENABLE_PROFILING"), 0, "CoreCLR only: Flag to indicate whether profiling should be enabled for the currently running process.", CLRConfig::LookupOptions::DontPrependPrefix) |
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.
This looks like it would work but instead of defining all these variables twice perhaps we could tweak the existing fallback behavior a little bit in order to use it? Something like:
RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_CORECLR_ENABLE_PROFILING, W("ENABLE_PROFILING"), 0, "CoreCLR
only: Flag to indicate whether profiling should be enabled for the currently running
process.", CLRConfig::LookupOptions::CoreclrFallbackPrefix)
Then over in clrconfig.cpp
bool coreclrPrefix = CheckLookupOption(options, LookupOptions::CoreclrFallbackPrefix);
fallbackPrefix = coreclrPrefix ? CORECLR_PREFIX : COMPLUS_PREFIX;
And in the cache
if(matchC)
{
if(SString::_wcsnicmp(wszName, COMPLUS_PREFIX, LEN_OF_COMPLUS_PREFIX) == 0)
{
wszName += LEN_OF_COMPLUS_PREFIX;
s_EnvNames.Add(wszName, (DWORD) (wszCurr - wszName));
}
else if(SString::_wcsnicmp(wszName, CORECLR_PREFIX, LEN_OF_CORECLR_PREFIX) == 0)
{
wszName += LEN_OF_CORECLR_PREFIX;
s_EnvNames.Add(wszName, (DWORD) (wszCurr - wszName));
}
}
| bool loadAsNotification = false, | ||
| int notificationCopies = 1) | ||
| { | ||
| // Run the test for both environment variables naming conventions. |
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.
Rather than running every profiler test twice with old and new variables we can probably get nearly the same test coverage and speed things up by picking one regular profiler test and one notification profiler test to run duplicated. Individually the tests probably run pretty fast but its easy for the cost to creep up when we create these multipliers.
| @@ -1,23 +1,39 @@ | |||
| **Note:** | |||
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.
This should be also documented in https://learn.microsoft.com/en-us/dotnet/core/runtime-config/debugging-profiling . The source for the official documentation lives at https://github.com/dotnet/docs/blob/main/docs/core/runtime-config/debugging-profiling.md
It may be best to delete the redundant documentation here and merge this doc into the official docs.
| RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_PROFILER_PATH, W("DOTNET_PROFILER_PATH"), "CoreCLR only: Specifies the path to the DLL of profiler to load into currently running process", CLRConfig::LookupOptions::DontPrependPrefix) | ||
| RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_PROFILER_PATH_32, W("DOTNET_PROFILER_PATH_32"), "CoreCLR only: Specifies the path to the DLL of profiler to load into currently running 32 process", CLRConfig::LookupOptions::DontPrependPrefix) | ||
| RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_PROFILER_PATH_64, W("DOTNET_PROFILER_PATH_64"), "CoreCLR only: Specifies the path to the DLL of profiler to load into currently running 64 process", CLRConfig::LookupOptions::DontPrependPrefix) | ||
| RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_PROFILER_PATH_ARM32, W("DOTNET_PROFILER_PATH_ARM32"), "CoreCLR only: Specifies the path to the DLL of profiler to load into currently running ARM32 process", CLRConfig::LookupOptions::DontPrependPrefix) |
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.
These config settings are missing in the docs
|
|
||
| if (fProfEnabled == 0) | ||
| { | ||
| fProfEnabled = CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_ENABLE_PROFILING); |
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.
- For other configs, the precedence order is DOTNET_ then whatever the legacy prefix was.
- GetConfigValue supports both prefixes. The way it works is; we define the unprefixed knob in configvalues, and let GetConfigValue handle the prefixes and their precedence.
Add DOTNET_* variations to existing CORECLR_* profiler environment variables.