Skip to content

fix(benches): 🐛 measure peak memory with the kernel high-water mark - #205

Merged
robertodr merged 7 commits into
mainfrom
fix/bench-memory-high-water-mark
Aug 5, 2026
Merged

fix(benches): 🐛 measure peak memory with the kernel high-water mark#205
robertodr merged 7 commits into
mainfrom
fix/bench-memory-high-water-mark

Conversation

@Panadestein

@Panadestein Panadestein commented Aug 5, 2026

Copy link
Copy Markdown
Member

🤖 AI text below 🤖

Description

benches/_memory.py measured peak memory by sampling RSS from a background thread and keeping the maximum. monoprop's nanobind bindings never release the GIL, so that thread cannot run while the operation it is meant to measure is running.

What's new

Peak memory now comes from the kernel's own high-water mark:

  • VmHWM in /proc/self/status — the kernel updates it on every RSS increase, so it cannot miss a transient, and no thread is involved.
  • Reset per measurement window by writing mode 5 (CLEAR_REFS_MM_HIWATER_RSS) to /proc/self/clear_refs. This is a counter reset, not a page-table walk, so it is cheap (27 µs per read, vs 41 µs for the RSS read it replaces).

HighWaterMark also records the settled floor (gc + heap trim) before opening the window, so callers can report growth rather than absolute footprint. It degrades gracefully: if clear_refs is not writable, exact is False and it falls back to current RSS.

MPI

A sampler is still needed under MPI: no per-rank scalar can recover which ranks peaked at the same moment, which is what peak-of-sum needs. So RssSampler is kept, renamed PssSampler, and now samples PSS instead of RSS — summing RSS across ranks double-counted shared pages.

Neither metric is simultaneously exact and MPI-aware, so conftest.py records both (memhwm and mem) and report.py presents the exact one.

The benchmark suite sampled RSS from a background thread and kept the maximum
seen. monoprop's nanobind bindings never release the GIL, so that thread is
frozen for the entire duration of the call it is meant to measure: a 5.96 s
propagate() step allowed a 1 ms sampler exactly 2 polls instead of ~5956.

Peak memory now comes from the kernel's own high-water mark, VmHWM in
/proc/self/status, reset per measurement window by writing mode 5
(CLEAR_REFS_MM_HIWATER_RSS) to /proc/self/clear_refs. The kernel updates it on
every RSS increase, so it cannot miss a transient, and no thread is involved.
HighWaterMark also records the settled floor before the window opens, so a
caller can report growth rather than absolute footprint.

The undercount was large and, worse, erratic: with only two polls per step one
lands on the reset, so the sampler occasionally reported the correct figure by
coincidence. On the random Schrodinger benchmarks the true peaks are 3.4x the
sampled ones (build_graph 993 -> 3395 MiB, inplace 1712 -> 5520 MiB).

A sampler is still needed under MPI, where no per-rank scalar can recover which
ranks peaked at the same moment, so PssSampler is kept for the peak-of-sum path
and now samples PSS instead of RSS: summing RSS across ranks double-counted
shared pages. conftest records both metrics, since neither is simultaneously
exact and MPI-aware, and report.py presents the exact one.

The comment claiming monoprop releases the GIL was false and is corrected; it
is the likely reason the sampler was never re-examined.

Assisted-by: GitHubCopilot:claude-opus-5
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Docs preview: https://pr-205.monoprop-docs.pages.dev

@Panadestein
Panadestein marked this pull request as ready for review August 5, 2026 15:28
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.77%. Comparing base (58d1498) to head (27b9964).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #205   +/-   ##
=======================================
  Coverage   94.77%   94.77%           
=======================================
  Files          14       14           
  Lines         708      708           
  Branches       90       90           
=======================================
  Hits          671      671           
  Misses         22       22           
  Partials       15       15           
Flag Coverage Δ
cpp 94.77% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@robertodr robertodr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks @Panadestein

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes benchmark peak-memory measurement by switching from a background-thread RSS sampler (blocked by the GIL in nanobind calls) to the kernel’s RSS high-water mark (VmHWM) with per-operation window resets, while retaining a sampled timeline metric for MPI peak-of-sum estimation.

Changes:

  • Added HighWaterMark (kernel VmHWM + /proc/self/clear_refs reset) and updated benchmarks to record memhwm.
  • Renamed the MPI sampler to PssSampler and switched timeline sampling from RSS to PSS for more meaningful across-rank aggregation.
  • Updated report rendering and unit tests to reflect the new memory metric naming/semantics.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
benches/_memory.py Introduces HighWaterMark, adds PSS sampling + helpers, and updates peak-of-sum merge documentation.
benches/conftest.py Records both per-rank kernel peak (memhwm) and MPI peak-of-sum timeline (mem).
benches/report.py Switches report memory section to memhwm and updates wording/title.
tests/test_bench_memory.py Adds coverage for PSS properties and HighWaterMark window/reset behavior.
tests/test_bench_report.py Updates report assertions to the new memhwm section and label text.
Suppressed comments (2)

benches/_memory.py:207

  • Same monotonicity issue as in _run: the entry sample should also avoid producing a timestamp earlier than any existing samples (future refactors could add samples before __enter__, and it keeps the invariant explicit).
    def __enter__(self) -> Self:
        self._samples.append((time.time(), pss_bytes()))
        self._thread.start()
        return self

benches/_memory.py:217

  • Same monotonicity issue as in _run: the final sample on exit should be clamped to >= the last background-thread sample, otherwise a backwards wall-clock adjustment between the last thread sample and __exit__ can make the timeline non-ordered.
        self._stop.set()
        self._thread.join()
        self._samples.append((time.time(), pss_bytes()))

Comment thread benches/report.py
Comment thread benches/conftest.py Outdated
Comment thread benches/_memory.py
robertodr
robertodr previously approved these changes Aug 5, 2026
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
robertodr
robertodr previously approved these changes Aug 5, 2026
@robertodr
robertodr enabled auto-merge (squash) August 5, 2026 19:20
Comment thread benches/conftest.py Outdated
Co-authored-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
@robertodr
robertodr merged commit ae2f4b2 into main Aug 5, 2026
22 checks passed
@robertodr
robertodr deleted the fix/bench-memory-high-water-mark branch August 5, 2026 19:38
@sonarqubecloud

sonarqubecloud Bot commented Aug 5, 2026

Copy link
Copy Markdown

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.

3 participants