Repository navigation
Fix memory leaks and an invalid free in the Go Tensor bindings - #2687
maxwbuckley wants to merge 1 commit into
Conversation
Tensor.Close() on a device tensor returned before freeing the DLPack shape and DLManagedTensor, and never destroyed the temporary Resource it created, leaking ~2.4 KB of host memory per call. It also dereferenced a nil C_tensor when called twice. Tensor.ToDevice() overwrote the host data pointer without freeing it, leaking the full host copy of every tensor moved to the GPU. Calling it on a tensor already on the device allocated and leaked a second device buffer. ToDevice and ToHost are now no-ops when the tensor is already in the requested location, and ToHost frees its staging buffer on failure. Tensor.Expand() pointed the DLPack shape at a Go-allocated slice, which violates the cgo pointer rules and made a later Close() pass a Go heap pointer to free(). It now updates the existing C shape array in place. cagra.SearchIndex() built, uploaded and freed a device bitset on every search even when no allow-list was given. The bitset is now only created when filtering. The constructors also free their earlier allocations when the DLManagedTensor allocation fails. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@achirkin any chance you can take a look :) |
|
Gentle bump on this one. @benfred you reviewed my earlier Go-binding PRs (#1471, #1472). Would you be up for taking a look, or pointing me to whoever owns the Go bindings now? The change is confined to It also needs labels ( |
benfred
left a comment
There was a problem hiding this comment.
changes look good to me - but I'm no longer at nvidia, so you'll need to get a review from someone else. Maybe @cjnolet, @achirkin or @divyegala will be able to help you out?
|
Thanks @benfred! @cjnolet @achirkin @divyegala, benfred suggested one of you might be able to review this. It's confined to |
Description
Fixes several memory-management bugs in the Go bindings'
Tensorlifecycle (go/dlpack.go) and incagra.SearchIndex. The changes are confined to the Go bindings, with no API changes.Bugs fixed
Tensor.Close()shapeand theDLManagedTensor, and never destroyed theResourceit created to callcuvsRMMFreeClose(). A secondClose()dereferenced a nilC_tensorand panickedTensor.ToDevice()Tensor.ToDevice()on a tensor already on the devicecudaMemcpyfrom a device pointer (undefined per the CUDA docs), and dropped the old bufferTensor.ToHost()on a tensor already on the hostcuvsRMMFree'd itinvalid argument. Separately, the staging buffer leaked on bothToHosterror pathsTensor.Expand()dl_tensor.shapeat a Go-allocated slicefatal error: unpinned Go pointer stored into non-Go memoryunderGOEXPERIMENT=cgocheck2). A laterClose()then passes a Go heap pointer tofree(): in the newTestExpandRepeatedlythis aborts withmunmap_chunk(): invalid pointercagra.SearchIndex()NewTensor/NewVector/NewTensorOnDeviceDLManagedTensorallocation failedFixes
Close()has one cleanup path for both host and device tensors, closes the temporaryResource, and is a no-op on a closed tensor. If the device free fails, the tensor is left intact and the error is returned.ToDevice()frees the host buffer once the copy succeeds.ToDevice()andToHost()are no-ops when the tensor is already where it's asked to go, which is what the existing CAGRA filtering test already assumes (it callsToDeviceon tensors that are on the device).Expand()updates the existing C shape array in place, so the DLPack struct no longer points into Go memory.cagra.SearchIndex()only builds the bitset whenallowList != nil. An empty non-nil allow-list still means "allow nothing", as before.Measurements
These are end-to-end measurements of the Go bindings on an RTX 5090, linked against the libcuvs 26.12 conda nightly with the CI link flags (
-lcudart_static -ldl -lrt -lcuvs -lcuvs_c). Host memory is glibc heap in use (mallinfo2:uordblks + hblkhd). Device memory is fromcudaMemGetInfo. Each probe ran 5 times after a warmup, and the before/after runs used the same probe program.NewTensorOnDevice+CloseToDevice→CloseToDeviceon a 16 MiB tensor already on the devicecagra.SearchIndexcagra.SearchIndexSearch results are the same before and after: identical neighbour IDs, and data round-trips intact. I'm not claiming any latency change for #6. The saving is the allocations and the copy listed above.
Tests
Three regression tests added to
go/dlpack_test.go, each checked to fail on the old code and pass with this change:TestRedundantTransfersAreNoOps: before,ToHoston a host tensor returnedinvalid argument.TestCloseIsIdempotent(host and device): before, the secondClose()panicked with a nil dereference.TestExpandRepeatedly: before,Close()afterExpandaborted withmunmap_chunk(): invalid pointer, and undercgocheck2theExpanditself was a fatal error.go test ./...passes in all five packages, both normally and withGOEXPERIMENT=cgocheck2. On currentmain, the unmodified suite fails undercgocheck2(in the existingTestDlPack, which callsExpand). After this change the whole suite iscgocheck2-clean, so running the Go tests undercgocheck2in CI would stop this class of bug from coming back. I've left that CI change for a separate PR.Not in this PR
cagra.SearchIndexsizes the allow-list bitset from the largest allowed ID rather than from the dataset's row count. When the largest ID is below the last row, CAGRA reads past the end of the bitset, becausebitset_view::testhas no bounds check. That's a separate correctness fix with its own test, so I'll send it separately.🤖 Generated with Claude Code