Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix StandardGpuResources memory leak #81

Merged
merged 1 commit into from
Apr 1, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/gpu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,14 @@ impl StandardGpuResources {
}
}

impl Drop for StandardGpuResources {
fn drop(&mut self) {
unsafe {
faiss_StandardGpuResources_free(self.inner);
}
}
}

impl GpuResourcesProvider for StandardGpuResources {
fn inner_ptr(&self) -> *mut FaissGpuResourcesProvider {
self.inner as *mut _
Expand Down Expand Up @@ -169,4 +177,20 @@ mod tests {
fn smoke_detector() {
StandardGpuResources::new().unwrap();
}

// The test marked as ignored because it takes a significant amount of time.
#[ignore]
#[test]
fn resources_leak() {
use crate::{index_factory, MetricType};

// Try to allocate gpu resources multiple times in a loop.
// If resources are not properly deallocated this will cause out-of-memory error.
for _ in 0..50 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going up to 16 was enough for my machine, but I guess it depends on how much temporary memory is allocated per instance. Maybe one could raise the temporary memory allocated so that it fails faster, but the worth of doing this, which is checking that the new Drop impl is called, is too low to block this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test can be considered just as reproducer of the issue. It should not be run every time, or maybe it should be removed at all because it consumes a lot of resources (all GPU memory) just to ensure that destructor is called.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go ahead and merge as-is, because there is not that much mental overhead in having an ignored test.

let res = StandardGpuResources::new().unwrap();
// We need to create an index because `StandardGpuResources` constructor does not allocate actual resources.
let index = index_factory(32, "Flat", MetricType::InnerProduct).unwrap();
let _gpu_index = index.into_gpu(&res, 0).unwrap();
}
}
}
Loading