Skip to content

Deallocate in BPatch destructor#644

Open
jrood-nrel wants to merge 7 commits intoAMReX-Combustion:developmentfrom
jrood-nrel:jrood/deall
Open

Deallocate in BPatch destructor#644
jrood-nrel wants to merge 7 commits intoAMReX-Combustion:developmentfrom
jrood-nrel:jrood/deall

Conversation

@jrood-nrel
Copy link
Copy Markdown
Contributor

No description provided.

@jrood-nrel jrood-nrel requested a review from baperry2 March 25, 2026 16:05
Copy link
Copy Markdown
Collaborator

@baperry2 baperry2 left a comment

Choose a reason for hiding this comment

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

It does look like this was memory leaking due to not actually deallocating the boundary patch data.

I also removed some unused pointers related to bpatches and updates the test that is supposed to include this capability (the wrong input file had the inputs).

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 updates boundary patch lifetime management so patch-related allocations are cleaned up automatically, and updates the TripleFlame regtest input to exercise boundary-patch mass-fraction flux diagnostics and active control.

Changes:

  • Call BPatch::deallocate() from BPatch’s destructor.
  • Remove unused boundary_patches/boundary_patches_d raw pointers from PeleLMeX.H.
  • Update Exec/RegTests/TripleFlame/tripleflame-2d.inp to enable patch MFR diagnostics and active control parameters.

Reviewed changes

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

File Description
Source/PeleLMeX_BPatch.H Makes BPatch destructor free its host/device allocations via deallocate().
Source/PeleLMeX.H Removes unused raw-pointer members related to boundary patches.
Exec/RegTests/TripleFlame/tripleflame-2d.inp Turns on patch MFR diagnostics and active control for the TripleFlame input.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Source/PeleLMeX_BPatch.H
public:
BPatch() = default;
~BPatch() = default;
~BPatch() { deallocate(); }
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

Adding a destructor that calls deallocate() makes BPatch responsible for freeing raw host/device allocations, but deallocate() does not reset m_host_allocated/m_device_allocated or null out the pointers. If deallocate() is ever called explicitly (or is called twice due to future refactors), the destructor will attempt to free the same pointers again, causing a double-free. Make deallocate() idempotent by clearing the flags and pointers after freeing (and consider guarding frees with nullptr checks as well).

Suggested change
~BPatch() { deallocate(); }
~BPatch() = default;

Copilot uses AI. Check for mistakes.
Comment thread Source/PeleLMeX_BPatch.H
BPatch() = default;
~BPatch() = default;
~BPatch() { deallocate(); }

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

With a user-defined destructor freeing internal raw pointers, the implicitly-generated copy/move constructor and assignment operators will perform shallow copies of m_bpdata_d and the host pointers, leading to double-free/use-after-free if a BPatch is ever copied or moved by value. Explicitly delete copy/move operations, or implement proper move semantics (and deep-copy if copying is intended).

Suggested change
BPatch(const BPatch&) = delete;
BPatch& operator=(const BPatch&) = delete;
BPatch(BPatch&&) = delete;
BPatch& operator=(BPatch&&) = delete;

Copilot uses AI. Check for mistakes.
Comment thread Exec/RegTests/TripleFlame/tripleflame-2d.inp
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