diff --git a/src/coreclr/src/vm/eventpipebuffer.cpp b/src/coreclr/src/vm/eventpipebuffer.cpp index eeed5246cd0e81..b1564bb14af38c 100644 --- a/src/coreclr/src/vm/eventpipebuffer.cpp +++ b/src/coreclr/src/vm/eventpipebuffer.cpp @@ -23,8 +23,21 @@ EventPipeBuffer::EventPipeBuffer(unsigned int bufferSize, EventPipeThread* pWrit m_state = EventPipeBufferState::WRITABLE; m_pWriterThread = pWriterThread; m_eventSequenceNumber = eventSequenceNumber; - m_pBuffer = new BYTE[bufferSize]; + // Use ClrVirtualAlloc instead of malloc to allocate buffer to avoid potential internal fragmentation in the native CRT heap. + // (See https://github.com/dotnet/runtime/pull/35924 and https://github.com/microsoft/ApplicationInsights-dotnet/issues/1678 for more details) + // + // This fix does cause a little bit of performance regression (1-2%) in throughput, + // but within acceptable boundaries, while minimizing the risk of the fix to be backported + // to servicing releases. We may come back in the future to reassess this and potentially improve + // the throughput via more performant solution afterwards. + m_pBuffer = (BYTE*)ClrVirtualAlloc(NULL, bufferSize, MEM_COMMIT, PAGE_READWRITE); + + // memset may be unnecessary here because VirtualAlloc with MEM_COMMIT zero-initializes the pages and mmap also zero-initializes + // if MAP_UNINITIALIZED isn't passed (which ClrVirtualAlloc doesn't). If this memset ends up being a perf cost in future investigations + // we may remove this. But for risk mitigation we're leaving it as-is. + // (See https://github.com/dotnet/runtime/pull/35924#discussion_r421282564 for discussion on this) memset(m_pBuffer, 0, bufferSize); + m_pLimit = m_pBuffer + bufferSize; m_pCurrent = GetNextAlignedAddress(m_pBuffer); @@ -47,7 +60,7 @@ EventPipeBuffer::~EventPipeBuffer() } CONTRACTL_END; - delete[] m_pBuffer; + ClrVirtualFree(m_pBuffer, 0, MEM_RELEASE); } bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, EventPipeEvent &event, EventPipeEventPayload &payload, LPCGUID pActivityId, LPCGUID pRelatedActivityId, StackContents *pStack) diff --git a/src/coreclr/src/vm/eventpipebuffermanager.cpp b/src/coreclr/src/vm/eventpipebuffermanager.cpp index 4990c1582dd219..f0f5f453943947 100644 --- a/src/coreclr/src/vm/eventpipebuffermanager.cpp +++ b/src/coreclr/src/vm/eventpipebuffermanager.cpp @@ -168,6 +168,9 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeThread const unsigned int maxBufferSize = 1024 * 1024; bufferSize = Min(bufferSize, maxBufferSize); + // Make the buffer size fit into with pagesize-aligned block, since ClrVirtualAlloc expects page-aligned sizes to be passed as arguments (see ctor of EventPipeBuffer) + bufferSize = (bufferSize + g_SystemInfo.dwAllocationGranularity - 1) & ~static_cast(g_SystemInfo.dwAllocationGranularity - 1); + // EX_TRY is used here as opposed to new (nothrow) because // the constructor also allocates a private buffer, which // could throw, and cannot be easily checked