Conversation
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.
| 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_{}; |
| @@ -0,0 +1,78 @@ | |||
| // Copyright (c) OpenMMLab. All rights reserved. | |||
There was a problem hiding this comment.
I don't think the test is necessary. Could we remove this file and its CMake target?
| set_property(TARGET gemm2 PROPERTY POSITION_INDEPENDENT_CODE ON) | ||
| set_property(TARGET gemm2 PROPERTY CUDA_RESOLVE_DEVICE_SYMBOLS ON) | ||
|
|
||
| if (BUILD_TEST) |
| #include <cstddef> | ||
| #include <cstdint> | ||
| #include <cuda_runtime_api.h> |
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