Skip to content

feat: support subregion annotations - #187

Open
cmelone wants to merge 2 commits into
developfrom
subregion-annotate
Open

feat: support subregion annotations#187
cmelone wants to merge 2 commits into
developfrom
subregion-annotate

Conversation

@cmelone

@cmelone cmelone commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Apps that depend on MFEM/Umpire may store fields as aliases inside a large device allocation rather than allocating each field separately.

Mneme currently only supports whole-allocation annotation, which doesn't work for these programs as the fields expose interior pointers inside a larger parent allocation.

Because multiple fields can share the same parent allocation, a mechanism to annotate "subregions" inside allocations will be helpful for apps that follow this pattern (eg, annotating density_ptr and energy_ptr):

mfem::Vector base(total_size);
base.UseDevice(true);

mfem::Vector density;
mfem::Vector energy;
density.MakeRef(base, density_offset, density_size);
energy.MakeRef(base, energy_offset, energy_size);

double *density_ptr = density.ReadWrite();
double *energy_ptr = energy.ReadWrite();

This PR adds support for registering annotations with a byte extent param, and preserves the existing API for annotating whole allocations.

Regarding the registration API, I'd appreciate some design discussion as it's a bit clunky at the moment. For instance, registering an annotation for density_ptr would involve:

mneme::annotate(
    density_ptr,
    static_cast<std::size_t>(density_size) * sizeof(*density_ptr),
    mneme::Metadata{
        .threshold = 1e-12,
        .threshold_kind = mneme::ThresholdKind::Absolute,
        .norm = mneme::Norm::Linf,
        .tag = std::string("density"),
    });

The callers have to calculate the byte extent manually for typed pointers, which we could improve with some convenience abstraction that takes non-void pointers and the element count, but I'm open to people's thoughts.

I also added some logging for when annotations are registered during record/used during replay.

TODO:

  • docs
  • tests

Signed-off-by: Caetano Melone <melone1@llnl.gov>
Signed-off-by: Caetano Melone <melone1@llnl.gov>
@cmelone
cmelone requested a review from ZwFink August 18, 2026 18:52
@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
python/mneme/commands.py 33.33% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@ZwFink ZwFink left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a good change.
I am fine if we introduce some sort of simplified span type that covers some (Base, Extent) with the idea that we later change this to std::span with C++20 support

struct MemoryRegionMetadata {
uint64_t Offset = 0;
uint64_t Extent = 0;
Metadata MD;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Design question: Do we need two distinct metadata types? Both describe some (Base Ptr, Offset, Additional Data). I don't see why we need to special-case here, rather than making the OG implementation more general.

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.

2 participants