From ee8cda063048305e730c92e6899cf0c523fbe483 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Sat, 9 Feb 2019 18:25:10 -0800 Subject: [PATCH] Move eventpipe buffer to TLS (#21817) * start ripping out eventpipe buffer to tls * can now emit events from gc threads * cleanup * more cleanup * more cleanup * tested on linux * Addressing PR comments * Move things around a bit to build in Linux * change eventpipe buffer deallocation code * more cleanup * this while loop doesnt do anything now * Fix build * fixing build * More cleanup * more pr comments * Fix unix build * more pr comments * trying to add a message to assertion that seems to be causing CIs to fail * more pr feedback * handle non-2-byte aligned string payloads inside payload buffers * some more cleanup * Fix off by one error in null index calculation * Make Get/SetThreadEventBufferList a static member of ThreadEventBufferList * make only the methods public in ThreadEventBufferList * Addressing noah's comments * fix comment and last off by 1 error --- .../Eventing/EventPipePayloadDecoder.cs | 23 +++- src/vm/eventpipe.cpp | 12 +- src/vm/eventpipebuffer.cpp | 9 +- src/vm/eventpipebuffermanager.cpp | 142 ++++++--------------- src/vm/eventpipebuffermanager.h | 57 ++++++++- src/vm/threads.cpp | 22 ---- src/vm/threads.h | 29 ----- 7 files changed, 120 insertions(+), 174 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 40269c2..1df1de8 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,29 @@ namespace System.Diagnostics.Tracing } 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 + // 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) { + if (payload[j-1] == (byte)(0) && payload[j] == (byte)(0)) + { + byteCount = j+1; + break; + } + } + + 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-2)); + payload = payload.Slice(byteCount); } 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 66b7c82..c2db06d 100644 --- a/src/vm/eventpipe.cpp +++ b/src/vm/eventpipe.cpp @@ -751,11 +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) { @@ -764,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(); } @@ -780,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(pThread == NULL || !s_pConfig->IsRundownThread(pThread)) { return; } diff --git a/src/vm/eventpipebuffer.cpp b/src/vm/eventpipebuffer.cpp index e7489cd..1149427 100644 --- a/src/vm/eventpipebuffer.cpp +++ b/src/vm/eventpipebuffer.cpp @@ -54,14 +54,13 @@ 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(); - + // Make sure we have enough space to write the event. if(m_pCurrent + eventSize >= m_pLimit) { @@ -75,14 +74,16 @@ bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, Eve 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 EventPipeEventInstance *pInstance = new (m_pCurrent) EventPipeEventInstance( session, event, - pThread->GetOSThreadId(), + (pThread == NULL) ? ::GetCurrentThreadId() : pThread->GetOSThreadId(), pDataDest, payload.GetSize(), - pActivityId, + (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 060707b..5355260 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; +#else // !__llvm__ +thread_local ThreadEventBufferList ThreadEventBufferList::gCurrentThreadEventBufferList; +#endif // !__llvm__ + EventPipeBufferManager::EventPipeBufferManager() { CONTRACTL @@ -54,16 +60,6 @@ EventPipeBufferManager::~EventPipeBufferManager() EventPipeBufferList *pThreadBufferList = pCurElem->GetValue(); if (!pThreadBufferList->OwnedByThread()) { - Thread *pThread = NULL; - while ((pThread = ThreadStore::GetThreadList(pThread)) != NULL) - { - if (pThread->GetEventPipeBufferList() == pThreadBufferList) - { - pThread->SetEventPipeBufferList(NULL); - break; - } - } - // We don't delete buffers themself because they can be in-use delete(pThreadBufferList); } @@ -77,14 +73,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; @@ -95,7 +90,8 @@ 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 = ThreadEventBufferList::Get(); + if(pThreadBufferList == NULL) { pThreadBufferList = new (nothrow) EventPipeBufferList(this); @@ -111,7 +107,7 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeSessio } m_pPerThreadBufferList->InsertTail(pElem); - pThread->SetEventPipeBufferList(pThreadBufferList); + ThreadEventBufferList::Set(pThreadBufferList); allocateNewBuffer = true; } @@ -326,9 +322,6 @@ bool EventPipeBufferManager::WriteEvent(Thread *pThread, EventPipeSession &sessi return false; } - // The event is still enabled. Mark that the thread is now writing an event. - 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 // must make sure that the event is enabled after we mark that we're writing to avoid @@ -341,7 +334,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 = ThreadEventBufferList::Get(); + if(pThreadBufferList == NULL) { allocNewBuffer = true; @@ -358,8 +353,14 @@ 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); + + // Mark that the thread is no longer writing an event. + pThreadBufferList->SetThreadEventWriteInProgress(false); } } @@ -374,7 +375,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. @@ -382,11 +383,16 @@ 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 = 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); + + // Mark that the thread is no longer writing an event. + pThreadBufferList->SetThreadEventWriteInProgress(false); } - // Mark that the thread is no longer writing an event. - pThread->SetEventWriteInProgress(false); #ifdef _DEBUG if(!allocNewBuffer) @@ -543,75 +549,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 = pThread->GetEventPipeBufferList(); - 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()) - { - 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. - pThread->SetEventPipeBufferList(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); @@ -646,9 +583,10 @@ void EventPipeBufferManager::DeAllocateBuffers() { pElem = m_pPerThreadBufferList->GetNext(pElem); } - } + } } + #ifdef _DEBUG bool EventPipeBufferManager::EnsureConsistency() { @@ -678,6 +616,7 @@ EventPipeBufferList::EventPipeBufferList(EventPipeBufferManager *pManager) m_bufferCount = 0; m_pReadBuffer = NULL; m_ownedByThread = true; + m_threadEventWriteInProgress = false; #ifdef _DEBUG m_pCreatingThread = GetThread(); @@ -871,18 +810,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; -} - #ifdef _DEBUG Thread* EventPipeBufferList::GetThread() { @@ -960,4 +887,13 @@ bool EventPipeBufferList::EnsureConsistency() } #endif // _DEBUG +ThreadEventBufferList::~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 (m_pThreadEventBufferList != NULL) + m_pThreadEventBufferList->SetOwnedByThread(false); +} + #endif // FEATURE_PERFTRACING diff --git a/src/vm/eventpipebuffermanager.h b/src/vm/eventpipebuffermanager.h index b72648a..386a54b 100644 --- a/src/vm/eventpipebuffermanager.h +++ b/src/vm/eventpipebuffermanager.h @@ -15,6 +15,32 @@ 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 +{ +#ifndef __llvm__ +__declspec(thread) static ThreadEventBufferList gCurrentThreadEventBufferList; +#else // !__llvm__ +thread_local static ThreadEventBufferList gCurrentThreadEventBufferList; +#endif // !__llvm__ + EventPipeBufferList * m_pThreadEventBufferList = NULL; + ~ThreadEventBufferList(); + +public: + static EventPipeBufferList* Get() + { + return gCurrentThreadEventBufferList.m_pThreadEventBufferList; + } + + static void Set(EventPipeBufferList * bl) + { + gCurrentThreadEventBufferList.m_pThreadEventBufferList = bl; + } +}; + class EventPipeBufferManager { @@ -37,6 +63,7 @@ private: // 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; @@ -50,7 +77,7 @@ private: // 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); @@ -114,6 +141,9 @@ private: // 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. Thread *m_pCreatingThread; @@ -145,13 +175,33 @@ public: 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); + 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. @@ -163,6 +213,7 @@ public: #endif // _DEBUG }; + #endif // FEATURE_PERFTRACING #endif // __EVENTPIPE_BUFFERMANAGER_H__ diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp index f657852..76bce8d 100644 --- a/src/vm/threads.cpp +++ b/src/vm/threads.cpp @@ -897,16 +897,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 = th->GetEventPipeBufferList(); - if(pBufferList != NULL) - { - pBufferList->SetOwnedByThread(false); - } -#endif // FEATURE_PERFTRACING - if (g_fEEShutDown == 0) { th->SetThreadState(Thread::TS_ReportDead); @@ -1020,16 +1010,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 = m_pEventPipeBufferList.Load(); - 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. @@ -1647,8 +1627,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 6ec6323..2fa7569 100644 --- a/src/vm/threads.h +++ b/src/vm/threads.h @@ -4902,11 +4902,6 @@ public: #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; // 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. @@ -4917,30 +4912,6 @@ private: 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 - { - LIMITED_METHOD_CONTRACT; - return m_eventWriteInProgress; - } - - void SetEventWriteInProgress(bool value) - { - LIMITED_METHOD_CONTRACT; - m_eventWriteInProgress = value; - } - bool GetGCModeOnSuspension() { LIMITED_METHOD_CONTRACT; -- 2.7.4