Fix a potential memory leak due to EventPipe buffer allocation #35924
Conversation
| @@ -23,7 +23,7 @@ EventPipeBuffer::EventPipeBuffer(unsigned int bufferSize, EventPipeThread* pWrit | |||
| m_state = EventPipeBufferState::WRITABLE; | |||
| m_pWriterThread = pWriterThread; | |||
| m_eventSequenceNumber = eventSequenceNumber; | |||
| m_pBuffer = new BYTE[bufferSize]; | |||
| m_pBuffer = (BYTE*)ClrVirtualAlloc(NULL, bufferSize, MEM_COMMIT, PAGE_READWRITE); | |||
| memset(m_pBuffer, 0, bufferSize); | |||
There was a problem hiding this comment.
I think we can skip the memset now since you used the MEM_COMMIT flag here:
The function also guarantees that when the caller later initially accesses the memory, the contents will be zero.
-docs
There was a problem hiding this comment.
Isn't that doc for the Windows VirtualAlloc API? I don't think the PAL implementation memsets it to 0 by default.
There was a problem hiding this comment.
runtime/src/coreclr/src/pal/src/map/virtual.cpp
Lines 1358 to 1362 in 84ec25f
These comments seem to indicate it is supported, but I'll double check that we actually mimic that behavior
There was a problem hiding this comment.
Based on mmap docs and our code not passing MAP_UNINITIALIZED flag to mmap I would assume the memory we get back from VirtualAlloc is zero initialized. However I'd recommend only adding a comment to the memset referencing this discussion rather than removing it. We'd like to take this into servicing which has low tolerance for risk and we've already benchmarked the current code as being good enough as-is. In the future we might do some performance measurements to improve the throughput of EventPipe - if the memset shows up in that analysis we can address it then, if not we've saved time worrying about it.
ghost
locked as resolved and limited conversation to collaborators
This fixes a potential memory leak due to EventPipe buffer allocation causing internal fragmentation within glibc's arenas. It was originally reported in microsoft/ApplicationInsights-dotnet#1678. glibc maintains a pool of memory called arenas per-thread that it uses to allocate blocks of memory requested by
malloc. EventPipe may allocate from many threads, and when it does that, it end up causing glibc to commit more memory than what the runtime thinks it committed.The fix is quite simple - change
malloctoClrVirtualAllocwhich usesmmap. I verified the fix does not introduce a significant performant with benchmarks.The fix was tested against the repro provided by the customer who reported the issue, and was verified that it fixes the issue (more details here: microsoft/ApplicationInsights-dotnet#1678 (comment)).