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

[ENH] Do not create temporary device memory pools in IVF-Flat, IVF-PQ and balanced k-means #1167

Closed
Nyrio opened this issue Jan 24, 2023 · 1 comment
Labels
0 - Backlog In queue waiting for assignment cpp FAISS improvement Improvement / enhancement to an existing function

Comments

@Nyrio
Copy link
Contributor

Nyrio commented Jan 24, 2023

These memory pools can currently exist concurrently with each other or with a user-defined memory pool, creating problems of both performance and memory limits/control. It should instead be the responsibility of the user to create an appropriate memory pool, and the algorithms should use the resources attached to the raft handle.

Places where memory pools are currently created:

  • cluster::detail::build_hierarchical
  • ivf_flat::detail::search
  • ivf_pq::detail::extend
  • ivf_pq::detail::build
  • ivf_pq::detail::search

Typical code used to create these pools:

auto pool_guard = raft::get_pool_memory_resource(device_memory, 1024 * 1024);
if (pool_guard) {

@cjnolet's message that explains why we want to avoid doing that: #1113 (comment)

Note: once we do this change, we should also update our benchmarks to reflect the typical/proper use of these pools (note that the use of pools in benchmarks can slow down benchmarks significantly though if setup for each instance of the benchmark class, bonus points if we can reuse the pools properly).

cc @achirkin @tfeher

@Nyrio Nyrio added improvement Improvement / enhancement to an existing function 0 - Backlog In queue waiting for assignment cpp labels Jan 24, 2023
@cjnolet cjnolet added the FAISS label Jan 30, 2023
@cjnolet
Copy link
Member

cjnolet commented Jan 16, 2024

This has since been completed. Unfortunately, we are still using managed memory in places on the user's behalf but the usage of pools behind the scenes has been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Backlog In queue waiting for assignment cpp FAISS improvement Improvement / enhancement to an existing function
Projects
Development

No branches or pull requests

2 participants