-
assigned issue to
- changed milestone to 1.6
Faulty memory management in adapt
The following code crashes:
from dolfin import *
mesh = UnitSquareMesh(16, 16)
subdomains = MeshFunction("size_t", mesh, 2, 0)
data = []
for i in range(5):
# Works if this line is not commented out
#data.append(subdomains)
markers = CellFunction("bool", mesh, True)
mesh = adapt(mesh, markers)
subdomains = adapt(subdomains, mesh)
The code runs if the MeshFunctions
are stored in a list during refinement.
The cause is likely the set_parent_child
function in adapt.cpp
which uses reference_to_no_delete_pointer
. What needs to be done is to improve the interface by adding shared pointer versions of all functions and making sure that these are used from Python (not the reference versions).
Comments (10)
-
-
reporter - changed status to wontfix
The new work Chris is doing on refinement hierarchies will change the way memory and adapted objects are handled, so a new issue will need to be opened in the unlikely event that the code by @chris_richardson is not perfect.
-
Issue
#563was marked as a duplicate of this issue. -
Example workaround for mesh functionadapt_wrapper_code = """ #include <dolfin/adaptivity/adapt.h> namespace dolfin { std::shared_ptr<MeshFunction<std::size_t>> adapt_wrapper( const MeshFunction<std::size_t>& mesh_function, std::shared_ptr<const Mesh> adapted_mesh) { std::shared_ptr<MeshFunction<std::size_t>> mf; mf = std::make_shared<MeshFunction<std::size_t>>(adapt(mesh_function, adapted_mesh)); return mf; } } """ adapt = compile_extension_module(adapt_wrapper_code).adapt_wrapper;
EDIT: This workaround is incorrect. Correct one is to never use returned value of any
adapt
function, see below. -
- removed milestone
Removing milestone: 1.6 (automated comment)
-
Few comments to this:
-
The nodeleter in
set_parent_child
_parent.set_child(child); child->set_parent(reference_to_no_delete_pointer(_parent));
is there to avoid circular pointersparent->child->parent->child-> ...
. Correct solution is havingstd::weak_ptr<T> Hierarchical::_parent
rather thanshared_ptr
with no deleter. -
Much more serious flaw is that many
adapt
functions allocate returned object bystd::shared_ptr<T> child(new T());
, store child usingset_parent_child
and return*child
by value! They should rather returnchild
, i.e. the shared ptr which takes care of object lifetime.
Workaround for users:
- Don't ever use returned object of any adapt functions!
- Access the result by
child()
method.
Example:
# Don't do this! mesh = adapt(V.mesh(), markers) u = adapt(u, mesh) # Do rather this adapt(V.mesh(), markers) mesh = V.mesh().child() adapt(u, mesh) u = u.child()
-
-
Fix of 2. in
jan/fix-issue-319
. Sketch of 1. injan/weak-parent
. -
- fixed in master.
-
-
Problem is still there. Code below work but using value returned from this function result in segmentation fault. I would guess pointer is set to local variable somewhere within code.
In general I consider declaring local variable as the first and best choice for memory magement since also make possible use of stack like memory allocation scheme with or without stack atlhough I am not certain compiler is able to handle the case then memory allocated by object is to large to handle by the stack but as with any memory magament care must be taken so that there is no pointer/reference left then object cease to exist.
void refiner(std::shared_ptr<Mesh> refined_mesh, std::shared_ptr<const Mesh> mesh, double r){ auto refineMarkers = MeshFunction<bool>(mesh, mesh->topology().dim()); refineMarkers.set_all(false); refineArea(r).mark(refineMarkers, true);
refined_mesh = std::make_shared<Mesh>(adapt(mesh.get(), refineMarkers));
VTKFile meshFile("results/meshDebug1.pvd", "ascii"); meshFile.write(*refined_mesh); }
- Log in to comment