[Fix] Release GEMM cache-flushing buffer on thread exit - #4955
Open
divedb wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
cache_utils.h still isn’t self-contained (uses cudaStream_t/size_t without including the needed headers), and the new .cc test currently depends on include order, which is a fragile build hazard.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a CUDA memory leak in TurboMind’s GEMM tuner by ensuring the thread-local cache-flushing helper releases its per-thread L2-sized device buffer when the worker thread exits, and adds a focused regression test intended to be run under Compute Sanitizer leak checking.
Changes:
- Add a
noexceptdestructor toCacheFlushingand make its CUDA buffer ownership non-copyable/non-movable. - Explicitly initialize buffer-related members to safe defaults.
- Add a standalone
test_cache_flushingexecutable underBUILD_TESTfor leak-regression verification.
File summaries
| File | Description |
|---|---|
| src/turbomind/kernels/gemm/tuner/cache_utils.h | Adds destructor declaration, deletes copy/move, and value-initializes members for safer ownership semantics. |
| src/turbomind/kernels/gemm/tuner/cache_utils.cu | Implements destructor to cudaFree the thread-local buffer on thread exit. |
| src/turbomind/kernels/gemm/test/test_cache_flushing.cc | Adds a regression test exercising TLS reuse/destruction paths across worker threads and main thread. |
| src/turbomind/kernels/gemm/CMakeLists.txt | Adds test_cache_flushing target gated by BUILD_TEST, linking only cudart and threads. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+14
to
+25
| CacheFlushing(); | ||
| ~CacheFlushing() noexcept; | ||
|
|
||
| CacheFlushing(const CacheFlushing&) = delete; | ||
| CacheFlushing& operator=(const CacheFlushing&) = delete; | ||
| CacheFlushing(CacheFlushing&&) = delete; | ||
| CacheFlushing& operator=(CacheFlushing&&) = delete; | ||
|
|
||
| void operator()(cudaStream_t stream) const; | ||
|
|
||
| uint32_t* buffer_; | ||
| size_t size_; | ||
| uint32_t* buffer_{}; | ||
| size_t size_{}; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
CacheFlushingallocates an L2-sized CUDA buffer for each thread that performs GEMM tuning, but the buffer is never freed. When an executor thread exits, its thread-localCacheFlushingobject loses ownership of the allocation, resulting in a memory leak.On an RTX 4080 SUPER, this leaks 64 MiB per thread that initializes the helper.
This PR fixes the lifetime management of the CUDA buffer and adds a regression test to verify that the allocation is released when the thread-local object is destroyed.
Modification
CacheFlushingthat releases the CUDA buffer.flushinterface and same-thread buffer reuse behavior unchanged.test_cache_flushingtarget underBUILD_TEST.The regression test:
Validation was performed with CUDA 13.0.88, an RTX 4080 SUPER, and GCC 13.3.
Before the fix:
After the fix:
The regression test can be built and checked with:
The build must be configured with
BUILD_TEST=ON.Running
test_cache_flushingwithout Compute Sanitizer exercises the relevant lifetime paths, but does not by itself assert leak freedom.BC-breaking (Optional)
No. This change does not modify the public
CacheFlushing::flushinterface or its existing same-thread buffer reuse behavior.Use cases (Optional)
N/A. This PR fixes a memory leak in the GEMM tuning helper and adds regression coverage; it does not introduce a new user-facing feature.
Checklist