Skip to content
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.

Sign up for GitHub

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

Add DOTNET_MODIFIABLE_ASSEMBLIES env var for hot reload support #48731

Merged
merged 3 commits into from
Feb 25, 2021
Merged

Add DOTNET_MODIFIABLE_ASSEMBLIES env var for hot reload support #48731

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b79f1c
Add EXTERNAL_DOTNET_MODIFIABLE_ASSEMBLIES env var for hot reload support
mikem8361 Feb 24, 2021
94a0a48
Code review feedback
mikem8361 Feb 25, 2021
a25d999
More code review feedback
mikem8361 Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/coreclr/inc/clrconfigvalues.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,7 @@ CONFIG_DWORD_INFO_EX(INTERNAL_MD_MiniMDBreak, W("MD_MiniMDBreak"), 0, "ASSERT wh
CONFIG_DWORD_INFO_EX(INTERNAL_MD_PreSaveBreak, W("MD_PreSaveBreak"), 0, "ASSERT when calling CMiniMdRw::PreSave", CLRConfig::EEConfig_default)
CONFIG_DWORD_INFO_EX(INTERNAL_MD_RegMetaBreak, W("MD_RegMetaBreak"), 0, "ASSERT when creating RegMeta class", CLRConfig::EEConfig_default)
CONFIG_DWORD_INFO_EX(INTERNAL_MD_RegMetaDump, W("MD_RegMetaDump"), 0, "Dump MD in 4 functions (?)", CLRConfig::EEConfig_default)
RETAIL_CONFIG_STRING_INFO_EX(EXTERNAL_DOTNET_MODIFIABLE_ASSEMBLIES, W("DOTNET_MODIFIABLE_ASSEMBLIES"), "Enables hot reload on debug built assemblies with the 'debug' keyword", CLRConfig::DontPrependCOMPlus_ | CLRConfig::TrimWhiteSpaceFromStringValue);

// Metadata - mscordbi only - this flag is only intended to mitigate potential issues in bug fix 458597.
RETAIL_CONFIG_DWORD_INFO_EX(EXTERNAL_MD_PreserveDebuggerMetadataMemory, W("MD_PreserveDebuggerMetadataMemory"), 0, "Save all versions of metadata memory in the debugger when debuggee metadata is updated", CLRConfig::EEConfig_default)
Expand Down
21 changes: 6 additions & 15 deletions src/coreclr/vm/ceeload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,11 +686,6 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName)
Module::CreateAssemblyRefByNameTable(pamTracker);
}

// If the program has the "ForceEnc" env variable set we ensure every eligible
// module has EnC turned on.
if (g_pConfig->ForceEnc() && IsEditAndContinueCapable())
EnableEditAndContinue();

#if defined(PROFILING_SUPPORTED) && !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE)
m_pJitInlinerTrackingMap = NULL;
if (ReJitManager::IsReJITInlineTrackingEnabled())
Expand Down Expand Up @@ -1078,17 +1073,13 @@ void Module::SetDebuggerInfoBits(DebuggerAssemblyControlFlags newBits)
m_dwTransientFlags |= (newBits << DEBUGGER_INFO_SHIFT_PRIV);

#ifdef DEBUGGING_SUPPORTED
BOOL setEnC = ((newBits & DACF_ENC_ENABLED) != 0) && IsEditAndContinueCapable();

// The only way can change Enc is through debugger override.
if (setEnC)
if (IsEditAndContinueCapable())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code depends on IsEditAndContinueCapable to be invariant, but it may actually change because of it depends on GetDebuggerInfoBits that the debugger can change. There is commented out assert in BOOL IsEditAndContinueEnabled() that suggest it may actually happen in some cases. I think that this can interact poorly with this change, depending on how the debugger decides to change the bits.

I think we should:

  • Compute IsEditAndContinueCapable just once in Module::Create
  • Use immutable cached value of IsEditAndContinueCapable everywhere else. It can be achieve by changing BOOL IsEditAndContinueCapable() to virtual method that returns false in class Module and returns true in class EditAndContinueModule.

Copy link
Member

@jkotas jkotas Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, enable the assert in IsEditAndContinueEnabled and change it to just return ((m_dwTransientFlags & IS_EDIT_AND_CONTINUE) != 0);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that after Module::Create the DACF_ALLOW_JIT_OPTS in the Assembly will not change? What about the debugger API (via DacDbiInterfaceImpl::SetCompilerFlags)? Isn't the module already created by then? It is clear to me and it is confusing because Assembly, DomainAssembly and Module all have flags.

To double check what you are saying about IsEditAndContinueCapable():

Module:
  virtual BOOL IsEditAndContinueCapable() const { return FALSE; }
EditAndContinueModule:
  virtual BOOL IsEditAndContinueCapable() const { return TRUE; }

Module::Create() doesn't need any changes because it already creates an
EditAndContinueModule instance if IsEditAndContinueCapable(pAssembly, file) returns true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is confusing and hard to reason about.

Module::Create() doesn't need any changes

Correct.

{
EnableEditAndContinue();
}
else
{
if (!g_pConfig->ForceEnc())
DisableEditAndContinue();
BOOL setEnC = (newBits & DACF_ENC_ENABLED) != 0 || g_pConfig->ForceEnc() || (g_pConfig->DebugAssembliesModifiable() && CORDisableJITOptimizations(GetDebuggerInfoBits()));
Copy link
Member

@jkotas jkotas Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that IsEditAndContinueCapable returns false when the JIT optimizations are enabled for module.

It suggests that CORDisableJITOptimizations is unnecessary here and this new setting behaves same as the existing ForceEnC switch for all practical purposes. It may be worth capturing this as a comment at least since it is not obvious by looking at the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed up to this point that when the ForceEnc env var was set all release and debug built binaries are ENC'able (except system, dynamic, resource and ready to run). If what you implied above that DACF_ALLOW_JIT_OPTS is already set when the Module or ENC module is created, then yes CORDisableJITOptimizations will always return the same thing (except something about profiling or instrumentation). But I may be confusing between the flags set in the Assembly with the ones set in the Module/ENC Module. I need to dig deeper into this. This is a lot more messy and complicated than I was hoping.

if (setEnC)
{
EnableEditAndContinue();
}
}
#endif // DEBUGGING_SUPPORTED

Expand Down
9 changes: 0 additions & 9 deletions src/coreclr/vm/ceeload.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1806,15 +1806,6 @@ class Module
m_dwTransientFlags |= IS_EDIT_AND_CONTINUE;
}

void DisableEditAndContinue()
{
LIMITED_METHOD_CONTRACT;
SUPPORTS_DAC;
// don't _ASSERTE(IsEditAndContinueCapable());
LOG((LF_ENC, LL_INFO100, "DisableEditAndContinue: this:0x%x, %s\n", this, GetDebugName()));
m_dwTransientFlags = m_dwTransientFlags.Load() & (~IS_EDIT_AND_CONTINUE);
}

BOOL IsTenured()
{
LIMITED_METHOD_CONTRACT;
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/vm/eeconfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,13 @@ fTrackDynamicMethodDebugInfo = CLRConfig::GetConfigValue(CLRConfig::UNSUPPORTED_
fStressLog = GetConfigDWORD_DontUse_(CLRConfig::UNSUPPORTED_StressLog, fStressLog) != 0;
fForceEnc = GetConfigDWORD_DontUse_(CLRConfig::UNSUPPORTED_ForceEnc, fForceEnc) != 0;

{
NewArrayHolder<WCHAR> wszModifiableAssemblies;
IfFailRet(CLRConfig::GetConfigValue(CLRConfig::EXTERNAL_DOTNET_MODIFIABLE_ASSEMBLIES, &wszModifiableAssemblies));
if (wszModifiableAssemblies)
fDebugAssembliesModifiable = _wcsicmp(wszModifiableAssemblies, W("debug")) == 0;
}

iRequireZaps = RequireZapsType(GetConfigDWORD_DontUse_(CLRConfig::EXTERNAL_ZapRequire, iRequireZaps));
if (IsCompilationProcess() || iRequireZaps >= REQUIRE_ZAPS_COUNT)
iRequireZaps = REQUIRE_ZAPS_NONE;
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/vm/eeconfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,7 @@ class EEConfig

bool StressLog() const { LIMITED_METHOD_CONTRACT; return fStressLog; }
bool ForceEnc() const { LIMITED_METHOD_CONTRACT; return fForceEnc; }
bool DebugAssembliesModifiable() const { LIMITED_METHOD_CONTRACT; return fDebugAssembliesModifiable; }

// Optimizations to improve working set

Expand Down Expand Up @@ -691,6 +692,7 @@ class EEConfig

bool fStressLog;
bool fForceEnc;
bool fDebugAssembliesModifiable;
bool fProbeForStackOverflow;

// Stackwalk optimization flag
Expand Down