From caf37e7daff8a507a8ee558aeb6f1343748c7400 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 4 Jan 2019 08:28:48 -0800 Subject: [PATCH 01/26] start ripping out eventpipe buffer to tls --- src/vm/eventpipebuffermanager.cpp | 24 +++++++++++++++++------- src/vm/eventpipebuffermanager.h | 7 +++++++ src/vm/threads.h | 1 + src/vm/threads.inl | 10 ++++++++++ 4 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 060707bb3f87..63514c2782c0 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -57,9 +57,9 @@ EventPipeBufferManager::~EventPipeBufferManager() Thread *pThread = NULL; while ((pThread = ThreadStore::GetThreadList(pThread)) != NULL) { - if (pThread->GetEventPipeBufferList() == pThreadBufferList) + if (pThreadBufferList) { - pThread->SetEventPipeBufferList(NULL); + //pThread->SetEventPipeBufferList(NULL); break; } } @@ -95,7 +95,11 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSessio // Determine if the requesting thread has at least one buffer. // If not, we guarantee that each thread gets at least one (to prevent thrashing when the circular buffer size is too small). bool allocateNewBuffer = false; - EventPipeBufferList *pThreadBufferList = pThread->GetEventPipeBufferList(); + + //EventPipeBufferList *pThreadBufferList = pThread->GetEventPipeBufferList(); + EventPipeBufferList *pThreadBufferList = GetThreadBufferList();; + + if(pThreadBufferList == NULL) { pThreadBufferList = new (nothrow) EventPipeBufferList(this); @@ -111,7 +115,8 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSessio } m_pPerThreadBufferList->InsertTail(pElem); - pThread->SetEventPipeBufferList(pThreadBufferList); + SetThreadBufferList(pThreadBufferList); + //pThread->SetEventPipeBufferList(pThreadBufferList); allocateNewBuffer = true; } @@ -327,6 +332,8 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi } // The event is still enabled. Mark that the thread is now writing an event. + //m_threadEventWriteInProgress = true; + pThread->SetEventWriteInProgress(true); // Check one more time to make sure that the event is still enabled. @@ -341,7 +348,9 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi // See if the thread already has a buffer to try. bool allocNewBuffer = false; EventPipeBuffer *pBuffer = NULL; - EventPipeBufferList *pThreadBufferList = pThread->GetEventPipeBufferList(); + + EventPipeBufferList *pThreadBufferList = GetThreadBufferList(); + if(pThreadBufferList == NULL) { allocNewBuffer = true; @@ -554,7 +563,7 @@ void EventPipeBufferManager::DeAllocateBuffers() while ((pThread = ThreadStore::GetThreadList(pThread)) != NULL) { // Get the thread's buffer list. - EventPipeBufferList *pBufferList = pThread->GetEventPipeBufferList(); + EventPipeBufferList *pBufferList = GetThreadBufferList(); if(pBufferList != NULL) { // Attempt to free the buffer list. @@ -592,7 +601,8 @@ void EventPipeBufferManager::DeAllocateBuffers() } // Remove the list reference from the thread. - pThread->SetEventPipeBufferList(NULL); + SetThreadBufferList(NULL); + //pThread->SetEventPipeBufferList(NULL); // Now that all of the list elements have been freed, free the list itself. delete(pBufferList); diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index b72648a764fe..def4ef5b5588 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -15,6 +15,11 @@ class EventPipeBufferList; + +// Thread local storage of EventPipeBufferList +// This is the per-thread object that contains a pointer to an object in m_pPerThreadBufferList above. +//thread_local Volatile m_threadEventWriteInProgress; + class EventPipeBufferManager { @@ -37,6 +42,7 @@ class EventPipeBufferManager // Lock to protect access to the per-thread buffer list and total allocation size. SpinLock m_lock; + #ifdef _DEBUG // For debugging purposes. unsigned int m_numBuffersAllocated; @@ -114,6 +120,7 @@ class EventPipeBufferList // If it is false, then this buffer can be de-allocated after it is drained. Volatile m_ownedByThread; + #ifdef _DEBUG // For diagnostics, keep the thread pointer. Thread *m_pCreatingThread; diff --git a/src/vm/threads.h b/src/vm/threads.h index dbbb00b0bf70..8c633441daf7 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -7039,6 +7039,7 @@ struct ThreadLocalInfo Thread* m_pThread; AppDomain* m_pAppDomain; // This field is read only by the SOS plugin to get the AppDomain void** m_EETlsData; // ClrTlsInfo::data + EventPipeBufferList* m_pThreadBufferList; }; class ThreadStateHolder diff --git a/src/vm/threads.inl b/src/vm/threads.inl index 2da3c1a3767e..278ad1f601db 100644 --- a/src/vm/threads.inl +++ b/src/vm/threads.inl @@ -39,6 +39,16 @@ EXTERN_C inline AppDomain* STDCALL GetAppDomain() return AppDomain::GetCurrentDomain(); } +EXTERN_C inline EventPipeBufferList* STDCALL GetThreadBufferList() +{ + return gCurrentThreadInfo.m_pThreadBufferList; +} + +EXTERN_C inline void STDCALL SetThreadBufferList(EventPipeBufferList* bl) +{ + gCurrentThreadInfo.m_pThreadBufferList = bl; +} + #endif // !DACCESS_COMPILE inline void Thread::IncLockCount() From 28d19d522d99e682e13cdac958c2b52877a3343f Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 4 Jan 2019 13:40:06 -0800 Subject: [PATCH 02/26] can now emit events from gc threads --- src/vm/eventpipe.cpp | 2 ++ src/vm/eventpipebuffer.cpp | 43 ++++++++++++++++++++++++++++--- src/vm/eventpipebuffermanager.cpp | 9 +++---- src/vm/eventpipebuffermanager.h | 2 +- src/vm/threads.cpp | 9 +++++-- src/vm/threads.h | 13 ---------- 6 files changed, 53 insertions(+), 25 deletions(-) diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp index 66b7c8223c4c..1dbea9628ef4 100644 --- a/src/vm/eventpipe.cpp +++ b/src/vm/eventpipe.cpp @@ -751,11 +751,13 @@ void EventPipe::WriteEventInternal(EventPipeEvent &event, EventPipeEventPayload // Get the current thread; Thread *pThread = GetThread(); + /* if(pThread == NULL) { // We can't write an event without the thread object. return; } + */ if(s_pConfig == NULL) { diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index e7489cd0cd37..4fe6a181c2aa 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -54,13 +54,16 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve NOTHROW; GC_NOTRIGGER; MODE_ANY; - PRECONDITION(pThread != NULL); PRECONDITION(((size_t)m_pCurrent % AlignmentSize) == 0); } CONTRACTL_END; // Calculate the size of the event. unsigned int eventSize = sizeof(EventPipeEventInstance) + payload.GetSize(); + DWORD osThreadId; + LPCGUID pThreadActivityID; + GUID incomingMVID; + ZeroMemory(&incomingMVID, sizeof(GUID)); // Make sure we have enough space to write the event. if(m_pCurrent + eventSize >= m_pLimit) @@ -68,21 +71,53 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve return false; } + if (pThread == NULL) + { + // if pthread is NULL, it's likely we are running in GC thread which is not a Thread object, so it can't have an activity ID set anyway + pThreadActivityID = NULL; + // For ThreadID just get the Current Thread ID + osThreadId = ::GetCurrentThreadId(); + } + else + { + osThreadId = pThread->GetOSThreadId(); + } + // Calculate the location of the data payload. BYTE *pDataDest = m_pCurrent + sizeof(EventPipeEventInstance); + EventPipeEventInstance *pInstance; bool success = true; EX_TRY { - // Placement-new the EventPipeEventInstance. - EventPipeEventInstance *pInstance = new (m_pCurrent) EventPipeEventInstance( + LPCGUID pThreadActivityID = pActivityId; + + + if (pThread == NULL) + { + // Placement-new the EventPipeEventInstance. + pInstance = new (m_pCurrent) EventPipeEventInstance( session, event, - pThread->GetOSThreadId(), + osThreadId, + pDataDest, + payload.GetSize(), + NULL, + pRelatedActivityId); + } + else + { + // Placement-new the EventPipeEventInstance. + pInstance = new (m_pCurrent) EventPipeEventInstance( + session, + event, + osThreadId, pDataDest, payload.GetSize(), pActivityId, pRelatedActivityId); + } + // Copy the stack if a separate stack trace was provided. if(pStack != NULL) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 63514c2782c0..4309a3232781 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -77,14 +77,13 @@ EventPipeBufferManager::~EventPipeBufferManager() } } -EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSession &session, Thread *pThread, unsigned int requestSize) +EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSession &session, unsigned int requestSize) { CONTRACTL { NOTHROW; GC_NOTRIGGER; MODE_ANY; - PRECONDITION(pThread != NULL); PRECONDITION(requestSize > 0); } CONTRACTL_END; @@ -334,7 +333,7 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi // The event is still enabled. Mark that the thread is now writing an event. //m_threadEventWriteInProgress = true; - pThread->SetEventWriteInProgress(true); + //pThread->SetEventWriteInProgress(true); // Check one more time to make sure that the event is still enabled. // We do this because we might be trying to disable tracing and free buffers, so we @@ -383,7 +382,7 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi // to switch to preemptive mode here. unsigned int requestSize = sizeof(EventPipeEventInstance) + payload.GetSize(); - pBuffer = AllocateBufferForThread(session, pThread, requestSize); + pBuffer = AllocateBufferForThread(session, requestSize); } // Try to write the event after we allocated (or stole) a buffer. @@ -395,7 +394,7 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi } // Mark that the thread is no longer writing an event. - pThread->SetEventWriteInProgress(false); + //pThread->SetEventWriteInProgress(false); #ifdef _DEBUG if(!allocNewBuffer) diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index def4ef5b5588..c1bda31849ad 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -56,7 +56,7 @@ class EventPipeBufferManager // Allocate a new buffer for the specified thread. // This function will store the buffer in the thread's buffer list for future use and also return it here. // A NULL return value means that a buffer could not be allocated. - EventPipeBuffer* AllocateBufferForThread(EventPipeSession &session, Thread *pThread, unsigned int requestSize); + EventPipeBuffer* AllocateBufferForThread(EventPipeSession &session, unsigned int requestSize); // Add a buffer to the thread buffer list. void AddBufferToThreadBufferList(EventPipeBufferList *pThreadBuffers, EventPipeBuffer *pBuffer); diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index fa316762c172..80f2b2232d5f 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -309,6 +309,7 @@ ThreadLocalInfo gCurrentThreadInfo = NULL, // m_pThread NULL, // m_pAppDomain NULL, // m_EETlsData + NULL, // m_pThreadBufferList }; } // extern "C" @@ -908,11 +909,13 @@ void DestroyThread(Thread *th) #ifdef FEATURE_PERFTRACING // Before the thread dies, mark its buffers as no longer owned // so that they can be cleaned up after the thread dies. - EventPipeBufferList *pBufferList = th->GetEventPipeBufferList(); + /* TODO: Can delete this? + EventPipeBufferList *pBufferList = GetThreadBufferList(); // th->GetEventPipeBufferList(); if(pBufferList != NULL) { pBufferList->SetOwnedByThread(false); } + */ #endif // FEATURE_PERFTRACING if (g_fEEShutDown == 0) @@ -1036,11 +1039,13 @@ HRESULT Thread::DetachThread(BOOL fDLLThreadDetach) #ifdef FEATURE_PERFTRACING // Before the thread dies, mark its buffers as no longer owned // so that they can be cleaned up after the thread dies. + /* EventPipeBufferList *pBufferList = m_pEventPipeBufferList.Load(); if(pBufferList != NULL) { pBufferList->SetOwnedByThread(false); } + */ #endif // FEATURE_PERFTRACING FastInterlockOr((ULONG*)&m_State, (int) (Thread::TS_Detached | Thread::TS_ReportDead)); @@ -1662,7 +1667,7 @@ Thread::Thread() m_pAllLoggedTypes = NULL; #ifdef FEATURE_PERFTRACING - m_pEventPipeBufferList = NULL; +// m_pEventPipeBufferList = NULL; m_eventWriteInProgress = false; memset(&m_activityId, 0, sizeof(m_activityId)); #endif // FEATURE_PERFTRACING diff --git a/src/vm/threads.h b/src/vm/threads.h index 8c633441daf7..89376a01acd6 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -5007,8 +5007,6 @@ class Thread: public IUnknown #ifdef FEATURE_PERFTRACING private: - // The object that contains the list write buffers used by this thread. - Volatile m_pEventPipeBufferList; // Whether or not the thread is currently writing an event. Volatile m_eventWriteInProgress; @@ -5022,17 +5020,6 @@ class Thread: public IUnknown GUID m_activityId; public: - EventPipeBufferList* GetEventPipeBufferList() - { - LIMITED_METHOD_CONTRACT; - return m_pEventPipeBufferList; - } - - void SetEventPipeBufferList(EventPipeBufferList *pList) - { - LIMITED_METHOD_CONTRACT; - m_pEventPipeBufferList = pList; - } bool GetEventWriteInProgress() const { From 73f698bb4f16e0619b93f6215d9a50a0e67848bb Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 4 Jan 2019 14:31:36 -0800 Subject: [PATCH 03/26] cleanup --- src/vm/eventpipe.cpp | 7 ----- src/vm/eventpipebuffer.cpp | 43 ++++++++++++++----------------- src/vm/eventpipebuffermanager.cpp | 8 ++---- src/vm/threads.cpp | 12 ++++----- src/vm/threads.h | 17 +----------- src/vm/threads.inl | 10 +++++++ 6 files changed, 38 insertions(+), 59 deletions(-) diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp index 1dbea9628ef4..691878ab7c8b 100644 --- a/src/vm/eventpipe.cpp +++ b/src/vm/eventpipe.cpp @@ -751,13 +751,6 @@ void EventPipe::WriteEventInternal(EventPipeEvent &event, EventPipeEventPayload // Get the current thread; Thread *pThread = GetThread(); - /* - if(pThread == NULL) - { - // We can't write an event without the thread object. - return; - } - */ if(s_pConfig == NULL) { diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index 4fe6a181c2aa..630f625be407 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -61,9 +61,6 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve // Calculate the size of the event. unsigned int eventSize = sizeof(EventPipeEventInstance) + payload.GetSize(); DWORD osThreadId; - LPCGUID pThreadActivityID; - GUID incomingMVID; - ZeroMemory(&incomingMVID, sizeof(GUID)); // Make sure we have enough space to write the event. if(m_pCurrent + eventSize >= m_pLimit) @@ -73,10 +70,10 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve if (pThread == NULL) { - // if pthread is NULL, it's likely we are running in GC thread which is not a Thread object, so it can't have an activity ID set anyway - pThreadActivityID = NULL; // For ThreadID just get the Current Thread ID osThreadId = ::GetCurrentThreadId(); + + // TODO: Does this work on cross plat? } else { @@ -92,30 +89,30 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve { LPCGUID pThreadActivityID = pActivityId; + // Placement-new the EventPipeEventInstance. if (pThread == NULL) { - // Placement-new the EventPipeEventInstance. - pInstance = new (m_pCurrent) EventPipeEventInstance( - session, - event, - osThreadId, - pDataDest, - payload.GetSize(), - NULL, - pRelatedActivityId); + // if pthread is NULL, it's likely we are running in something like a GC thread which is not a Thread object, so it can't have an activity ID set anyway + pInstance = new (m_pCurrent) EventPipeEventInstance( + session, + event, + osThreadId, + pDataDest, + payload.GetSize(), + NULL, + pRelatedActivityId); } - else + else { - // Placement-new the EventPipeEventInstance. pInstance = new (m_pCurrent) EventPipeEventInstance( - session, - event, - osThreadId, - pDataDest, - payload.GetSize(), - pActivityId, - pRelatedActivityId); + session, + event, + osThreadId, + pDataDest, + payload.GetSize(), + pActivityId, + pRelatedActivityId); } diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 4309a3232781..35b9fa85df45 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -95,7 +95,6 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSessio // If not, we guarantee that each thread gets at least one (to prevent thrashing when the circular buffer size is too small). bool allocateNewBuffer = false; - //EventPipeBufferList *pThreadBufferList = pThread->GetEventPipeBufferList(); EventPipeBufferList *pThreadBufferList = GetThreadBufferList();; @@ -115,7 +114,6 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSessio m_pPerThreadBufferList->InsertTail(pElem); SetThreadBufferList(pThreadBufferList); - //pThread->SetEventPipeBufferList(pThreadBufferList); allocateNewBuffer = true; } @@ -331,9 +329,7 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi } // The event is still enabled. Mark that the thread is now writing an event. - //m_threadEventWriteInProgress = true; - - //pThread->SetEventWriteInProgress(true); + SetEventWriteInProgress(true); // Check one more time to make sure that the event is still enabled. // We do this because we might be trying to disable tracing and free buffers, so we @@ -394,7 +390,7 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi } // Mark that the thread is no longer writing an event. - //pThread->SetEventWriteInProgress(false); + SetEventWriteInProgress(false); #ifdef _DEBUG if(!allocNewBuffer) diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index 80f2b2232d5f..da9c650aae2a 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -909,13 +909,13 @@ void DestroyThread(Thread *th) #ifdef FEATURE_PERFTRACING // Before the thread dies, mark its buffers as no longer owned // so that they can be cleaned up after the thread dies. - /* TODO: Can delete this? + // TODO: Can delete this? EventPipeBufferList *pBufferList = GetThreadBufferList(); // th->GetEventPipeBufferList(); if(pBufferList != NULL) { pBufferList->SetOwnedByThread(false); } - */ + #endif // FEATURE_PERFTRACING if (g_fEEShutDown == 0) @@ -1039,13 +1039,13 @@ HRESULT Thread::DetachThread(BOOL fDLLThreadDetach) #ifdef FEATURE_PERFTRACING // Before the thread dies, mark its buffers as no longer owned // so that they can be cleaned up after the thread dies. - /* - EventPipeBufferList *pBufferList = m_pEventPipeBufferList.Load(); + + EventPipeBufferList *pBufferList = GetThreadBufferList(); if(pBufferList != NULL) { pBufferList->SetOwnedByThread(false); } - */ + #endif // FEATURE_PERFTRACING FastInterlockOr((ULONG*)&m_State, (int) (Thread::TS_Detached | Thread::TS_ReportDead)); @@ -1667,8 +1667,6 @@ Thread::Thread() m_pAllLoggedTypes = NULL; #ifdef FEATURE_PERFTRACING -// m_pEventPipeBufferList = NULL; - m_eventWriteInProgress = false; memset(&m_activityId, 0, sizeof(m_activityId)); #endif // FEATURE_PERFTRACING m_HijackReturnKind = RT_Illegal; diff --git a/src/vm/threads.h b/src/vm/threads.h index 89376a01acd6..c3397734537c 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -5008,9 +5008,6 @@ class Thread: public IUnknown #ifdef FEATURE_PERFTRACING private: - // Whether or not the thread is currently writing an event. - Volatile m_eventWriteInProgress; - // SampleProfiler thread state. This is set on suspension and cleared before restart. // True if the thread was in cooperative mode. False if it was in preemptive when the suspension started. Volatile m_gcModeOnSuspension; @@ -5020,19 +5017,6 @@ class Thread: public IUnknown GUID m_activityId; public: - - bool GetEventWriteInProgress() const - { - LIMITED_METHOD_CONTRACT; - return m_eventWriteInProgress; - } - - void SetEventWriteInProgress(bool value) - { - LIMITED_METHOD_CONTRACT; - m_eventWriteInProgress = value; - } - bool GetGCModeOnSuspension() { LIMITED_METHOD_CONTRACT; @@ -7027,6 +7011,7 @@ struct ThreadLocalInfo AppDomain* m_pAppDomain; // This field is read only by the SOS plugin to get the AppDomain void** m_EETlsData; // ClrTlsInfo::data EventPipeBufferList* m_pThreadBufferList; + bool m_threadEventWriteInProgress; }; class ThreadStateHolder diff --git a/src/vm/threads.inl b/src/vm/threads.inl index 278ad1f601db..5ca012838464 100644 --- a/src/vm/threads.inl +++ b/src/vm/threads.inl @@ -49,6 +49,16 @@ EXTERN_C inline void STDCALL SetThreadBufferList(EventPipeBufferList* bl) gCurrentThreadInfo.m_pThreadBufferList = bl; } +EXTERN_C inline void STDCALL GetEventWriteInProgress() +{ + return gCurrentThreadInfo.m_threadEventWriteInProgress; +} + +EXTERN_C inline void STDCALL SetEventWriteInProgress(bool p) +{ + gCurrentThreadInfo.m_threadEventWriteInProgress = p; +} + #endif // !DACCESS_COMPILE inline void Thread::IncLockCount() From 42b6d5da620261370bce50a78ad9b9f2bc7c5498 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 4 Jan 2019 15:18:06 -0800 Subject: [PATCH 04/26] more cleanup --- src/vm/eventpipebuffermanager.cpp | 19 ++++++++----------- src/vm/eventpipebuffermanager.h | 5 ----- src/vm/threads.cpp | 7 ++++--- src/vm/threads.h | 2 +- src/vm/threads.inl | 10 +++++----- 5 files changed, 18 insertions(+), 25 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 35b9fa85df45..4f49eea651ac 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -57,9 +57,9 @@ EventPipeBufferManager::~EventPipeBufferManager() Thread *pThread = NULL; while ((pThread = ThreadStore::GetThreadList(pThread)) != NULL) { - if (pThreadBufferList) + if (pThreadBufferList == GetThreadEventBufferList()) { - //pThread->SetEventPipeBufferList(NULL); + SetThreadEventBufferList(NULL); break; } } @@ -94,9 +94,7 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSessio // Determine if the requesting thread has at least one buffer. // If not, we guarantee that each thread gets at least one (to prevent thrashing when the circular buffer size is too small). bool allocateNewBuffer = false; - - EventPipeBufferList *pThreadBufferList = GetThreadBufferList();; - + EventPipeBufferList *pThreadBufferList = GetThreadEventBufferList(); if(pThreadBufferList == NULL) { @@ -113,7 +111,7 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSessio } m_pPerThreadBufferList->InsertTail(pElem); - SetThreadBufferList(pThreadBufferList); + SetThreadEventBufferList(pThreadBufferList); allocateNewBuffer = true; } @@ -344,7 +342,7 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi bool allocNewBuffer = false; EventPipeBuffer *pBuffer = NULL; - EventPipeBufferList *pThreadBufferList = GetThreadBufferList(); + EventPipeBufferList *pThreadBufferList = GetThreadEventBufferList(); if(pThreadBufferList == NULL) { @@ -558,13 +556,13 @@ void EventPipeBufferManager::DeAllocateBuffers() while ((pThread = ThreadStore::GetThreadList(pThread)) != NULL) { // Get the thread's buffer list. - EventPipeBufferList *pBufferList = GetThreadBufferList(); + EventPipeBufferList *pBufferList = GetThreadEventBufferList(); if(pBufferList != NULL) { // Attempt to free the buffer list. // If the thread is using its buffer list skip it. // This means we will leak a single buffer, but if tracing is re-enabled, that buffer can be used again. - if(!pThread->GetEventWriteInProgress()) + if(GetEventWriteInProgress()) { EventPipeBuffer *pBuffer = pBufferList->GetAndRemoveHead(); while(pBuffer != NULL) @@ -596,8 +594,7 @@ void EventPipeBufferManager::DeAllocateBuffers() } // Remove the list reference from the thread. - SetThreadBufferList(NULL); - //pThread->SetEventPipeBufferList(NULL); + SetThreadEventBufferList(NULL); // Now that all of the list elements have been freed, free the list itself. delete(pBufferList); diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index c1bda31849ad..72bb1e15991d 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -15,11 +15,6 @@ class EventPipeBufferList; - -// Thread local storage of EventPipeBufferList -// This is the per-thread object that contains a pointer to an object in m_pPerThreadBufferList above. -//thread_local Volatile m_threadEventWriteInProgress; - class EventPipeBufferManager { diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index da9c650aae2a..b53813a377a8 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -309,7 +309,8 @@ ThreadLocalInfo gCurrentThreadInfo = NULL, // m_pThread NULL, // m_pAppDomain NULL, // m_EETlsData - NULL, // m_pThreadBufferList + NULL, // m_pThreadEventBufferList + false, // m_threadEventWriteInProgress }; } // extern "C" @@ -910,7 +911,7 @@ void DestroyThread(Thread *th) // Before the thread dies, mark its buffers as no longer owned // so that they can be cleaned up after the thread dies. // TODO: Can delete this? - EventPipeBufferList *pBufferList = GetThreadBufferList(); // th->GetEventPipeBufferList(); + EventPipeBufferList *pBufferList = GetThreadEventBufferList(); // th->GetEventPipeBufferList(); if(pBufferList != NULL) { pBufferList->SetOwnedByThread(false); @@ -1040,7 +1041,7 @@ HRESULT Thread::DetachThread(BOOL fDLLThreadDetach) // Before the thread dies, mark its buffers as no longer owned // so that they can be cleaned up after the thread dies. - EventPipeBufferList *pBufferList = GetThreadBufferList(); + EventPipeBufferList *pBufferList = GetThreadEventBufferList(); if(pBufferList != NULL) { pBufferList->SetOwnedByThread(false); diff --git a/src/vm/threads.h b/src/vm/threads.h index c3397734537c..a866dc44b419 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -7010,7 +7010,7 @@ struct ThreadLocalInfo Thread* m_pThread; AppDomain* m_pAppDomain; // This field is read only by the SOS plugin to get the AppDomain void** m_EETlsData; // ClrTlsInfo::data - EventPipeBufferList* m_pThreadBufferList; + EventPipeBufferList* m_pThreadEventBufferList; bool m_threadEventWriteInProgress; }; diff --git a/src/vm/threads.inl b/src/vm/threads.inl index 5ca012838464..10c2f91d9132 100644 --- a/src/vm/threads.inl +++ b/src/vm/threads.inl @@ -39,17 +39,17 @@ EXTERN_C inline AppDomain* STDCALL GetAppDomain() return AppDomain::GetCurrentDomain(); } -EXTERN_C inline EventPipeBufferList* STDCALL GetThreadBufferList() +EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList() { - return gCurrentThreadInfo.m_pThreadBufferList; + return gCurrentThreadInfo.m_pThreadEventBufferList; } -EXTERN_C inline void STDCALL SetThreadBufferList(EventPipeBufferList* bl) +EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl) { - gCurrentThreadInfo.m_pThreadBufferList = bl; + gCurrentThreadInfo.m_pThreadEventBufferList = bl; } -EXTERN_C inline void STDCALL GetEventWriteInProgress() +EXTERN_C inline bool STDCALL GetEventWriteInProgress() { return gCurrentThreadInfo.m_threadEventWriteInProgress; } From 170d85424f6aa342283d63adee73b166f1825503 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 4 Jan 2019 15:22:07 -0800 Subject: [PATCH 05/26] more cleanup --- src/vm/threads.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index b53813a377a8..c68be1094c27 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -910,8 +910,7 @@ void DestroyThread(Thread *th) #ifdef FEATURE_PERFTRACING // Before the thread dies, mark its buffers as no longer owned // so that they can be cleaned up after the thread dies. - // TODO: Can delete this? - EventPipeBufferList *pBufferList = GetThreadEventBufferList(); // th->GetEventPipeBufferList(); + EventPipeBufferList *pBufferList = GetThreadEventBufferList(); if(pBufferList != NULL) { pBufferList->SetOwnedByThread(false); From 662f9c0e35fbce5cd32277ddc38f55c4d290d896 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 4 Jan 2019 15:37:48 -0800 Subject: [PATCH 06/26] tested on linux --- src/vm/eventpipebuffer.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index 630f625be407..4fe30cef45d6 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -72,8 +72,6 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve { // For ThreadID just get the Current Thread ID osThreadId = ::GetCurrentThreadId(); - - // TODO: Does this work on cross plat? } else { From f29f9ae78cdefc19a4d13e45619b67c34f7ccb18 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Mon, 7 Jan 2019 15:10:38 -0800 Subject: [PATCH 07/26] Addressing PR comments --- src/vm/eventpipe.cpp | 7 +++--- src/vm/eventpipebuffer.cpp | 36 +++++++++------------------- src/vm/eventpipebuffermanager.cpp | 38 +++++++++++++++++++++++++---- src/vm/eventpipebuffermanager.h | 40 +++++++++++++++++++++++++++++++ src/vm/threads.cpp | 25 ------------------- src/vm/threads.h | 2 -- src/vm/threads.inl | 12 ++-------- 7 files changed, 90 insertions(+), 70 deletions(-) diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp index 691878ab7c8b..35c64bc5548c 100644 --- a/src/vm/eventpipe.cpp +++ b/src/vm/eventpipe.cpp @@ -759,8 +759,9 @@ void EventPipe::WriteEventInternal(EventPipeEvent &event, EventPipeEventPayload } _ASSERTE(s_pSession != NULL); - // If the activity id isn't specified, pull it from the current thread. - if(pActivityId == NULL) + // If the activity id isn't specified AND we are in a managed thread, pull it from the current thread. + // If pThread is NULL (we aren't in writing from a managed thread) then pActivityId can be NULL + if(pActivityId == NULL && pThread != NULL) { pActivityId = pThread->GetActivityId(); } @@ -775,7 +776,7 @@ void EventPipe::WriteEventInternal(EventPipeEvent &event, EventPipeEventPayload // We're not interested in these events and they can cause corrupted trace files because rundown // events are written synchronously and not under lock. // If we encounter an event that did not originate on the thread that is doing rundown, ignore it. - if(!s_pConfig->IsRundownThread(pThread)) + if(!s_pConfig->IsRundownThread(pThread) || pThread == NULL) { return; } diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index 4fe30cef45d6..91b17620ea4b 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -70,7 +70,7 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve if (pThread == NULL) { - // For ThreadID just get the Current Thread ID + // For ThreadID just get the current OS thread ID osThreadId = ::GetCurrentThreadId(); } else @@ -89,30 +89,16 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve // Placement-new the EventPipeEventInstance. - if (pThread == NULL) - { - // if pthread is NULL, it's likely we are running in something like a GC thread which is not a Thread object, so it can't have an activity ID set anyway - pInstance = new (m_pCurrent) EventPipeEventInstance( - session, - event, - osThreadId, - pDataDest, - payload.GetSize(), - NULL, - pRelatedActivityId); - } - else - { - pInstance = new (m_pCurrent) EventPipeEventInstance( - session, - event, - osThreadId, - pDataDest, - payload.GetSize(), - pActivityId, - pRelatedActivityId); - } - + // if pthread is NULL, it's likely we are running in something like a GC thread which is not a Thread object, so it can't have an activity ID set anyway + pInstance = new (m_pCurrent) EventPipeEventInstance( + session, + event, + osThreadId, + pDataDest, + payload.GetSize(), + (pThread == NULL) ? NULL : pActivityId, + pRelatedActivityId); + // Copy the stack if a separate stack trace was provided. if(pStack != NULL) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 4f49eea651ac..ce2f57dd7df8 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -326,9 +326,6 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi return false; } - // The event is still enabled. Mark that the thread is now writing an event. - SetEventWriteInProgress(true); - // Check one more time to make sure that the event is still enabled. // We do this because we might be trying to disable tracing and free buffers, so we // must make sure that the event is enabled after we mark that we're writing to avoid @@ -360,6 +357,9 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi } else { + // The event is still enabled. Mark that the thread is now writing an event. + pThreadBufferList->SetThreadEventWriteInProgress(true); + // Attempt to write the event to the buffer. If this fails, we should allocate a new buffer. allocNewBuffer = !pBuffer->WriteEvent(pEventThread, session, event, payload, pActivityId, pRelatedActivityId, pStack); } @@ -384,11 +384,15 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi // This is the second time if this thread did have one or more buffers, but they were full. if(allocNewBuffer && pBuffer != NULL) { + // By this point, a new buffer list has been allocated so we should fetch it again before using it. + pThreadBufferList = GetThreadEventBufferList(); + // The event is still enabled. Mark that the thread is now writing an event. + pThreadBufferList->SetThreadEventWriteInProgress(true); allocNewBuffer = !pBuffer->WriteEvent(pEventThread, session, event, payload, pActivityId, pRelatedActivityId, pStack); } // Mark that the thread is no longer writing an event. - SetEventWriteInProgress(false); + pThreadBufferList->SetThreadEventWriteInProgress(false); #ifdef _DEBUG if(!allocNewBuffer) @@ -562,7 +566,7 @@ void EventPipeBufferManager::DeAllocateBuffers() // Attempt to free the buffer list. // If the thread is using its buffer list skip it. // This means we will leak a single buffer, but if tracing is re-enabled, that buffer can be used again. - if(GetEventWriteInProgress()) + if(pBufferList->GetThreadEventWriteInProgress()) { EventPipeBuffer *pBuffer = pBufferList->GetAndRemoveHead(); while(pBuffer != NULL) @@ -680,6 +684,7 @@ EventPipeBufferList::EventPipeBufferList(EventPipeBufferManager *pManager) m_bufferCount = 0; m_pReadBuffer = NULL; m_ownedByThread = true; + m_threadEventWriteInProgress = false; #ifdef _DEBUG m_pCreatingThread = GetThread(); @@ -885,6 +890,18 @@ void EventPipeBufferList::SetOwnedByThread(bool value) m_ownedByThread = value; } +bool EventPipeBufferList::GetThreadEventWriteInProgress() +{ + LIMITED_METHOD_CONTRACT; + return m_threadEventWriteInProgress; +} + +void EventPipeBufferList::SetThreadEventWriteInProgress(bool value) +{ + LIMITED_METHOD_CONTRACT; + m_threadEventWriteInProgress = value; +} + #ifdef _DEBUG Thread* EventPipeBufferList::GetThread() { @@ -962,4 +979,15 @@ bool EventPipeBufferList::EnsureConsistency() } #endif // _DEBUG +extern "C" { +#ifndef __llvm__ +__declspec(thread) +#else // !__llvm__ +__thread +#endif // !__llvm__ +ThreadEventBufferList gCurrentThreadEventBufferList = { + NULL, // m_pThreadEventBufferList + }; +} // extern "C" + #endif // FEATURE_PERFTRACING diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index 72bb1e15991d..04b752a355f8 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -15,6 +15,14 @@ class EventPipeBufferList; +EXTERN_C struct ThreadEventBufferList; + +#ifndef __llvm__ +EXTERN_C __declspec(thread) ThreadEventBufferList gCurrentThreadEventBufferList; +#else // !__llvm__ +EXTERN_C __thread ThreadEventBufferList gCurrentThreadEventBufferList; +#endif // !__llvm__ + class EventPipeBufferManager { @@ -115,6 +123,8 @@ class EventPipeBufferList // If it is false, then this buffer can be de-allocated after it is drained. Volatile m_ownedByThread; + // True if a thread is writing something to this list + Volatile m_threadEventWriteInProgress; #ifdef _DEBUG // For diagnostics, keep the thread pointer. @@ -155,6 +165,10 @@ class EventPipeBufferList // The default value is true. void SetOwnedByThread(bool value); + bool GetThreadEventWriteInProgress(); + + void SetThreadEventWriteInProgress(bool value); + #ifdef _DEBUG // Get the thread associated with this list. Thread* GetThread(); @@ -165,6 +179,32 @@ class EventPipeBufferList #endif // _DEBUG }; +// This struct is a TLS wrapper around a pointer to thread-specific EventPipeBufferList +// The struct wrapper is present mainly because we need a way to free the EventPipeBufferList +// when the thread that owns it dies. Placing this struct as a TLS variable will call ~ThreadEventBufferList() +// when the thread dies so we can free EventPipeBufferList in the destructor. +struct ThreadEventBufferList +{ + EventPipeBufferList* m_pThreadEventBufferList; + +~ThreadEventBufferList() +{ + // TODO: Move the cleanup code to here + m_pThreadEventBufferList = NULL; +} + +}; + +EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList() +{ + return gCurrentThreadEventBufferList.m_pThreadEventBufferList; +} + +EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl) +{ + gCurrentThreadEventBufferList.m_pThreadEventBufferList = bl; +} + #endif // FEATURE_PERFTRACING #endif // __EVENTPIPE_BUFFERMANAGER_H__ diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index c68be1094c27..27dfca4f9d79 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -309,8 +309,6 @@ ThreadLocalInfo gCurrentThreadInfo = NULL, // m_pThread NULL, // m_pAppDomain NULL, // m_EETlsData - NULL, // m_pThreadEventBufferList - false, // m_threadEventWriteInProgress }; } // extern "C" @@ -907,17 +905,6 @@ void DestroyThread(Thread *th) #endif // _TARGET_X86_ #endif // WIN64EXCEPTIONS -#ifdef FEATURE_PERFTRACING - // Before the thread dies, mark its buffers as no longer owned - // so that they can be cleaned up after the thread dies. - EventPipeBufferList *pBufferList = GetThreadEventBufferList(); - if(pBufferList != NULL) - { - pBufferList->SetOwnedByThread(false); - } - -#endif // FEATURE_PERFTRACING - if (g_fEEShutDown == 0) { th->SetThreadState(Thread::TS_ReportDead); @@ -1036,18 +1023,6 @@ HRESULT Thread::DetachThread(BOOL fDLLThreadDetach) m_pClrDebugState = NULL; #endif //ENABLE_CONTRACTS_DATA -#ifdef FEATURE_PERFTRACING - // Before the thread dies, mark its buffers as no longer owned - // so that they can be cleaned up after the thread dies. - - EventPipeBufferList *pBufferList = GetThreadEventBufferList(); - if(pBufferList != NULL) - { - pBufferList->SetOwnedByThread(false); - } - -#endif // FEATURE_PERFTRACING - FastInterlockOr((ULONG*)&m_State, (int) (Thread::TS_Detached | Thread::TS_ReportDead)); // Do not touch Thread object any more. It may be destroyed. diff --git a/src/vm/threads.h b/src/vm/threads.h index a866dc44b419..519a2f2dfc12 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -7010,8 +7010,6 @@ struct ThreadLocalInfo Thread* m_pThread; AppDomain* m_pAppDomain; // This field is read only by the SOS plugin to get the AppDomain void** m_EETlsData; // ClrTlsInfo::data - EventPipeBufferList* m_pThreadEventBufferList; - bool m_threadEventWriteInProgress; }; class ThreadStateHolder diff --git a/src/vm/threads.inl b/src/vm/threads.inl index 10c2f91d9132..46429fff9821 100644 --- a/src/vm/threads.inl +++ b/src/vm/threads.inl @@ -39,6 +39,7 @@ EXTERN_C inline AppDomain* STDCALL GetAppDomain() return AppDomain::GetCurrentDomain(); } +/* EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList() { return gCurrentThreadInfo.m_pThreadEventBufferList; @@ -48,16 +49,7 @@ EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl) { gCurrentThreadInfo.m_pThreadEventBufferList = bl; } - -EXTERN_C inline bool STDCALL GetEventWriteInProgress() -{ - return gCurrentThreadInfo.m_threadEventWriteInProgress; -} - -EXTERN_C inline void STDCALL SetEventWriteInProgress(bool p) -{ - gCurrentThreadInfo.m_threadEventWriteInProgress = p; -} +*/ #endif // !DACCESS_COMPILE From 6c7be1f928e5f106d94d2d36049cd6c895cece8c Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Mon, 7 Jan 2019 19:06:54 -0800 Subject: [PATCH 08/26] Move things around a bit to build in Linux --- src/vm/eventpipebuffermanager.cpp | 20 +++++++++---- src/vm/eventpipebuffermanager.h | 47 +++++++++++-------------------- 2 files changed, 31 insertions(+), 36 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index ce2f57dd7df8..1996ac32345b 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -981,13 +981,23 @@ bool EventPipeBufferList::EnsureConsistency() extern "C" { #ifndef __llvm__ -__declspec(thread) +__declspec(thread) ThreadEventBufferList gCurrentThreadEventBufferList = { NULL }; #else // !__llvm__ -__thread +thread_local ThreadEventBufferList gCurrentThreadEventBufferList = { NULL }; #endif // !__llvm__ -ThreadEventBufferList gCurrentThreadEventBufferList = { - NULL, // m_pThreadEventBufferList - }; } // extern "C" + +EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList() +{ + return gCurrentThreadEventBufferList.m_pThreadEventBufferList; +} + +EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl) +{ + gCurrentThreadEventBufferList.m_pThreadEventBufferList = bl; +} + + + #endif // FEATURE_PERFTRACING diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index 04b752a355f8..973f4bd5176f 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -15,13 +15,21 @@ class EventPipeBufferList; -EXTERN_C struct ThreadEventBufferList; +// This struct is a TLS wrapper around a pointer to thread-specific EventPipeBufferList +// The struct wrapper is present mainly because we need a way to free the EventPipeBufferList +// when the thread that owns it dies. Placing this struct as a TLS variable will call ~ThreadEventBufferList() +// when the thread dies so we can free EventPipeBufferList in the destructor. +struct ThreadEventBufferList +{ + EventPipeBufferList* m_pThreadEventBufferList; + +~ThreadEventBufferList() +{ + // TODO: Move the cleanup code to here + m_pThreadEventBufferList = NULL; +} -#ifndef __llvm__ -EXTERN_C __declspec(thread) ThreadEventBufferList gCurrentThreadEventBufferList; -#else // !__llvm__ -EXTERN_C __thread ThreadEventBufferList gCurrentThreadEventBufferList; -#endif // !__llvm__ +}; class EventPipeBufferManager { @@ -179,31 +187,8 @@ class EventPipeBufferList #endif // _DEBUG }; -// This struct is a TLS wrapper around a pointer to thread-specific EventPipeBufferList -// The struct wrapper is present mainly because we need a way to free the EventPipeBufferList -// when the thread that owns it dies. Placing this struct as a TLS variable will call ~ThreadEventBufferList() -// when the thread dies so we can free EventPipeBufferList in the destructor. -struct ThreadEventBufferList -{ - EventPipeBufferList* m_pThreadEventBufferList; - -~ThreadEventBufferList() -{ - // TODO: Move the cleanup code to here - m_pThreadEventBufferList = NULL; -} - -}; - -EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList() -{ - return gCurrentThreadEventBufferList.m_pThreadEventBufferList; -} - -EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl) -{ - gCurrentThreadEventBufferList.m_pThreadEventBufferList = bl; -} +EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList(); +EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl); #endif // FEATURE_PERFTRACING From 71ebf6525bc07d909aa9e29b35e8642676f28555 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 9 Jan 2019 07:54:02 -0800 Subject: [PATCH 09/26] change eventpipe buffer deallocation code --- src/vm/eventpipebuffermanager.cpp | 99 +++++++++---------------------- src/vm/eventpipebuffermanager.h | 40 ++++++++----- 2 files changed, 53 insertions(+), 86 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 1996ac32345b..1432e2e33cee 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -537,6 +537,33 @@ EventPipeEventInstance* EventPipeBufferManager::GetNextEvent() } } +void EventPipeBufferManager::RemovePerThreadBufferListEntry(EventPipeBufferList* listToRemove) +{ + + // Remove the list entry from the per thread buffer list. + SListElem *pElem = m_pPerThreadBufferList->GetHead(); + while(pElem != NULL) + { + EventPipeBufferList* pEntry = pElem->GetValue(); + if(pEntry == listToRemove) + { + pElem = m_pPerThreadBufferList->FindAndRemove(pElem); + + // In DEBUG, make sure that the element was found and removed. + _ASSERTE(pElem != NULL); + + SListElem *pCurElem = pElem; + pElem = m_pPerThreadBufferList->GetNext(pElem); + delete(pCurElem); + } + else + { + pElem = m_pPerThreadBufferList->GetNext(pElem); + } + } +} + + void EventPipeBufferManager::DeAllocateBuffers() { CONTRACTL @@ -549,75 +576,6 @@ void EventPipeBufferManager::DeAllocateBuffers() _ASSERTE(EnsureConsistency()); - // Take the thread store lock because we're going to iterate through the thread list. - { - ThreadStoreLockHolder tsl; - - // Take the buffer manager manipulation lock. - SpinLockHolder _slh(&m_lock); - - Thread *pThread = NULL; - while ((pThread = ThreadStore::GetThreadList(pThread)) != NULL) - { - // Get the thread's buffer list. - EventPipeBufferList *pBufferList = GetThreadEventBufferList(); - if(pBufferList != NULL) - { - // Attempt to free the buffer list. - // If the thread is using its buffer list skip it. - // This means we will leak a single buffer, but if tracing is re-enabled, that buffer can be used again. - if(pBufferList->GetThreadEventWriteInProgress()) - { - EventPipeBuffer *pBuffer = pBufferList->GetAndRemoveHead(); - while(pBuffer != NULL) - { - DeAllocateBuffer(pBuffer); - pBuffer = pBufferList->GetAndRemoveHead(); - } - - // Remove the list entry from the per thread buffer list. - SListElem *pElem = m_pPerThreadBufferList->GetHead(); - while(pElem != NULL) - { - EventPipeBufferList* pEntry = pElem->GetValue(); - if(pEntry == pBufferList) - { - pElem = m_pPerThreadBufferList->FindAndRemove(pElem); - - // In DEBUG, make sure that the element was found and removed. - _ASSERTE(pElem != NULL); - - SListElem *pCurElem = pElem; - pElem = m_pPerThreadBufferList->GetNext(pElem); - delete(pCurElem); - } - else - { - pElem = m_pPerThreadBufferList->GetNext(pElem); - } - } - - // Remove the list reference from the thread. - SetThreadEventBufferList(NULL); - - // Now that all of the list elements have been freed, free the list itself. - delete(pBufferList); - pBufferList = NULL; - } -#ifdef _DEBUG - else - { - // We can't deallocate the buffers. - m_numBuffersLeaked += pBufferList->GetCount(); - } -#endif // _DEBUG - } - } - } - - // Now that we've walked through all of the threads, let's see if there are any other buffers - // that belonged to threads that died during tracing. We can free these now. - // Take the buffer manager manipulation lock SpinLockHolder _slh(&m_lock); @@ -652,9 +610,10 @@ void EventPipeBufferManager::DeAllocateBuffers() { pElem = m_pPerThreadBufferList->GetNext(pElem); } - } + } } + #ifdef _DEBUG bool EventPipeBufferManager::EnsureConsistency() { diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index 973f4bd5176f..28fded83bfe1 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -15,22 +15,6 @@ class EventPipeBufferList; -// This struct is a TLS wrapper around a pointer to thread-specific EventPipeBufferList -// The struct wrapper is present mainly because we need a way to free the EventPipeBufferList -// when the thread that owns it dies. Placing this struct as a TLS variable will call ~ThreadEventBufferList() -// when the thread dies so we can free EventPipeBufferList in the destructor. -struct ThreadEventBufferList -{ - EventPipeBufferList* m_pThreadEventBufferList; - -~ThreadEventBufferList() -{ - // TODO: Move the cleanup code to here - m_pThreadEventBufferList = NULL; -} - -}; - class EventPipeBufferManager { @@ -103,6 +87,9 @@ class EventPipeBufferManager // Get next event. This is used to dispatch events to EventListener. EventPipeEventInstance* GetNextEvent(); + // Deallocate the given Buffer List entry + void RemovePerThreadBufferListEntry(EventPipeBufferList* listToRemove); + #ifdef _DEBUG bool EnsureConsistency(); #endif // _DEBUG @@ -190,6 +177,27 @@ class EventPipeBufferList EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList(); EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl); + +// This struct is a TLS wrapper around a pointer to thread-specific EventPipeBufferList +// The struct wrapper is present mainly because we need a way to free the EventPipeBufferList +// when the thread that owns it dies. Placing this struct as a TLS variable will call ~ThreadEventBufferList() +// when the thread dies so we can free EventPipeBufferList in the destructor. +struct ThreadEventBufferList +{ + Volatile m_pThreadEventBufferList; + + ~ThreadEventBufferList() + { + // Before the thread dies, mark its buffers as no longer owned + // so that they can be cleaned up after the thread dies. + EventPipeBufferList *pList = GetThreadEventBufferList(); + if (pList != NULL) + pList->SetOwnedByThread(false); + } +}; + + + #endif // FEATURE_PERFTRACING #endif // __EVENTPIPE_BUFFERMANAGER_H__ From 4247f7a78b61ccac3f06c59e2cd7118e36c39a61 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 9 Jan 2019 08:01:16 -0800 Subject: [PATCH 10/26] more cleanup --- src/vm/eventpipebuffer.cpp | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index 91b17620ea4b..c300ce423c09 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -68,16 +68,6 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve return false; } - if (pThread == NULL) - { - // For ThreadID just get the current OS thread ID - osThreadId = ::GetCurrentThreadId(); - } - else - { - osThreadId = pThread->GetOSThreadId(); - } - // Calculate the location of the data payload. BYTE *pDataDest = m_pCurrent + sizeof(EventPipeEventInstance); EventPipeEventInstance *pInstance; @@ -93,7 +83,7 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve pInstance = new (m_pCurrent) EventPipeEventInstance( session, event, - osThreadId, + (pThread == NULL) ? ::GetCurrentThreadId() : pThread->GetOSThreadId, pDataDest, payload.GetSize(), (pThread == NULL) ? NULL : pActivityId, From 11f931fabe9d0ddfba36db347be137745950f79b Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 9 Jan 2019 08:23:59 -0800 Subject: [PATCH 11/26] this while loop doesnt do anything now --- src/vm/eventpipebuffermanager.cpp | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 1432e2e33cee..9ff4f393da86 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -54,16 +54,6 @@ EventPipeBufferManager::~EventPipeBufferManager() EventPipeBufferList *pThreadBufferList = pCurElem->GetValue(); if (!pThreadBufferList->OwnedByThread()) { - Thread *pThread = NULL; - while ((pThread = ThreadStore::GetThreadList(pThread)) != NULL) - { - if (pThreadBufferList == GetThreadEventBufferList()) - { - SetThreadEventBufferList(NULL); - break; - } - } - // We don't delete buffers themself because they can be in-use delete(pThreadBufferList); } From 150fb56776d2cfa34a73366679aec886631fa8a6 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 9 Jan 2019 08:26:17 -0800 Subject: [PATCH 12/26] Fix build --- src/vm/eventpipebuffer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index c300ce423c09..d51bb4c0efa1 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -83,7 +83,7 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve pInstance = new (m_pCurrent) EventPipeEventInstance( session, event, - (pThread == NULL) ? ::GetCurrentThreadId() : pThread->GetOSThreadId, + (pThread == NULL) ? ::GetCurrentThreadId() : pThread->GetOSThreadId(), pDataDest, payload.GetSize(), (pThread == NULL) ? NULL : pActivityId, From 3c4f2700cb73c2d5207cf47c00d5a0541c3b4331 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 9 Jan 2019 08:35:34 -0800 Subject: [PATCH 13/26] fixing build --- src/vm/eventpipebuffer.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index d51bb4c0efa1..0504f9e1fc34 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -60,8 +60,7 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve // Calculate the size of the event. unsigned int eventSize = sizeof(EventPipeEventInstance) + payload.GetSize(); - DWORD osThreadId; - + // Make sure we have enough space to write the event. if(m_pCurrent + eventSize >= m_pLimit) { From a18754eeda4971acb2940430a4d58fa4eee5c6e9 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 9 Jan 2019 08:45:42 -0800 Subject: [PATCH 14/26] More cleanup --- src/vm/eventpipebuffermanager.cpp | 27 --------------------------- src/vm/eventpipebuffermanager.h | 3 --- 2 files changed, 30 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 9ff4f393da86..8cebeac7af7b 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -527,33 +527,6 @@ EventPipeEventInstance* EventPipeBufferManager::GetNextEvent() } } -void EventPipeBufferManager::RemovePerThreadBufferListEntry(EventPipeBufferList* listToRemove) -{ - - // Remove the list entry from the per thread buffer list. - SListElem *pElem = m_pPerThreadBufferList->GetHead(); - while(pElem != NULL) - { - EventPipeBufferList* pEntry = pElem->GetValue(); - if(pEntry == listToRemove) - { - pElem = m_pPerThreadBufferList->FindAndRemove(pElem); - - // In DEBUG, make sure that the element was found and removed. - _ASSERTE(pElem != NULL); - - SListElem *pCurElem = pElem; - pElem = m_pPerThreadBufferList->GetNext(pElem); - delete(pCurElem); - } - else - { - pElem = m_pPerThreadBufferList->GetNext(pElem); - } - } -} - - void EventPipeBufferManager::DeAllocateBuffers() { CONTRACTL diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index 28fded83bfe1..f48a93da3ed4 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -87,9 +87,6 @@ class EventPipeBufferManager // Get next event. This is used to dispatch events to EventListener. EventPipeEventInstance* GetNextEvent(); - // Deallocate the given Buffer List entry - void RemovePerThreadBufferListEntry(EventPipeBufferList* listToRemove); - #ifdef _DEBUG bool EnsureConsistency(); #endif // _DEBUG From 884133c739daad6c8cc4f9eda283d1283b75e581 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 9 Jan 2019 08:56:37 -0800 Subject: [PATCH 15/26] more pr comments --- src/vm/eventpipebuffermanager.h | 2 +- src/vm/threads.inl | 12 ------------ 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index f48a93da3ed4..61ddb3e8cec8 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -181,7 +181,7 @@ EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl); // when the thread dies so we can free EventPipeBufferList in the destructor. struct ThreadEventBufferList { - Volatile m_pThreadEventBufferList; + EventPipeBufferList* m_pThreadEventBufferList; ~ThreadEventBufferList() { diff --git a/src/vm/threads.inl b/src/vm/threads.inl index 46429fff9821..2da3c1a3767e 100644 --- a/src/vm/threads.inl +++ b/src/vm/threads.inl @@ -39,18 +39,6 @@ EXTERN_C inline AppDomain* STDCALL GetAppDomain() return AppDomain::GetCurrentDomain(); } -/* -EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList() -{ - return gCurrentThreadInfo.m_pThreadEventBufferList; -} - -EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl) -{ - gCurrentThreadInfo.m_pThreadEventBufferList = bl; -} -*/ - #endif // !DACCESS_COMPILE inline void Thread::IncLockCount() From c7202eb8991cca83ea4dd646d111270987c78920 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 9 Jan 2019 09:28:13 -0800 Subject: [PATCH 16/26] Fix unix build --- src/vm/eventpipebuffermanager.cpp | 19 +++++++++++++++++++ src/vm/eventpipebuffermanager.h | 19 ------------------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 8cebeac7af7b..dff489f6bc55 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -901,6 +901,25 @@ bool EventPipeBufferList::EnsureConsistency() } #endif // _DEBUG +// This struct is a TLS wrapper around a pointer to thread-specific EventPipeBufferList +// The struct wrapper is present mainly because we need a way to free the EventPipeBufferList +// when the thread that owns it dies. Placing this struct as a TLS variable will call ~ThreadEventBufferList() +// when the thread dies so we can free EventPipeBufferList in the destructor. +struct ThreadEventBufferList +{ + EventPipeBufferList* m_pThreadEventBufferList; + + ~ThreadEventBufferList() + { + // Before the thread dies, mark its buffers as no longer owned + // so that they can be cleaned up after the thread dies. + EventPipeBufferList *pList = GetThreadEventBufferList(); + if (pList != NULL) + pList->SetOwnedByThread(false); + } +}; + + extern "C" { #ifndef __llvm__ __declspec(thread) ThreadEventBufferList gCurrentThreadEventBufferList = { NULL }; diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index 61ddb3e8cec8..fc112272647d 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -175,25 +175,6 @@ EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList(); EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl); -// This struct is a TLS wrapper around a pointer to thread-specific EventPipeBufferList -// The struct wrapper is present mainly because we need a way to free the EventPipeBufferList -// when the thread that owns it dies. Placing this struct as a TLS variable will call ~ThreadEventBufferList() -// when the thread dies so we can free EventPipeBufferList in the destructor. -struct ThreadEventBufferList -{ - EventPipeBufferList* m_pThreadEventBufferList; - - ~ThreadEventBufferList() - { - // Before the thread dies, mark its buffers as no longer owned - // so that they can be cleaned up after the thread dies. - EventPipeBufferList *pList = GetThreadEventBufferList(); - if (pList != NULL) - pList->SetOwnedByThread(false); - } -}; - - #endif // FEATURE_PERFTRACING From e0bf8fec23da9d29cd206bd75d777ae332c1b5df Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 9 Jan 2019 10:03:23 -0800 Subject: [PATCH 17/26] more pr comments --- src/vm/eventpipebuffermanager.cpp | 24 ------------------------ src/vm/eventpipebuffermanager.h | 28 ++++++++++++++++++++++------ 2 files changed, 22 insertions(+), 30 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index dff489f6bc55..b516268f01ec 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -800,30 +800,6 @@ EventPipeEventInstance* EventPipeBufferList::PopNextEvent(LARGE_INTEGER beforeTi return pNext; } -bool EventPipeBufferList::OwnedByThread() -{ - LIMITED_METHOD_CONTRACT; - return m_ownedByThread; -} - -void EventPipeBufferList::SetOwnedByThread(bool value) -{ - LIMITED_METHOD_CONTRACT; - m_ownedByThread = value; -} - -bool EventPipeBufferList::GetThreadEventWriteInProgress() -{ - LIMITED_METHOD_CONTRACT; - return m_threadEventWriteInProgress; -} - -void EventPipeBufferList::SetThreadEventWriteInProgress(bool value) -{ - LIMITED_METHOD_CONTRACT; - m_threadEventWriteInProgress = value; -} - #ifdef _DEBUG Thread* EventPipeBufferList::GetThread() { diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index fc112272647d..8dd30bf5fdd4 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -149,17 +149,33 @@ class EventPipeBufferList EventPipeEventInstance* PopNextEvent(LARGE_INTEGER beforeTimeStamp); // True if a thread owns this list. - bool OwnedByThread(); + bool OwnedByThread() const + { + LIMITED_METHOD_CONTRACT; + return m_ownedByThread; + } // Set whether or not this list is owned by a thread. // If it is not owned by a thread, then it can be de-allocated // after the buffer is drained. // The default value is true. - void SetOwnedByThread(bool value); - - bool GetThreadEventWriteInProgress(); - - void SetThreadEventWriteInProgress(bool value); + void SetOwnedByThread(bool value) + { + LIMITED_METHOD_CONTRACT; + m_ownedByThread = value; + } + + bool GetThreadEventWriteInProgress() const + { + LIMITED_METHOD_CONTRACT; + return m_threadEventWriteInProgress; + } + + void SetThreadEventWriteInProgress(bool value) + { + LIMITED_METHOD_CONTRACT; + m_threadEventWriteInProgress = value; + } #ifdef _DEBUG // Get the thread associated with this list. From 917aac3a1c84b858a1737ec82f99936685ac79e5 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Thu, 24 Jan 2019 18:04:58 -0800 Subject: [PATCH 18/26] trying to add a message to assertion that seems to be causing CIs to fail --- src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs b/src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs index d57944e9bf76..b217cc1f47cf 100644 --- a/src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs +++ b/src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs @@ -213,7 +213,7 @@ public static unsafe bool Contains(ref char searchSpace, char value, int length) public static unsafe int IndexOf(ref char searchSpace, char value, int length) { - Debug.Assert(length >= 0); + Debug.Assert(length >= 0, $"length is {length}"); fixed (char* pChars = &searchSpace) { From b982b143f41466d9af9e9ba5e713b85634629f99 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 25 Jan 2019 14:58:49 -0800 Subject: [PATCH 19/26] more pr feedback --- src/vm/eventpipebuffer.cpp | 3 --- src/vm/eventpipebuffermanager.cpp | 8 ++++++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index 0504f9e1fc34..41b5a0d8d5ea 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -74,10 +74,7 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve bool success = true; EX_TRY { - LPCGUID pThreadActivityID = pActivityId; - // Placement-new the EventPipeEventInstance. - // if pthread is NULL, it's likely we are running in something like a GC thread which is not a Thread object, so it can't have an activity ID set anyway pInstance = new (m_pCurrent) EventPipeEventInstance( session, diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index b516268f01ec..5b90c4e973fd 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -352,6 +352,9 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi // Attempt to write the event to the buffer. If this fails, we should allocate a new buffer. allocNewBuffer = !pBuffer->WriteEvent(pEventThread, session, event, payload, pActivityId, pRelatedActivityId, pStack); + + // Mark that the thread is no longer writing an event. + pThreadBufferList->SetThreadEventWriteInProgress(false); } } @@ -379,10 +382,11 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi // The event is still enabled. Mark that the thread is now writing an event. pThreadBufferList->SetThreadEventWriteInProgress(true); allocNewBuffer = !pBuffer->WriteEvent(pEventThread, session, event, payload, pActivityId, pRelatedActivityId, pStack); + + // Mark that the thread is no longer writing an event. + pThreadBufferList->SetThreadEventWriteInProgress(false); } - // Mark that the thread is no longer writing an event. - pThreadBufferList->SetThreadEventWriteInProgress(false); #ifdef _DEBUG if(!allocNewBuffer) From fa2e16a170929ee63f618af4367cbe549db73fd7 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 29 Jan 2019 15:27:32 -0800 Subject: [PATCH 20/26] handle non-2-byte aligned string payloads inside payload buffers --- .../Eventing/EventPipePayloadDecoder.cs | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs index 40269c276599..b2d0fc5738c3 100644 --- a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs +++ b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs @@ -110,16 +110,34 @@ internal static object[] DecodePayload(ref EventSource.EventMetadata metadata, R } else if (parameterType == typeof(string)) { - ReadOnlySpan charPayload = MemoryMarshal.Cast(payload); - int charCount = charPayload.IndexOf('\0'); - if (charCount < 0) + // Try to find null terminator (0x00) from the byte span + int byteCount = -1; + bool sawZero = false; + foreach(byte b in payload) { + if (b == (byte)(0)) + { + if (sawZero) + break; + sawZero = true; + } + else + { + sawZero = false; + } + byteCount++; + } + + ReadOnlySpan charPayload; + if (byteCount < 0) + { + charPayload = MemoryMarshal.Cast(payload); payload = default; } else { - charPayload = charPayload.Slice(0, charCount); - payload = payload.Slice((charCount + 1) * sizeof(char)); + charPayload = MemoryMarshal.Cast(payload.Slice(0, byteCount+1)); + payload = payload.Slice((byteCount / 2 + 1) * sizeof(char)); } decodedFields[i] = BitConverter.IsLittleEndian ? new string(charPayload) : Encoding.Unicode.GetString(MemoryMarshal.Cast(charPayload)); } From 59d4e13f5876ac087fe47e20f46e1ef2e7bcadb8 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 30 Jan 2019 11:33:38 -0800 Subject: [PATCH 21/26] some more cleanup --- .../shared/System/SpanHelpers.Char.cs | 2 +- .../Eventing/EventPipePayloadDecoder.cs | 19 +++++++------------ src/vm/eventpipe.cpp | 2 +- src/vm/eventpipebuffer.cpp | 3 +-- 4 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs b/src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs index b217cc1f47cf..d57944e9bf76 100644 --- a/src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs +++ b/src/System.Private.CoreLib/shared/System/SpanHelpers.Char.cs @@ -213,7 +213,7 @@ public static unsafe bool Contains(ref char searchSpace, char value, int length) public static unsafe int IndexOf(ref char searchSpace, char value, int length) { - Debug.Assert(length >= 0, $"length is {length}"); + Debug.Assert(length >= 0); fixed (char* pChars = &searchSpace) { diff --git a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs index b2d0fc5738c3..2742d9495bde 100644 --- a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs +++ b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs @@ -111,21 +111,16 @@ internal static object[] DecodePayload(ref EventSource.EventMetadata metadata, R else if (parameterType == typeof(string)) { // Try to find null terminator (0x00) from the byte span + // NOTE: we do this by hand instead of using payload.IndexOf('\n') because payload may be unaligned due to + // mixture of different types being stored in the same buffer. (see eventpipe.cpp:CopyData) int byteCount = -1; - bool sawZero = false; - foreach(byte b in payload) + for (int j = 1; j < payload.Length; j+=2) { - if (b == (byte)(0)) + if (payload[j-1] == (byte)(0) && payload[j] == (byte)(0)) { - if (sawZero) - break; - sawZero = true; + byteCount = j; + break; } - else - { - sawZero = false; - } - byteCount++; } ReadOnlySpan charPayload; @@ -136,7 +131,7 @@ internal static object[] DecodePayload(ref EventSource.EventMetadata metadata, R } else { - charPayload = MemoryMarshal.Cast(payload.Slice(0, byteCount+1)); + charPayload = MemoryMarshal.Cast(payload.Slice(0, byteCount)); payload = payload.Slice((byteCount / 2 + 1) * sizeof(char)); } decodedFields[i] = BitConverter.IsLittleEndian ? new string(charPayload) : Encoding.Unicode.GetString(MemoryMarshal.Cast(charPayload)); diff --git a/src/vm/eventpipe.cpp b/src/vm/eventpipe.cpp index 35c64bc5548c..c2db06de8c34 100644 --- a/src/vm/eventpipe.cpp +++ b/src/vm/eventpipe.cpp @@ -776,7 +776,7 @@ void EventPipe::WriteEventInternal(EventPipeEvent &event, EventPipeEventPayload // We're not interested in these events and they can cause corrupted trace files because rundown // events are written synchronously and not under lock. // If we encounter an event that did not originate on the thread that is doing rundown, ignore it. - if(!s_pConfig->IsRundownThread(pThread) || pThread == NULL) + if(pThread == NULL || !s_pConfig->IsRundownThread(pThread)) { return; } diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index 41b5a0d8d5ea..114942779823 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -69,14 +69,13 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve // Calculate the location of the data payload. BYTE *pDataDest = m_pCurrent + sizeof(EventPipeEventInstance); - EventPipeEventInstance *pInstance; bool success = true; EX_TRY { // Placement-new the EventPipeEventInstance. // if pthread is NULL, it's likely we are running in something like a GC thread which is not a Thread object, so it can't have an activity ID set anyway - pInstance = new (m_pCurrent) EventPipeEventInstance( + EventPipeEventInstance *pInstance = new (m_pCurrent) EventPipeEventInstance( session, event, (pThread == NULL) ? ::GetCurrentThreadId() : pThread->GetOSThreadId(), From f4697d5e5488f894603afa75752ff9576d988f03 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Thu, 7 Feb 2019 14:59:34 -0800 Subject: [PATCH 22/26] Fix off by one error in null index calculation --- .../Eventing/EventPipePayloadDecoder.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs index 2742d9495bde..5d7cd77ffde6 100644 --- a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs +++ b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs @@ -113,26 +113,26 @@ internal static object[] DecodePayload(ref EventSource.EventMetadata metadata, R // Try to find null terminator (0x00) from the byte span // NOTE: we do this by hand instead of using payload.IndexOf('\n') because payload may be unaligned due to // mixture of different types being stored in the same buffer. (see eventpipe.cpp:CopyData) - int byteCount = -1; - for (int j = 1; j < payload.Length; j+=2) + int nullByteIdx = -1; + for (int j = 0; j < payload.Length; j+=2) { - if (payload[j-1] == (byte)(0) && payload[j] == (byte)(0)) + if (payload[j] == (byte)(0) && payload[j+1] == (byte)(0)) { - byteCount = j; + nullByteIdx = j; break; } } ReadOnlySpan charPayload; - if (byteCount < 0) + if (nullByteIdx < 0) { charPayload = MemoryMarshal.Cast(payload); payload = default; } else { - charPayload = MemoryMarshal.Cast(payload.Slice(0, byteCount)); - payload = payload.Slice((byteCount / 2 + 1) * sizeof(char)); + charPayload = MemoryMarshal.Cast(payload.Slice(0, nullByteIdx)); + payload = payload.Slice((nullByteIdx / 2) * sizeof(char)); } decodedFields[i] = BitConverter.IsLittleEndian ? new string(charPayload) : Encoding.Unicode.GetString(MemoryMarshal.Cast(charPayload)); } From a0d938f26b2b07c4f70fc7fa9d891468745ed2ea Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 8 Feb 2019 12:16:35 -0800 Subject: [PATCH 23/26] Make Get/SetThreadEventBufferList a static member of ThreadEventBufferList --- src/vm/eventpipebuffermanager.cpp | 57 +++++++++---------------------- src/vm/eventpipebuffermanager.h | 31 ++++++++++++++--- 2 files changed, 43 insertions(+), 45 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 5b90c4e973fd..8acb08a1d915 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -10,6 +10,12 @@ #ifdef FEATURE_PERFTRACING +#ifndef __llvm__ +__declspec(thread) ThreadEventBufferList ThreadEventBufferList::gCurrentThreadEventBufferList = { NULL }; +#else // !__llvm__ +thread_local ThreadEventBufferList ThreadEventBufferList::gCurrentThreadEventBufferList = { NULL }; +#endif // !__llvm__ + EventPipeBufferManager::EventPipeBufferManager() { CONTRACTL @@ -84,7 +90,7 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSessio // Determine if the requesting thread has at least one buffer. // If not, we guarantee that each thread gets at least one (to prevent thrashing when the circular buffer size is too small). bool allocateNewBuffer = false; - EventPipeBufferList *pThreadBufferList = GetThreadEventBufferList(); + EventPipeBufferList *pThreadBufferList = ThreadEventBufferList::Get(); if(pThreadBufferList == NULL) { @@ -101,7 +107,7 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSessio } m_pPerThreadBufferList->InsertTail(pElem); - SetThreadEventBufferList(pThreadBufferList); + ThreadEventBufferList::Set(pThreadBufferList); allocateNewBuffer = true; } @@ -329,7 +335,7 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi bool allocNewBuffer = false; EventPipeBuffer *pBuffer = NULL; - EventPipeBufferList *pThreadBufferList = GetThreadEventBufferList(); + EventPipeBufferList *pThreadBufferList = ThreadEventBufferList::Get(); if(pThreadBufferList == NULL) { @@ -378,7 +384,7 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi if(allocNewBuffer && pBuffer != NULL) { // By this point, a new buffer list has been allocated so we should fetch it again before using it. - pThreadBufferList = GetThreadEventBufferList(); + pThreadBufferList = ThreadEventBufferList::Get(); // The event is still enabled. Mark that the thread is now writing an event. pThreadBufferList->SetThreadEventWriteInProgress(true); allocNewBuffer = !pBuffer->WriteEvent(pEventThread, session, event, payload, pActivityId, pRelatedActivityId, pStack); @@ -881,44 +887,13 @@ bool EventPipeBufferList::EnsureConsistency() } #endif // _DEBUG -// This struct is a TLS wrapper around a pointer to thread-specific EventPipeBufferList -// The struct wrapper is present mainly because we need a way to free the EventPipeBufferList -// when the thread that owns it dies. Placing this struct as a TLS variable will call ~ThreadEventBufferList() -// when the thread dies so we can free EventPipeBufferList in the destructor. -struct ThreadEventBufferList -{ - EventPipeBufferList* m_pThreadEventBufferList; - - ~ThreadEventBufferList() - { - // Before the thread dies, mark its buffers as no longer owned - // so that they can be cleaned up after the thread dies. - EventPipeBufferList *pList = GetThreadEventBufferList(); - if (pList != NULL) - pList->SetOwnedByThread(false); - } -}; - - -extern "C" { -#ifndef __llvm__ -__declspec(thread) ThreadEventBufferList gCurrentThreadEventBufferList = { NULL }; -#else // !__llvm__ -thread_local ThreadEventBufferList gCurrentThreadEventBufferList = { NULL }; -#endif // !__llvm__ -} // extern "C" - - -EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList() +ThreadEventBufferList::~ThreadEventBufferList() { - return gCurrentThreadEventBufferList.m_pThreadEventBufferList; + // Before the thread dies, mark its buffers as no longer owned + // so that they can be cleaned up after the thread dies. + // EventPipeBufferList *pList = GetThreadEventBufferList(); + if (m_pThreadEventBufferList != NULL) + m_pThreadEventBufferList->SetOwnedByThread(false); } -EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl) -{ - gCurrentThreadEventBufferList.m_pThreadEventBufferList = bl; -} - - - #endif // FEATURE_PERFTRACING diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index 8dd30bf5fdd4..8825346f65aa 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -15,6 +15,33 @@ class EventPipeBufferList; +// This class is a TLS wrapper around a pointer to thread-specific EventPipeBufferList +// The struct wrapper is present mainly because we need a way to free the EventPipeBufferList +// when the thread that owns it dies. Placing this class as a TLS variable will call ~ThreadEventBufferList() +// when the thread dies so we can free EventPipeBufferList in the destructor. +class ThreadEventBufferList +{ +public: + +#ifndef __llvm__ +__declspec(thread) static ThreadEventBufferList gCurrentThreadEventBufferList; +#else // !__llvm__ +thread_local static ThreadEventBufferList gCurrentThreadEventBufferList; +#endif // !__llvm__ + EventPipeBufferList * m_pThreadEventBufferList; + ~ThreadEventBufferList(); + + static EventPipeBufferList* Get() + { + return gCurrentThreadEventBufferList.m_pThreadEventBufferList; + } + + static void Set(EventPipeBufferList * bl) + { + gCurrentThreadEventBufferList.m_pThreadEventBufferList = bl; + } +}; + class EventPipeBufferManager { @@ -187,10 +214,6 @@ class EventPipeBufferList #endif // _DEBUG }; -EXTERN_C inline EventPipeBufferList* STDCALL GetThreadEventBufferList(); -EXTERN_C inline void STDCALL SetThreadEventBufferList(EventPipeBufferList* bl); - - #endif // FEATURE_PERFTRACING From b6b2a641a7a170fd44dfd6b089025b329073e988 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 8 Feb 2019 14:54:58 -0800 Subject: [PATCH 24/26] make only the methods public in ThreadEventBufferList --- src/vm/eventpipebuffermanager.cpp | 4 ++-- src/vm/eventpipebuffermanager.h | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/vm/eventpipebuffermanager.cpp b/src/vm/eventpipebuffermanager.cpp index 8acb08a1d915..5355260fa1ce 100644 --- a/src/vm/eventpipebuffermanager.cpp +++ b/src/vm/eventpipebuffermanager.cpp @@ -11,9 +11,9 @@ #ifdef FEATURE_PERFTRACING #ifndef __llvm__ -__declspec(thread) ThreadEventBufferList ThreadEventBufferList::gCurrentThreadEventBufferList = { NULL }; +__declspec(thread) ThreadEventBufferList ThreadEventBufferList::gCurrentThreadEventBufferList; #else // !__llvm__ -thread_local ThreadEventBufferList ThreadEventBufferList::gCurrentThreadEventBufferList = { NULL }; +thread_local ThreadEventBufferList ThreadEventBufferList::gCurrentThreadEventBufferList; #endif // !__llvm__ EventPipeBufferManager::EventPipeBufferManager() diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index 8825346f65aa..386a54b85088 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -21,16 +21,15 @@ class EventPipeBufferList; // when the thread dies so we can free EventPipeBufferList in the destructor. class ThreadEventBufferList { -public: - #ifndef __llvm__ __declspec(thread) static ThreadEventBufferList gCurrentThreadEventBufferList; #else // !__llvm__ thread_local static ThreadEventBufferList gCurrentThreadEventBufferList; #endif // !__llvm__ - EventPipeBufferList * m_pThreadEventBufferList; + EventPipeBufferList * m_pThreadEventBufferList = NULL; ~ThreadEventBufferList(); +public: static EventPipeBufferList* Get() { return gCurrentThreadEventBufferList.m_pThreadEventBufferList; From 4e7eebc67325cf5d8ad6ea0340ec5c35adf85052 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Fri, 8 Feb 2019 16:06:43 -0800 Subject: [PATCH 25/26] Addressing noah's comments --- .../Eventing/EventPipePayloadDecoder.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs index 5d7cd77ffde6..551fdc3a6ffc 100644 --- a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs +++ b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs @@ -113,26 +113,26 @@ internal static object[] DecodePayload(ref EventSource.EventMetadata metadata, R // Try to find null terminator (0x00) from the byte span // NOTE: we do this by hand instead of using payload.IndexOf('\n') because payload may be unaligned due to // mixture of different types being stored in the same buffer. (see eventpipe.cpp:CopyData) - int nullByteIdx = -1; - for (int j = 0; j < payload.Length; j+=2) + int byteCount = -1; + for (int j = 1; j < payload.Length; j+=2) { - if (payload[j] == (byte)(0) && payload[j+1] == (byte)(0)) + if (payload[j-1] == (byte)(0) && payload[j] == (byte)(0)) { - nullByteIdx = j; + byteCount = j+1; break; } } ReadOnlySpan charPayload; - if (nullByteIdx < 0) + if (byteCount < 0) { charPayload = MemoryMarshal.Cast(payload); payload = default; } else { - charPayload = MemoryMarshal.Cast(payload.Slice(0, nullByteIdx)); - payload = payload.Slice((nullByteIdx / 2) * sizeof(char)); + charPayload = MemoryMarshal.Cast(payload.Slice(0, byteCount)); + payload = payload.Slice(byteCount); } decodedFields[i] = BitConverter.IsLittleEndian ? new string(charPayload) : Encoding.Unicode.GetString(MemoryMarshal.Cast(charPayload)); } From 0104e17ae41f7b1f7933d689f35cb997d81ea92e Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Sat, 9 Feb 2019 12:28:34 -0800 Subject: [PATCH 26/26] fix comment and last off by 1 error --- .../System/Diagnostics/Eventing/EventPipePayloadDecoder.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs index 551fdc3a6ffc..1df1de8524cc 100644 --- a/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs +++ b/src/System.Private.CoreLib/src/System/Diagnostics/Eventing/EventPipePayloadDecoder.cs @@ -111,7 +111,7 @@ internal static object[] DecodePayload(ref EventSource.EventMetadata metadata, R else if (parameterType == typeof(string)) { // Try to find null terminator (0x00) from the byte span - // NOTE: we do this by hand instead of using payload.IndexOf('\n') because payload may be unaligned due to + // NOTE: we do this by hand instead of using IndexOf because payload may be unaligned due to // mixture of different types being stored in the same buffer. (see eventpipe.cpp:CopyData) int byteCount = -1; for (int j = 1; j < payload.Length; j+=2) @@ -131,7 +131,7 @@ internal static object[] DecodePayload(ref EventSource.EventMetadata metadata, R } else { - charPayload = MemoryMarshal.Cast(payload.Slice(0, byteCount)); + charPayload = MemoryMarshal.Cast(payload.Slice(0, byteCount-2)); payload = payload.Slice(byteCount); } decodedFields[i] = BitConverter.IsLittleEndian ? new string(charPayload) : Encoding.Unicode.GetString(MemoryMarshal.Cast(charPayload));