-
Notifications
You must be signed in to change notification settings - Fork 548
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
[REVIEW] Fix memory leak and unnecessary allocs in TSNE::get_distances #2542
Conversation
Can one of the admins verify this patch? |
c679194
to
20a0a86
Compare
cpp/src/tsne/distances.cuh
Outdated
std::vector<int> sizes_vec(1); | ||
input_vec.push_back(knn_input[0]); | ||
sizes_vec.push_back(sizes[0]); | ||
// TODO make brute_force_knn take a const float* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a convenience wrapper for the brute force kNN that accepts a single float* and calls the version that accepts multiple. Would you like to do that in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, unclear comment. I was only trying to avoid the const_cast
s here and on line 58R, not the vector
. But it looks like making it const-correct would be hard, so I've removed the comment.
c3ab759 left behind two extra allocations. Those allocations also leaked because `delete knn_input, sizes` only deletes `knn_input`. (It should also be `delete[]`.) Finally, modernizes the vector initialization.
20a0a86
to
b9fa4eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. Thanks for the contribution!
Ok to test |
Just realized that this fixed another bug that caused double the memory usage.
cuml/cpp/src_prims/selection/knn.cuh Lines 263 to 265 in 069a229
The only other place I see that issue is here, but it's in a test, so not necessarily wrong: Lines 70 to 73 in 069a229
|
@zbjornson feel free to make the change to the kNN test in #2548. |
Continuation of rapidsai#2542, see rapidsai#2542 (comment)
c3ab759 left behind two extra allocations. Those allocations also leaked because
delete knn_input, sizes
only deletesknn_input
. (It should also bedelete[]
.)Finally, modernizes the vector initialization syntax.