Skip to content

Add candidate lookup + strategy-hash cache to topology_index#164

Draft
aminaramoon wants to merge 1 commit into
NVIDIA:mainfrom
aminaramoon:feature/topology-index-candidates
Draft

Add candidate lookup + strategy-hash cache to topology_index#164
aminaramoon wants to merge 1 commit into
NVIDIA:mainfrom
aminaramoon:feature/topology-index-candidates

Conversation

@aminaramoon

Copy link
Copy Markdown
Contributor

Summary

Grows topology_index into the place that resolves and memoizes reservation candidates, and wires it into memory_reservation_manager.

  • reservation_request_strategy::hash() — new virtual method (typeid-based default, kept non-pure so downstream custom strategies still compile) overridden by all six strategies to mix their fields; backed by a file-local hash_combine helper.
  • topology_index — new get_spaces_of(tier) (mutable + const overloads), get_candidates(strategy) memoized by strategy hash under a std::shared_mutex (concurrent cache hits, exclusive lock only on insert), and a build(topology, manager) factory. The index keeps a non-owning back-pointer to the manager (documented: the manager must outlive the index). Existing numa_node_of/gpus_of/gpu_ids and both constructors are unchanged, so io_context/prefetching_cache keep compiling.
  • memory_reservation_manager — holds an std::atomic<std::shared_ptr<const topology_index>> with set_/get_topology_index, and routes select_memory_space_and_make_reservation through the memoized candidate path. The index is loaded into a local shared_ptr and pinned for the lifetime of the returned candidate span, so a concurrent set_topology_index() can't free the cache mid-use.

Design notes

  • Candidate sets depend only on the topology (live free-memory is still checked at reservation time), and the manager's memory-space set is immutable after construction — so memoizing candidates by strategy identity is sound.
  • Cache key is the 64-bit strategy.hash(); collisions only cost cache accuracy, not safety.

Testing

New test/memory/test_topology_index.cpp (6 cases, 26 assertions) covering build(), both get_spaces_of overloads, hash() distinctions, candidate memoization (cache-hit returns identical backing storage), the no-manager throw, and end-to-end reservation routing.

  • pixi run build links cleanly.
  • [topology_index] (26 assertions) and [memory_space] (173 assertions) suites pass on an RTX 6000 Ada.
  • clang-format pre-commit hook passes.

🤖 Generated with Claude Code

Grow topology_index into the place that resolves and memoizes reservation
candidates, and wire it into memory_reservation_manager.

- reservation_request_strategy gains a virtual hash() (typeid default, kept
  non-pure for API compat) overridden by all six strategies to mix their
  fields; a file-local hash_combine helper backs them.
- topology_index gains get_spaces_of(tier) (mutable + const overloads),
  get_candidates(strategy) memoized by strategy hash under a shared_mutex
  (concurrent cache hits, exclusive insert only), and a build() factory.
  The index keeps a non-owning back-pointer to the manager.
- memory_reservation_manager holds an atomic shared_ptr<const topology_index>
  with set/get accessors and routes select_memory_space_and_make_reservation
  through the memoized candidate path, pinning the index in a local shared_ptr
  so a concurrent swap cannot free the candidate span mid-use.
- Adds test/memory/test_topology_index.cpp (build, get_spaces_of both
  overloads, hash distinctions, candidate memoization, no-manager throw,
  end-to-end routing).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@aminaramoon
aminaramoon marked this pull request as ready for review July 22, 2026 22:57
@aminaramoon
aminaramoon marked this pull request as draft July 22, 2026 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant