KratosMultiphysics / Kratos

Kratos Multiphysics (A.K.A Kratos) is a framework for building parallel multi-disciplinary simulation software. Modularity, extensibility and HPC are the main objectives. Kratos has BSD license and is written in C++ with extensive Python interface.
https://kratosmultiphysics.github.io/Kratos/
Other
1.01k stars 244 forks source link

Many historical variables in the compressible potential flow application #5159

Open inigolcanalejo opened 5 years ago

inigolcanalejo commented 5 years ago

Do we really need all of these historical variables?

https://github.com/KratosMultiphysics/Kratos/blob/df29b6f07cdb937ec6d5f796b46ae23762295576/applications/CompressiblePotentialFlowApplication/python_scripts/potential_flow_solver.py#L119-L127

I thought at least that Velocity and Pressure will no longer be needed once https://github.com/KratosMultiphysics/Kratos/pull/5066 was merged.

What about the rest?

From what I understood from my discussions with @philbucher, @jcotela, @msandre and @armingeiser historical variables are typically to be used in transient simulations when one needs to retrieve the value of a variable from previous time steps. When using historical variables, memory is assigned for ALL nodes in the root model part at the beginning of the simulation. Therefore, having a large number of historical variables can lead to a large memory usage (imagine a 3D mesh with 5 million nodes).

For a buffer size of one, initializing a non historical variable in all nodes at the beginning of the simulation is equivalent in terms of memory usage as using a historical variable. However, the retrieve of historical variables is quicker than the retrieve of non historical variables.

My conclusion is, since our buffer size is always one:

Do you agree?

Having said this, I repeat: Do we really need all of these historical variables?

In 2D this might not have a huge impact but in 3D it will make a big difference if we want to be fast, memory efficient and robust (initializing the variables properly to avoid the access of unallocated memory)

inigolcanalejo commented 5 years ago

Please note that the above variables are only needed in the embedded case. https://github.com/KratosMultiphysics/Kratos/pull/5160 intends to solve this issue.

philbucher commented 5 years ago

is this solved?

rubenzorrilla commented 5 years ago

Actually I don't think it is as simple as

If we know that a variable is going to be set or accessed in all nodes at some point let's make it historical.

If we know that a variable is going to be set or accessed only at certain nodes, let's make it non historical and only initialize it in those nodes.

More than the number of accesses, the key aspect in here is if you need the history (time dependence) of the magnitude you are storing in such value. On top of this, you need to consider which database the auxiliary utilities and processes you need are using. Thus, it might be not as straightforward as changing your solver.

On top of this, the non-historical database requires a first SetValue call to be threadsafe. Thus, to change the database might introduce memory leaks if this is not considered.

inigolcanalejo commented 5 years ago

On top of this, the non-historical database requires a first SetValue call to be threadsafe. Thus, to change the database might introduce memory leaks if this is not considered.

I see... thanks for the explanation. Actually by initializing I was also meaning making a SetValue call at the beginning to ensure "thread safety".

rubenzorrilla commented 5 years ago

@marcnunezc got rid of the PRESSURE and VELOCITY. #5431 makes the NODAL_H non-historical. To make the level-set ones non-historical is more cumbersome since it requires to modify the distance algorithms in the core.