Report actual requested memory on allocation failure; add memory_pool::free() - #174
Open
mohitt31 wants to merge 2 commits into
Open
Report actual requested memory on allocation failure; add memory_pool::free()#174mohitt31 wants to merge 2 commits into
mohitt31 wants to merge 2 commits into
Conversation
…::free() Addresses two of the three asks in eth-cscs#118: the bad_alloc/length_error handlers in memory_pool now report the exact size COSMA tried to allocate (in both human-readable units and element count) instead of a generic message, and memory_pool gains a free() method (plus thin wrappers on cosma_context and a top-level free_memory_pool<Scalar>()) that actually releases the pool's capacity back to the OS, unlike the existing reset() which only marks it logically empty. Adds tests/memory_pool.cpp (non-MPI) and tests/memory_pool_mpi.cpp covering both changes, including a regression test that frees the pool between two independent multiply() calls and checks the result.
Member
|
cscs-ci run GH200 |
Author
|
Hi — just checking in. CI on GH200 looks green. Is there anything you'd like me to change, or is this good to merge as-is? Happy to address any feedback. |
Member
Hi, thanks for the PR. I'm looking at it now. |
Member
|
cscs-ci run GH200 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Report actual requested memory on allocation failure; add a way to release the memory pool
Addresses two of the three asks in #118 ("(Still) Excessive memory usage"):
"Add a hint displaying the actual amount of missing memory in case of
COSMA being able to catch the OOM event" —
memory_pool::resize()andmemory_pool::reserve()already catchbad_alloc/length_errorand printa hint, but the message was generic and didn't say how much memory COSMA
actually tried to allocate. It now reports the exact requested size, in
both human-readable units and raw element count, e.g.:
This doesn't query the OS for how much memory is actually free (that's
platform-specific and unreliable across the batch/container environments
COSMA runs in), but it gives users a concrete number to compare against
what they know is available, rather than a generic message.
"Provide a function to ask COSMA to release its buffers" — the
existing
memory_pool::reset()only marks the pool as logically empty;it never shrinks the underlying
std::vector, so COSMA's peak memorypool capacity stays reserved for the lifetime of the process (relevant
since
cosma_contextis a long-lived Meyer's singleton). Addedmemory_pool::free(), which actually releases the capacity back to theOS via the swap-with-empty-vector idiom, plus a thin
free_memory_pool()wrapper on
cosma_contextand a top-levelcosma::free_memory_pool<Scalar>()convenience function mirroring the existing
get_context_instance<Scalar>()pattern. This lets an application (e.g. one that interleaves COSMA calls
with other memory-hungry libraries) give memory back between
multiplications without tearing down and recreating the whole context.
Important caveat, documented in both docstrings: this is only safe to
call when no
CosmaMatrix/Bufferobjects from a previousmultiply()are still alive, since those cache raw pointers into the pool's backing
storage. It's safe between independent
multiply()/multiply_using_layout()calls (which build fresh matrix objectsinternally each time — the common ScaLAPACK-replacement / pxgemm usage
pattern), but calling it while manually holding onto
CosmaMatrixobjects is a use-after-free. I found this the hard way with a segfault
in local testing before adding the guard comment — worth flagging in
review in case a more defensive runtime check is wanted instead of a
comment.
I did not attempt (1) from the issue, a general explanation of COSMA's
memory scaling behavior, beyond what's now in these two docstrings — that
felt better suited to a wiki/docs page than code, and I didn't want to
guess at wording without maintainer input.
What I found but did not change
While tracing this I noticed a real discrepancy between two memory
estimates:
Strategy::memory_used(the "Required memory per rank" valueprinted to the user, and the value checked against
COSMA_CPU_MAX_MEMORYduring strategy selection) is computed with a buffer-reuse-aware heuristic
(
memory_with_buffer_optimization, which only counts the two largestper-matrix buffers). The memory actually reserved at runtime, in
multiply_using_layout()viaCosmaMatrix::required_memory()→memory_pool::reserve(), is the sum of every buffer allocated acrossthe whole recursion tree for A, B and C combined, further inflated by the
1.2x default
COSMA_MEMORY_POOL_AMORTIZATIONfactor. That reserve calldoesn't account for the LIFO alloc/free pattern the buffers actually
follow during execution. I believe this mismatch is the real reason the
strategy selector's OOM-avoidance check (and the number printed to users)
can look safely under
COSMA_CPU_MAX_MEMORYwhile the process still OOMs— matching what's described in #97 and the
COSMA_ADAPT_STRATEGY=OFFworkaround mentioned in this issue's comments.
I'm flagging this rather than attempting a fix, for two reasons: making
memory_pool::reserve()peak-aware (instead of sum-aware) is a realalgorithmic change to a hot path with correctness risk if I get the
buffer-lifetime accounting wrong, and I have no multi-node/MPI cluster to
validate that a fix actually reduces measured RSS at scale without
regressing performance or correctness. Happy to take a swing at it in a
follow-up if a maintainer confirms this reading of the code is right and
can help validate on a real cluster.
About me / testing scope
First-time contributor here. My background is single-node C++ performance
work (cache-blocking, AVX2 GEMM tiling), not distributed-memory algorithms,
so I want to be upfront about what I could and couldn't verify:
COSMA_SCALAPACK=OFF,no GPU backend) and ran the full existing test suite (
ctest) — all pass,no regressions.
tests/memory_pool.cpp(non-MPIgtest, same pattern as theexisting
test.mapper):free()actually zeroescapacity(), the poolis reusable afterward,
reset()vsfree()behave differently asdocumented, and an intentionally-oversized request throws with the new,
more informative message.
tests/memory_pool_mpi.cpp: runs a fullmultiply()via theexisting
test_cosma()correctness harness, callsfree_memory_pool(),then runs another
multiply()and checks the result against thereference — verified locally at both 4 and 8 (oversubscribed) ranks on
a single machine.
real multi-node job the way the reporters in (Still) Excessive memory usage #118 need, or how it
behaves under a real distributed OOM. This PR is scoped to what I could
responsibly test — it doesn't claim to close (Still) Excessive memory usage #118 on its own.
Fixes part of #118.