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.

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

Improve performance of Debugger.NotifyOfCrossThreadDependency under a debugger #101864

Merged
merged 4 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 28 additions & 5 deletions src/coreclr/debug/di/process.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5318,7 +5318,7 @@ void CordbProcess::RawDispatchEvent(
// if the class is NULL, that means the debugger never enabled notifications for it. Otherwise,
// the CordbClass instance would already have been created when the notifications were
// enabled.
if ((pNotificationClass != NULL) && pNotificationClass->CustomNotificationsEnabled())
if (pNotificationClass != NULL)

{
PUBLIC_CALLBACK_IN_THIS_SCOPE(this, pLockHolder, pEvent);
Expand Down Expand Up @@ -11414,14 +11414,37 @@ const EXCEPTION_RECORD * CordbProcess::ValidateExceptionRecord(
HRESULT CordbProcess::SetEnableCustomNotification(ICorDebugClass * pClass, BOOL fEnable)
{
HRESULT hr = S_OK;
PUBLIC_API_BEGIN(this); // takes the lock
PUBLIC_API_ENTRY(this);
FAIL_IF_NEUTERED(this);
ATT_REQUIRE_STOPPED_MAY_FAIL(GetProcess());

ValidateOrThrow(pClass);

((CordbClass *)pClass)->SetCustomNotifications(fEnable);
CordbProcess * pProcess = GetProcess();
RSLockHolder lockHolder(pProcess->GetProcessLock());

PUBLIC_API_END(hr);
return hr;
DebuggerIPCEvent event;
CordbClass *pCordbClass = static_cast<CordbClass *>(pClass);
_ASSERTE(pCordbClass != NULL);
CordbAppDomain * pAppDomain = pCordbClass->GetAppDomain();
_ASSERTE (pAppDomain != NULL);
CordbModule *pModule = pCordbClass->GetModule();

pProcess->InitIPCEvent(&event,
DB_IPCE_SET_ENABLE_CUSTOM_NOTIFICATION,
true,
pAppDomain->GetADToken());
event.CustomNotificationData.vmModule = pModule->GetRuntimeModule();
event.CustomNotificationData.classMetadataToken = pCordbClass->MDToken();
event.CustomNotificationData.Enabled = fEnable;

lockHolder.Release();
hr = pProcess->m_cordb->SendIPCEvent(pProcess, &event, sizeof(DebuggerIPCEvent));
lockHolder.Acquire();

_ASSERTE(event.type == DB_IPCE_SET_ENABLE_CUSTOM_NOTIFICATION_RESULT);

return event.hr;
} // CordbProcess::SetEnableCustomNotification

//---------------------------------------------------------------------------------------
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/debug/di/rsclass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ CordbClass::CordbClass(CordbModule *m, mdTypeDef classMetadataToken)
m_fIsValueClassKnown(false),
m_fIsValueClass(false),
m_fHasTypeParams(false),
m_continueCounterLastSync(0),
m_fCustomNotificationsEnabled(false)
m_continueCounterLastSync(0)
{
m_classInfo.Clear();
}
Expand Down
7 changes: 0 additions & 7 deletions src/coreclr/debug/di/rspriv.h
Original file line number Diff line number Diff line change
Expand Up @@ -5100,10 +5100,6 @@ class CordbClass : public CordbBase, public ICorDebugClass, public ICorDebugClas

public:

// set or clear the custom notifications flag to control whether we ignore custom debugger notifications
void SetCustomNotifications(BOOL fEnable) { m_fCustomNotificationsEnabled = fEnable; }
BOOL CustomNotificationsEnabled () { return m_fCustomNotificationsEnabled; }

HRESULT GetFieldInfo(mdFieldDef fldToken, FieldData ** ppFieldData);

// If you want to force the init to happen even if we think the class
Expand Down Expand Up @@ -5178,9 +5174,6 @@ class CordbClass : public CordbBase, public ICorDebugClass, public ICorDebugClas
// their value will be hung off the FieldDesc. Hold information about such fields here.
CordbHangingFieldTable m_hangingFieldsStatic;

// this indicates whether we should send custom debugger notifications
BOOL m_fCustomNotificationsEnabled;

};


Expand Down
69 changes: 66 additions & 3 deletions src/coreclr/debug/ee/debugger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -916,6 +916,7 @@ Debugger::Debugger()
m_ignoreThreadDetach(FALSE),
m_pMethodInfos(NULL),
m_pForceCatchHandlerFoundEventsTable(NULL),
m_pCustomNotificationTable(NULL),
m_mutex(CrstDebuggerMutex, (CrstFlags)(CRST_UNSAFE_ANYMODE | CRST_REENTRANCY | CRST_DEBUGGER_THREAD)),
#ifdef _DEBUG
m_mutexOwner(0),
Expand Down Expand Up @@ -958,8 +959,7 @@ Debugger::Debugger()
m_processId = GetCurrentProcessId();

m_pForceCatchHandlerFoundEventsTable = new ForceCatchHandlerFoundTable();


m_pCustomNotificationTable = new CustomNotificationTable();

//------------------------------------------------------------------------------
// Metadata data structure version numbers
Expand Down Expand Up @@ -8045,6 +8045,20 @@ BOOL Debugger::ShouldSendCatchHandlerFound(Thread* pThread)
}
}

BOOL Debugger::ShouldSendCustomNotification(DomainAssembly *pAssembly, mdTypeDef typeDef)
{
CONTRACTL
{
THROWS;
GC_NOTRIGGER;
MODE_ANY;
}
CONTRACTL_END;

Module *pModule = pAssembly->GetModule();
TypeInModule tim(pModule, typeDef);
return !(m_pCustomNotificationTable->Lookup(tim).IsNull());
}

// Actually send the catch handler found event.
// This can be used to send CHF for both regular managed catchers as well
Expand Down Expand Up @@ -10485,6 +10499,26 @@ bool Debugger::HandleIPCEvent(DebuggerIPCEvent * pEvent)
}
break;

case DB_IPCE_SET_ENABLE_CUSTOM_NOTIFICATION:
{
Module * pModule = pEvent->CustomNotificationData.vmModule.GetRawPtr();
mdTypeDef classToken = pEvent->CustomNotificationData.classMetadataToken;
BOOL enabled = pEvent->CustomNotificationData.Enabled;

HRESULT hr = UpdateCustomNotificationTable(pModule, classToken, enabled);

DebuggerIPCEvent * pIPCResult = m_pRCThread->GetIPCEventReceiveBuffer();
InitIPCEvent(pIPCResult,
DB_IPCE_SET_ENABLE_CUSTOM_NOTIFICATION_RESULT,
g_pEEInterface->GetThread(),
pEvent->vmAppDomain);

pIPCResult->hr = hr;

m_pRCThread->SendIPCReply();
}
break;

case DB_IPCE_BREAKPOINT_ADD:
{

Expand Down Expand Up @@ -12408,6 +12442,35 @@ HRESULT Debugger::IsMethodDeoptimized(Module *pModule, mdMethodDef methodDef, BO
return S_OK;
}

HRESULT Debugger::UpdateCustomNotificationTable(Module *pModule, mdTypeDef classToken, BOOL enabled)
{
CONTRACTL
{
THROWS;
CAN_TAKE_LOCK;
GC_NOTRIGGER;
}
CONTRACTL_END;

TypeInModule tim(pModule, classToken);
if (enabled)
{
if (m_pCustomNotificationTable->Lookup(tim).IsNull())
{
m_pCustomNotificationTable->Add(tim);
}
}
else
{
if (!(m_pCustomNotificationTable->Lookup(tim).IsNull()))
{
m_pCustomNotificationTable->Remove(tim);
}
}

return S_OK;
}

HRESULT Debugger::UpdateForceCatchHandlerFoundTable(BOOL enableEvents, OBJECTREF exObj, AppDomain *pAppDomain)
{
CONTRACTL
Expand Down Expand Up @@ -14583,7 +14646,7 @@ void Debugger::SendCustomDebuggerNotification(Thread * pThread,
Thread *curThread = g_pEEInterface->GetThread();
SENDIPCEVENT_BEGIN(this, curThread);

if (CORDebuggerAttached())
if (CORDebuggerAttached() && ShouldSendCustomNotification(pDomain, classToken))
{
DebuggerIPCEvent* ipce = m_pRCThread->GetIPCEventSendBuffer();
InitIPCEvent(ipce,
Expand Down
89 changes: 88 additions & 1 deletion src/coreclr/debug/ee/debugger.h
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,88 @@ class EMPTY_BASES_DECL ForceCatchHandlerFoundSHashTraits : public DefaultSHashTr
}
};
typedef SHash<ForceCatchHandlerFoundSHashTraits> ForceCatchHandlerFoundTable;

class TypeInModule
{
private:
Module *m_module;
mdTypeDef m_typeDef;

public:

bool operator ==(const TypeInModule& other) const
{
return m_module == other.m_module && m_typeDef == other.m_typeDef;
}

bool operator !=(const TypeInModule& other) const
{
return !(*this == other);
}

bool IsNull() const
{
return m_module == NULL && m_typeDef == 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why is there an && here instead of an ||?

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 have && because the NULL TypeInModule is defined as a NULL m_module pointer and a 0 m_typeDef. If one of them is true but not the other, it means that something has created a TypeInModule with invalid data, which is different than a NULL TypeInModule. We shouldn't ever see one or the other, only both

}

INT32 Hash() const
{
return (INT32)((UINT_PTR)m_module ^ m_typeDef);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you choose to use this hash function?

Copy link
Member Author

Choose a reason for hiding this comment

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

For a hash function you want it to be quick to calculate, and also relatively unique to each entry. This is very quick to compute and should be unique most of the time. On 64 bit platforms there is a very low chance that the lower 32 bits of a module could be the same as another module and the typeDef could be repeated.

}

TypeInModule(Module * module, mdTypeDef typeDef)
:m_module(module), m_typeDef(typeDef)
{
LIMITED_METHOD_DAC_CONTRACT;
}

TypeInModule()
:m_module(NULL), m_typeDef(0)
{
LIMITED_METHOD_DAC_CONTRACT;
}
};

class EMPTY_BASES_DECL CustomNotificationSHashTraits : public DefaultSHashTraits<TypeInModule>
{
public:
typedef TypeInModule element_t;
typedef TypeInModule key_t;
static const bool s_NoThrow = false;

static BOOL Equals(const TypeInModule &e, const TypeInModule &f)
{
return e == f;
}
static TypeInModule GetKey(const TypeInModule &e)
{
return e;
}
static INT32 Hash(const TypeInModule &e)
{
return e.Hash();
}
static TypeInModule Null()
{
TypeInModule tim;
return tim;
}
static bool IsNull(const TypeInModule &e)
{
return e.IsNull();
}
static TypeInModule Deleted()
{
TypeInModule tim((Module *)-1, -1);
return tim;
}
static bool IsDeleted(const TypeInModule &e)
{
TypeInModule tim((Module *)-1, -1);
return e == tim;
}
};
typedef SHash<CustomNotificationSHashTraits> CustomNotificationTable;
#endif

/* ------------------------------------------------------------------------ *
Expand Down Expand Up @@ -1974,6 +2056,8 @@ class Debugger : public DebugInterface

BOOL ShouldSendCatchHandlerFound(Thread* pThread);

BOOL ShouldSendCustomNotification(DomainAssembly *pAssembly, mdTypeDef typeDef);

void SendCatchHandlerFound(Thread *pThread,
FramePointer fp,
SIZE_T nOffset,
Expand Down Expand Up @@ -2276,6 +2360,7 @@ class Debugger : public DebugInterface
#endif //DACCESS_COMPILE
HRESULT IsMethodDeoptimized(Module *pModule, mdMethodDef methodDef, BOOL *pResult);
HRESULT UpdateForceCatchHandlerFoundTable(BOOL enableEvents, OBJECTREF exObj, AppDomain *pAppDomain);
HRESULT UpdateCustomNotificationTable(Module *pModule, mdTypeDef classToken, BOOL enabled);

//
// The debugger mutex is used to protect any "global" Left Side
Expand Down Expand Up @@ -2865,9 +2950,11 @@ class Debugger : public DebugInterface
BOOL m_ignoreThreadDetach;
PTR_DebuggerMethodInfoTable m_pMethodInfos;
#ifdef DACCESS_COMPILE
VOID * m_pForceCatchHandlerFoundEventsTable;
VOID *m_pForceCatchHandlerFoundEventsTable;
VOID *m_pCustomNotificationTable;
#else
ForceCatchHandlerFoundTable *m_pForceCatchHandlerFoundEventsTable;
CustomNotificationTable *m_pCustomNotificationTable;
#endif


Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/debug/inc/dbgipcevents.h
Original file line number Diff line number Diff line change
Expand Up @@ -2060,6 +2060,13 @@ struct MSLAYOUT DebuggerIPCEvent
VMPTR_Object vmObj;
} ForceCatchHandlerFoundData;

struct MSLAYOUT
{
VMPTR_Module vmModule;
mdTypeDef classMetadataToken;
BOOL Enabled;
} CustomNotificationData;

struct MSLAYOUT
{
LSPTR_BREAKPOINT breakpointToken;
Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/debug/inc/dbgipceventtypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ IPC_EVENT_TYPE1(DB_IPCE_BEFORE_GARBAGE_COLLECTION ,0x0161)
IPC_EVENT_TYPE1(DB_IPCE_AFTER_GARBAGE_COLLECTION ,0x0162)
IPC_EVENT_TYPE1(DB_IPCE_DISABLE_OPTS_RESULT ,0x0163)
IPC_EVENT_TYPE1(DB_IPCE_CATCH_HANDLER_FOUND_RESULT ,0x0165)
IPC_EVENT_TYPE0(DB_IPCE_RUNTIME_LAST ,0x0166) // The last event from runtime
IPC_EVENT_TYPE1(DB_IPCE_SET_ENABLE_CUSTOM_NOTIFICATION_RESULT, 0x0166)
IPC_EVENT_TYPE0(DB_IPCE_RUNTIME_LAST ,0x0167) // The last event from runtime



Expand Down Expand Up @@ -145,5 +146,6 @@ IPC_EVENT_TYPE2(DB_IPCE_RESOLVE_UPDATE_METADATA_1 ,0x0256)
IPC_EVENT_TYPE2(DB_IPCE_RESOLVE_UPDATE_METADATA_2 ,0x0257)
IPC_EVENT_TYPE2(DB_IPCE_DISABLE_OPTS ,0x0258)
IPC_EVENT_TYPE2(DB_IPCE_FORCE_CATCH_HANDLER_FOUND ,0x025A)
IPC_EVENT_TYPE0(DB_IPCE_DEBUGGER_LAST ,0x025B) // The last event from the debugger
IPC_EVENT_TYPE2(DB_IPCE_SET_ENABLE_CUSTOM_NOTIFICATION, 0x025B)
IPC_EVENT_TYPE0(DB_IPCE_DEBUGGER_LAST ,0x025C) // The last event from the debugger

5 changes: 5 additions & 0 deletions src/coreclr/debug/shared/dbgtransportsession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2502,10 +2502,15 @@ DWORD DbgTransportSession::GetEventSize(DebuggerIPCEvent *pEvent)
case DB_IPCE_DISABLE_OPTS:
cbAdditionalSize = sizeof(pEvent->DisableOptData);
break;

case DB_IPCE_FORCE_CATCH_HANDLER_FOUND:
cbAdditionalSize = sizeof(pEvent->ForceCatchHandlerFoundData);
break;

case DB_IPCE_SET_ENABLE_CUSTOM_NOTIFICATION:
cbAdditionalSize = sizeof(pEvent->CustomNotificationData);
break;

default:
STRESS_LOG1(LF_CORDB, LL_INFO1000, "Unknown debugger event type: 0x%x\n", (pEvent->type & DB_IPCE_TYPE_MASK));
_ASSERTE(!"Unknown debugger event type");
Expand Down