Require explicit memory management from the user and remove sloppy memory management. by andrewxu319 · Pull Request #436 · ORNL/ReSolve

andrewxu319 · 2026-06-09T18:40:46Z

Description

Remove automatic memory allocation and memory syncing in various memory management functions and require the user to explicitly manage their own memory.
Resolves #295.

Proposed changes

Major changes:

Assert that data is updated in Vector::getData() and remove automatic synchronization
Assert that data is allocated in copyFromExternal() and remove automatic allocation for vectors and matrices
Assert that data is not already allocated in allocate() and allocateMatrixData() for vectors and matrices and remove auto deallocation
Add isUpdated(), isAllocated(), allocateAll() to Vector
Add allocateAll() to Sparse
Update all parts of the code that call these functions and are affected by these changes such that they work correctly

Minor changes:

Update .gitignore to ignore test outputs
Remove HyKKTSolver::readMatrixFiles() which is unnecessary

Checklist

Further comments

I also tried to add status returns to a few functions in the VectorHandler and LinAlgWorkspace series of classes, but that crashes the HIP implementation for seemingly unknown reasons.

…a simple test.

…ale tests to fail. Made "createIncrementingVector()" return a pointer to the new vector instead of a copy (which causes the data to be freed prematurely).

…arse" This reverts commit 00f6992. Kept changes to "io.cpp", which fixes the an issue with out-of-bounds iterators.

No commented out code.

…HyKKT implementation (if matvec works correctly).

…ed right now, but good to prevent stack overflows.

… doesn't work because MatrixHandler is broken (specifically matvec).

* use caller-provided backend-specific handlers in SCCG * load SCCG data on host before device sync * refresh CUDA SpMV cache when matrix changes * fix HyKKT SCCG HIP target setup * document SCCG backend setup and CUDA SpMV cache handling * mirror SpMV cache reset for HIP * Shaked/random error fix (#424) Fixed stochastic failures due to a faulty random number generator and asynchronous operations. Co-authored-by: shakedregev <shakedregev@users.noreply.github.com> --------- Co-authored-by: tamar-dewilde <tamar-dewilde@users.noreply.github.com> Co-authored-by: Shaked Regev <35384901+shakedregev@users.noreply.github.com> Co-authored-by: shakedregev <shakedregev@users.noreply.github.com>

…e codebase.

shakedregev · 2026-06-09T19:16:55Z

        for (real_type val = 0.0; val <= 1.0; val += 1.0)
        { // Use a step to prevent infinite loop


Not really part of this PR, but it's not clear to me why we have a loop at all here. It's simply the first thing happening, then the second.

Fixed in the latest commit

shakedregev

Very good work! Tests pass on all platforms. See:

Some minor documentation comments.
Some very weird code that was not part of this PR. Should be fixed now or later.

superwhiskers

some minor comments.

i ran the cpu-only tests on my local machine, all cpu tests passed. one warning was not addressed:

ReSolve/examples/sysRefactor.cpp:33:43: warning: unused parameter ‘hw_backend’ [-Wunused-parameter]
   33 | static void syncDevice(const std::string& hw_backend)
      |                        ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~

other than that, it seems fine. this warning may be an artifact of a cpu-only build. the gpu build tests are taking a little longer, so i'll leave this review without them.

andrewxu319 · 2026-06-09T19:37:16Z

some minor comments.

i ran the cpu-only tests on my local machine, all cpu tests passed. one warning was not addressed:
ReSolve/examples/sysRefactor.cpp:33:43: warning: unused parameter ‘hw_backend’ [-Wunused-parameter]
   33 | static void syncDevice(const std::string& hw_backend)
      |                        ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
other than that, it seems fine. this warning may be an artifact of a cpu-only build. the gpu build tests are taking a little longer, so i'll leave this review without them.

The full function is

static void syncDevice(const std::string& hw_backend)
{
#ifdef RESOLVE_USE_CUDA
  if (hw_backend == "CUDA")
  {
    cudaDeviceSynchronize();
  }
#endif

#ifdef RESOLVE_USE_HIP
  if (hw_backend == "HIP")
  {
    hipDeviceSynchronize();
  }
#endif
}

So the variable is used if either CUDA or HIP is enabled.

@shakedregev Would it be a good idea to add a [[maybe_unused]]? Or just leave it be?

shakedregev · 2026-06-09T20:13:24Z

some minor comments.
i ran the cpu-only tests on my local machine, all cpu tests passed. one warning was not addressed:
ReSolve/examples/sysRefactor.cpp:33:43: warning: unused parameter ‘hw_backend’ [-Wunused-parameter]
   33 | static void syncDevice(const std::string& hw_backend)
      |                        ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
other than that, it seems fine. this warning may be an artifact of a cpu-only build. the gpu build tests are taking a little longer, so i'll leave this review without them.
The full function is
static void syncDevice(const std::string& hw_backend)
{
#ifdef RESOLVE_USE_CUDA
  if (hw_backend == "CUDA")
  {
    cudaDeviceSynchronize();
  }
#endif

#ifdef RESOLVE_USE_HIP
  if (hw_backend == "HIP")
  {
    hipDeviceSynchronize();
  }
#endif
}
So the variable is used if either CUDA or HIP is enabled.

@shakedregev Would it be a good idea to add a [[maybe_unused]]? Or just leave it be?

It makes more sense to me to call the function only if you need it. HIP/CUDA hw_backend, call this. CPU don't. Anything else should probably throw an error.

superwhiskers · 2026-06-09T20:24:11Z

managed to complete the rocm-enabled tests on my local machine after instituting a workaround for a rocm bug related to my specific amd gpu. builds and runs fine. no leaks outside those present within rocm, runs fine.

there is one concerning thing, and that is the flakiness of two tests on my machine:

sys_refactor_hip_asym_test
sys_refactor_hip_test

primarily the former. this most likely is not related to this pull request, though. just noting this for future reference. it's probably a tolerance thing, unlikely to be anything significant.

superwhiskers

still getting this warning on a cpu-only build.

ReSolve/examples/sysRefactor.cpp:33:43: warning: unused parameter ‘hw_backend’ [-Wunused-parameter]
   33 | static void syncDevice(const std::string& hw_backend)
      |                        ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~

everything else looks fine.

superseded

shakedregev

All tests pass and concerns addressed.

andrewxu319 and others added 30 commits June 4, 2026 16:59

Create Matrix class, a parent class for matrices including Sparse

7990197

Added CPU implementation for Schur Complement Conjugate Gradient and …

37c8ccf

…a simple test.

Fixed bug in MatrixHandlerTests.hpp that caused leftScale and rightSc…

08d691c

…ale tests to fail. Made "createIncrementingVector()" return a pointer to the new vector instead of a copy (which causes the data to be freed prematurely).

Add to CHANGELOG.md. Removed some unnecessary comments.

7868e59

Bug fixes (missing <cmath>, type mismatch of variable "i").

8d3a3d8

Revert "Create Matrix class, a parent class for matrices including Sp…

1bf3553

…arse" This reverts commit 00f6992. Kept changes to "io.cpp", which fixes the an issue with out-of-bounds iterators.

Apply pre-commmit fixes

d025508

Update tests/unit/hykkt/HykktSCCGTests.hpp

276571c

No commented out code.

Apply pre-commmit fixes

b2d0d13

Formatting fixes.

2bc2d4a

Apply pre-commmit fixes

2d090b6

Removed "Matrix.hpp" from resolve/matrix/CMakeLists.txt

78b532e

Apply pre-commmit fixes

a2af9f3

Apply suggestion from @shakedregev

b3eed10

fixed not building test, test still fails on HIP

df856c0

Fix SCCG vector initialization values. The outputs now match the old …

37d6f1f

…HyKKT implementation (if matvec works correctly).

Put SCCG's internal vectors on the heap via malloc. Not strictly need…

55d9318

…ed right now, but good to prevent stack overflows.

Create SCCG unit tests (one set of inputs). NOTE: this code currently…

9e9bdd6

… doesn't work because MatrixHandler is broken (specifically matvec).

Address SCCG GPU review comments

e6eba9d

Add comments to edited sections of removeDuplicates.

5fffaee

Standardize matrix/vector handler object use and naming.

3974c6c

Apply pre-commmit fixes

c25cd6f

Make matrix and vector handlers passed to SCCG instead of owned by it

85cfd95

Pass handlers by pointer. This is more consistent with the rest of th…

1d75e64

…e codebase.

Formatting fixes as suggested in PR.

b7c09b0

Apply pre-commmit fixes

2d44fb0

Reordered variable declaration in SCCG to avoid [-Wreorder] warning

c98b838

SCCG test no longer needs a random number generator

9c16eff

Apply pre-commmit fixes

84d3063

shakedregev reviewed Jun 9, 2026

View reviewed changes

shakedregev approved these changes Jun 9, 2026

View reviewed changes

superwhiskers reviewed Jun 9, 2026

View reviewed changes

Comment thread resolve/matrix/Coo.cpp

Comment thread resolve/vector/Vector.cpp Outdated

Comment thread resolve/workspace/LinAlgWorkspaceCUDA.cpp Outdated

andrewxu319 and others added 2 commits June 9, 2026 15:27

Simplify logic in MatrixHandlerTests::transpose()

46f72fc

Apply pre-commmit fixes

d6cecbe

andrewxu319 and others added 6 commits June 9, 2026 19:41

Change load-bearing assert() to out::error()

64d2415

Return after out::error()'s

8795970

Add Logger includes for error logging

0a103e3

Apply pre-commmit fixes

3aa0f11

Clarify comment in HyKKTSolver

683af6a

Remove confusing comment

2c28a56

andrewxu319 and others added 5 commits June 9, 2026 16:24

Call syncDevice() in sysRefactor only when GPU is enabled

a2029ed

Apply pre-commmit fixes

a2fd2a6

Add return statements after errors in Vector

a0e50c1

Add more return statements after errors.

8cfd2ca

Apply pre-commmit fixes

35ff137

superwhiskers previously requested changes Jun 9, 2026

View reviewed changes

Comment thread examples/sysRefactor.cpp Outdated

Comment thread examples/sysRefactor.cpp Outdated

Comment thread resolve/workspace/LinAlgWorkspaceCUDA.cpp

andrewxu319 added 2 commits June 10, 2026 13:18

Fix syntax error in sysRefactor

376e146

Fix logic error in LinAlgWorkspaceCUDA/HIP

b8d573c

superwhiskers reviewed Jun 10, 2026

View reviewed changes

shakedregev requested a review from superwhiskers June 10, 2026 13:55

shakedregev approved these changes Jun 10, 2026

View reviewed changes

shakedregev merged commit a93fb65 into develop Jun 10, 2026
6 checks passed

andrewxu319 deleted the andrew/strict-mem-management branch June 10, 2026 14:57

TamarDeWilde mentioned this pull request Jun 19, 2026

Bug: ReadTheDocs docs build failing due to relative path in conf.py #437

Closed

tamar-dewilde mentioned this pull request Jun 19, 2026

Bug: ReadTheDocs docs build failing due to relative path in conf.py #438

Open

		for (real_type val = 0.0; val <= 1.0; val += 1.0)
		{ // Use a step to prevent infinite loop

Uh oh!

Conversation

andrewxu319 commented Jun 9, 2026

Description

Proposed changes

Checklist

Further comments

Uh oh!

shakedregev Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

shakedregev Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

andrewxu319 Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

shakedregev left a comment

Choose a reason for hiding this comment

Uh oh!

superwhiskers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewxu319 commented Jun 9, 2026

Uh oh!

shakedregev commented Jun 9, 2026

Uh oh!

superwhiskers commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

superwhiskers left a comment

Choose a reason for hiding this comment

Uh oh!

shakedregev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

superwhiskers commented Jun 9, 2026 •

edited

Loading