Fix memory leaks in CHeatSolver by maxaehle · Pull Request #1256 · su2code/SU2

maxaehle · 2021-04-07T10:18:55Z

Proposed Changes

The following members of CHeatSolver were allocated but never freed: Primitive_Flow_i, Primitive_Flow_j, HeatFlux_per_Marker, AverageT_per_Marker, Surface_Areas, ConjugateVar.

I propose to correct this in the spirit of #1225, using containers (std::vector) instead of pointers. There is a bit of additional cleanup.

Related Work

#1225

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

[X ] I am submitting my contribution to the develop branch.
My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
My contribution is commented and consistent with SU2 style.
I have added a test case that demonstrates my contribution, if necessary.
I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

and replace pointers by containers

SU2_CFD/src/solvers/CHeatSolver.cpp

maxaehle · 2021-04-07T10:22:18Z

SU2_CFD/src/solvers/CHeatSolver.cpp

  }

-  SU2_MPI::Allreduce(Local_Surface_Areas, Surface_Areas, config->GetnMarker_HeatFlux(), MPI_DOUBLE, MPI_SUM, SU2_MPI::GetComm());
+  SU2_MPI::Allreduce(Local_Surface_Areas, Surface_Areas.data(), config->GetnMarker_HeatFlux(), MPI_DOUBLE, MPI_SUM, SU2_MPI::GetComm());


Could this be problematic (MPI + containers)?

not that I am aware of :)

There is no problem, memory is memory, but when doing this I prefer to use the .size() of the container for the MPI count rather than reading it from config or somewhere else.

maxaehle · 2021-04-07T10:24:07Z

SU2_CFD/include/solvers/CHeatSolver.hpp

-            *Primitive, *Primitive_Flow_i, *Primitive_Flow_j,
-            *Surface_Areas, Total_HeatFlux_Areas, Total_HeatFlux_Areas_Monitor;
-  su2double ***ConjugateVar, ***InterfaceVar;
+  vector<vector<su2double> > HeatFlux;


Maybe someone could add /*!< \brief comments */.

Hmm yes I can do that, I already worked on the output of the heatsolver ... but I put that on the long bench. I am not sure if all those containers are necessary.
I'll add a commit tomorrow 👍

lgtm-com · 2021-04-07T11:25:40Z

This pull request fixes 6 alerts when merging 84e0585 into e823be7 - view on LGTM.com

fixed alerts:

6 for Resource not released in destructor

TobiKattmann

Thanks for this @maxaehle 💐 lgtm

SU2_CFD/src/solvers/CHeatSolver.cpp

TobiKattmann · 2021-04-07T12:27:23Z

SU2_CFD/src/solvers/CHeatSolver.cpp

  }

-  SU2_MPI::Allreduce(Local_Surface_Areas, Surface_Areas, config->GetnMarker_HeatFlux(), MPI_DOUBLE, MPI_SUM, SU2_MPI::GetComm());
+  SU2_MPI::Allreduce(Local_Surface_Areas, Surface_Areas.data(), config->GetnMarker_HeatFlux(), MPI_DOUBLE, MPI_SUM, SU2_MPI::GetComm());


not that I am aware of :)

TobiKattmann · 2021-04-07T12:32:34Z

SU2_CFD/include/solvers/CHeatSolver.hpp

-            *Primitive, *Primitive_Flow_i, *Primitive_Flow_j,
-            *Surface_Areas, Total_HeatFlux_Areas, Total_HeatFlux_Areas_Monitor;
-  su2double ***ConjugateVar, ***InterfaceVar;
+  vector<vector<su2double> > HeatFlux;


Hmm yes I can do that, I already worked on the output of the heatsolver ... but I put that on the long bench. I am not sure if all those containers are necessary.
I'll add a commit tomorrow 👍

SU2_CFD/src/solvers/CHeatSolver.cpp

Common/include/containers/C2DContainer.hpp

SU2_CFD/src/solvers/CHeatSolver.cpp

lgtm-com · 2021-04-07T16:48:32Z

This pull request fixes 6 alerts when merging dad20d7 into 1f7f6bb - view on LGTM.com

fixed alerts:

6 for Resource not released in destructor

lgtm-com · 2021-04-08T09:41:08Z

This pull request fixes 6 alerts when merging e1896e6 into 1f7f6bb - view on LGTM.com

fixed alerts:

6 for Resource not released in destructor

…cheatsolver

pcarruscag · 2021-04-08T10:54:52Z

Common/include/containers/container_decorators.hpp

+  auto M = N.size();
+  AllocVectorOfVectors(M, N, X, val);


Good idea with the overload 👍

pcarruscag · 2021-04-08T10:59:11Z

Common/include/containers/container_decorators.hpp

+  X.resize(M);
+  for(size_t i = 0; i < M; ++i){
+    X[i].resize(N[i]);
+    for (auto& x : X[i]) x = val;


I changed it a bit to try to make it compatible with more types. If I broke anything I'll fix it.
Thanks for all the cleanup.

lgtm-com · 2021-04-08T11:54:55Z

This pull request fixes 6 alerts when merging 026fbff into 1f7f6bb - view on LGTM.com

fixed alerts:

6 for Resource not released in destructor

lgtm-com · 2021-04-14T15:07:10Z

This pull request fixes 6 alerts when merging 0e250e1 into 7fa0763 - view on LGTM.com

fixed alerts:

6 for Resource not released in destructor

maxaehle added 2 commits April 7, 2021 09:24

Fix memory leaks in CHeatSolver

3771900

and replace pointers by containers

delete[] Marker_CHTInterface;

e3f878a

pr-triage bot added the PR: unreviewed label Apr 7, 2021

maxaehle commented Apr 7, 2021

View reviewed changes

unused variables removed

84e0585

maxaehle changed the title ~~Fix memoryleaks cheatsolver~~ Fix memory leaks in CHeatSolver Apr 7, 2021

TobiKattmann approved these changes Apr 7, 2021

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Apr 7, 2021

maxaehle added 2 commits April 7, 2021 17:13

declaration of local su2double* Normal shifted

9f78e46

Move Alloc2D/3D to namespace container_helpers

3f65e8e

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Apr 7, 2021

extra ';'

dad20d7

pcarruscag reviewed Apr 7, 2021

View reviewed changes

Common/include/containers/C2DContainer.hpp Outdated Show resolved Hide resolved

pcarruscag reviewed Apr 7, 2021

View reviewed changes

Common/include/containers/C2DContainer.hpp Outdated Show resolved Hide resolved

pcarruscag reviewed Apr 7, 2021

View reviewed changes

Common/include/containers/C2DContainer.hpp Outdated Show resolved Hide resolved

pcarruscag reviewed Apr 7, 2021

View reviewed changes

SU2_CFD/src/solvers/CHeatSolver.cpp Outdated Show resolved Hide resolved

maxaehle added 3 commits April 8, 2021 10:29

Changes proposed by @pcarruscag

f32f9ee

Alloc2D->AllocVectorOfVectors, Alloc3D->AllocVectorOfMatrices

daad1d0

Use AllocVectorOf* in CFVMFlowSolverBase as well

e1896e6

pcarruscag added 2 commits April 8, 2021 11:16

Merge remote-tracking branch 'upstream/develop' into fix_memoryleaks_…

64b33d1

…cheatsolver

a little more generic

8042ff1

pcarruscag reviewed Apr 8, 2021

View reviewed changes

pcarruscag approved these changes Apr 8, 2021

View reviewed changes

pr-triage bot removed the PR: unreviewed label Apr 8, 2021

pr-triage bot added the PR: reviewed-approved label Apr 8, 2021

pcarruscag reviewed Apr 8, 2021

View reviewed changes

Derive -> Deduce

026fbff

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Apr 8, 2021

Merge branch 'develop' into fix_memoryleaks_cheatsolver

0e250e1

maxaehle merged commit 13c9943 into develop Apr 14, 2021

maxaehle deleted the fix_memoryleaks_cheatsolver branch April 14, 2021 15:14

pr-triage bot added PR: merged and removed PR: unreviewed labels Apr 14, 2021

TobiKattmann added the changelog:fix label Apr 28, 2021

TobiKattmann mentioned this pull request May 8, 2021

Chore in CHeatSolver.cpp #1281

Merged

5 tasks

Conversation

maxaehle commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Related Work

PR Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Apr 7, 2021

Uh oh!

TobiKattmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Apr 7, 2021

Uh oh!

lgtm-com bot commented Apr 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Apr 8, 2021

Uh oh!

lgtm-com bot commented Apr 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maxaehle commented Apr 7, 2021 •

edited

Loading