Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SU2-NEMO - Optimize initialization time #1340

Merged
merged 5 commits into from
Jul 31, 2021
Merged

SU2-NEMO - Optimize initialization time #1340

merged 5 commits into from
Jul 31, 2021

Conversation

fmpmorgado
Copy link
Contributor

@fmpmorgado fmpmorgado commented Jul 30, 2021

Proposed Changes

Optimize the SU2-NEMO solver initialization time

Related Work

None that I remember

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.
  • [ X ] 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).
  • [ X ] My contribution is commented and consistent with SU2 style.
  • [ X ] I have added a test case that demonstrates my contribution, if necessary.
  • [ X ] I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

Work Overview

I have optimized the initialization time of SU2-NEMO. You can check the time comparison between the old and new version at the table below.

image

Some steps that I have done:

CNEMOEulerVariable.cpp

  • Removed constant calculations inside a for loop
  • Passed the Solution_Old = Solution to outside the loop

CNEMOEulerSolver.cpp

  • Removed the "for" loop that fills the Nodes Primitive Fields (already done in the Preprocessing phase)
  • Removed the error counter (already done in the Preprocessing phase)

CSolverFactory.cpp

  • Removed the Preprocessing step after the CNEMOEulerSolver instantiation (This is done before the start of the iterative process, regardless. Additionally, the Navier-Stokes do not have any preprocess phase after the instantiation, why is that ? )

The residuals may slightly different, because of the removal of the for loop inside CNEMOEulerSolver.cpp. Notice that inside that for loop, the soundspeed is obtained using the function FluidModel->GetSoundSpeed(), which gives a slight different value than the one calculated inside CNEMOEulerVariable.cpp using FluidModel->ComputeSoundSpeed(). This means that the regression tests may fail.

@pr-triage pr-triage bot added the PR: draft label Jul 30, 2021
@fmpmorgado fmpmorgado changed the title Nemo opt init SU2-NEMO - Optimize initialization time Jul 30, 2021
SU2_CFD/src/variables/CNEMOEulerVariable.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/variables/CNEMOEulerVariable.cpp Outdated Show resolved Hide resolved
Comment on lines 151 to 156
rho = fluidmodel->GetDensity();
soundspeed = fluidmodel->ComputeSoundSpeed();
for (iDim = 0; iDim < nDim; iDim++){
sqvel += val_mach[iDim]*soundspeed * val_mach[iDim]*soundspeed;
}
energies = fluidmodel->ComputeMixtureEnergies();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rho = fluidmodel->GetDensity();
soundspeed = fluidmodel->ComputeSoundSpeed();
for (iDim = 0; iDim < nDim; iDim++){
sqvel += val_mach[iDim]*soundspeed * val_mach[iDim]*soundspeed;
}
energies = fluidmodel->ComputeMixtureEnergies();
const su2double rho = fluidmodel->GetDensity();
const su2double soundspeed = fluidmodel->ComputeSoundSpeed();
const su2double sqvel = GeometryToolbox::SquaredNorm(nDim, val_mach) * pow(soundspeed,2);
const auto& energies = fluidmodel->ComputeMixtureEnergies();

You may need the include for geometry_toolbox.hpp.
Be aware of the methods in CNEMOGas that return by reference, those little copies of std::vector add up and it is an easy fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is true, and may be the main reason of the slowness of SU2-NEMO iterations. I will try to address that in my next PR.

Copy link
Member

Choose a reason for hiding this comment

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

👍 You can have a look at #716 (scroll down) for how to use perf-tools to profile the code and point you to the hot spots "first rule of performance is that you need to measure it". There should be hotter areas in NEMO than vector copies.

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CSolverFactory.cpp Outdated Show resolved Hide resolved
@@ -168,14 +166,9 @@ CNEMOEulerVariable::CNEMOEulerVariable(su2double val_pressure,

Solution(iPoint,nSpecies+nDim) = rho*(energies[0]+0.5*sqvel);
Solution(iPoint,nSpecies+nDim+1) = rho*(energies[1]);

Solution_Old = Solution;
Copy link
Member

Choose a reason for hiding this comment

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

If you suspect this may be happening somewhere else I can tell you how to instrument the code to find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Pedro, I still need to check what is happening at each iteration, but first wanted to sort out the initialization issue

@fmpmorgado fmpmorgado marked this pull request as ready for review July 30, 2021 20:41
@lgtm-com
Copy link

lgtm-com bot commented Jul 30, 2021

This pull request fixes 1 alert when merging 040da9e into cf4677b - view on LGTM.com

fixed alerts:

  • 1 for Comparison of narrow type with wide type in loop condition

@WallyMaier
Copy link
Contributor

Thanks @fmpmorgado! NEMO definitely needs some speed up! This looks good!

Regarding the preprocessing, why is the ComputeSoundSpeed() function returning slightly different results?

@fmpmorgado
Copy link
Contributor Author

fmpmorgado commented Jul 31, 2021

Thanks @fmpmorgado! NEMO definitely needs some speed up! This looks good!

Regarding the preprocessing, why is the ComputeSoundSpeed() function returning slightly different results?

@WallyMaier

Inside the CNEMOEulerVariable, you set the fluidmodel with the freestream conditions written in the config file and use the ComputeSoundSpeed().

However, in the CNEMOEulerSolver, in the deleted loop, you had GetSoundSpeed(), which retrieves the solution already estabilished in the Primitive Matrix. The Primitive Matrix was estabilished inside the SetPrimVar() function. However, inside the function, we set the fluidmodel with slightly different values than the ones used in the config file, because we first run the iterative process to find the translational and vibrational temperature with the given energies. This iterative process provides slightly different temperatures than the ones in the config file, thus computing slightly different sound speeds

@fmpmorgado fmpmorgado merged commit adf8fbd into develop Jul 31, 2021
@fmpmorgado fmpmorgado deleted the NEMO_opt_init branch July 31, 2021 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants