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.

Sign up for GitHub

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

Jump to bottom

Memory issue fix #57

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Memory issue fix #57

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
611011c
Safely free the memory in a structure
jianguan210 Jan 25, 2021
b123c45
Fixed memory issues in "init_prism()"
jianguan210 Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 39 additions & 12 deletions utils/geom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include <string.h>
#include <math.h>
#include <stdarg.h>
#include <stdbool.h>

#ifndef LIBCTLGEOM
#include "ctl-io.h"
Expand Down Expand Up @@ -80,7 +81,7 @@ static double intersect_line_segment_with_prism(prism *prsm, vector3 pc, vector3
static double get_prism_volume(prism *prsm);
static void get_prism_bounding_box(prism *prsm, geom_box *box);
static void display_prism_info(int indentby, geometric_object *o);
static void init_prism(geometric_object *o);
static void init_prism(geometric_object *o, const bool first_time);
/**************************************************************************/

/* Allows writing to Python's stdout when running from Meep's Python interface */
Expand Down Expand Up @@ -157,7 +158,7 @@ void geom_fix_object_ptr(geometric_object *o) {
break;
}
case GEOM PRISM: {
init_prism(o);
init_prism(o, false);
break;
}
case GEOM COMPOUND_GEOMETRIC_OBJECT: {
Expand Down Expand Up @@ -1359,8 +1360,28 @@ void destroy_geom_box_tree(geom_box_tree t) {
if (t) {
destroy_geom_box_tree(t->t1);
destroy_geom_box_tree(t->t2);
if (t->nobjects && t->objects) FREE(t->objects);
if (t->nobjects && t->objects) {
if (t->objects->o)
if (t->objects->o->which_subclass == GEOM PRISM) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't make sense to me. t->objects is an array of pointers to objects, but it doesn't "own" the objects and is not responsible for freeing them. (Even if it were, this code is only freeing the first object in the list, and only for prisms?)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. If I understand correctly, the purpose of this function is to destroy (free) all the memory associated with a geom_box_tree_struct object. Since we do not have the C++ style destruction function, we need to manually free all the arrays from bottom to top. The chain I found is: geom_box_object --> geometric_object --> prism_struct --> vectors3_list --> items. If we only free the top level object, the lower level memory allocated by pointers will be leaked (since no pointers are cleaned, and not pointing to the heap arrays).

I think only prism structure has this issue, other geometry objects do not have because they do not have heap arrays allocated by pointers.

Copy link
Collaborator

@stevengj stevengj Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The geom_box_tree object is used in situations like this or this or this where it is an auxiliary data structure to a linear geometric_object_list — the objects should only be freed when the corresponding list is destroyed (which should happen after the tree is destroyed). (Moreover, the same object may appear in multiple branches of the tree, so you have to be careful not to free it multiple times.)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the explanation. Since free of NULL pointer is safe, that is why NULL is assigned to the pointers after memory is freed to avoid double deletion.
There are no such memory leak issue in simulation (forward and adjoint) flow. It only happens in calculate_gradient. The flow is: calculate_gradient -> get_gradient -> destroy_geom_box_tree in "meep-python.cxx".

Also could you please point me to the place we destruct geometric_object_list since I did not find it? Thanks.

prism *prsm = t->objects->o->subclass.prism_data;
FREE(prsm->vertices_p.items);
prsm->vertices_p.items = NULL;
FREE(prsm->top_polygon_diff_vectors_p.items);
prsm->top_polygon_diff_vectors_p.items = NULL;
FREE(prsm->top_polygon_diff_vectors_scaled_p.items);
prsm->top_polygon_diff_vectors_scaled_p.items = NULL;
FREE(prsm->vertices_top.items);
prsm->vertices_top.items = NULL;
FREE(prsm->vertices_top_p.items);
prsm->vertices_top_p.items = NULL;
FREE(prsm->workspace.items);
prsm->workspace.items = NULL;
}
FREE(t->objects);
t->objects = NULL;
}
FREE1(t);
t = NULL;
}
}

Expand Down Expand Up @@ -2564,7 +2585,7 @@ vector3 triangle_normal(vector3 v1, vector3 v2, vector3 v3) {
/***************************************************************/
// special vector3 that signifies 'no value specified'
vector3 auto_center = {NAN, NAN, NAN};
void init_prism(geometric_object *o) {
void init_prism(geometric_object *o, const bool first_time) {
prism *prsm = o->subclass.prism_data;
vector3 *vertices = prsm->vertices.items;
int num_vertices = prsm->vertices.num_items;
Expand Down Expand Up @@ -2661,7 +2682,8 @@ void init_prism(geometric_object *o) {

// compute vertices in prism coordinate system
prsm->vertices_p.num_items = num_vertices;
prsm->vertices_p.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
if (first_time || !prsm->vertices_p.items)
prsm->vertices_p.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
for (nv = 0; nv < num_vertices; nv++)
prsm->vertices_p.items[nv] = prism_coordinate_c2p(prsm, vertices[nv]);

Expand Down Expand Up @@ -2703,7 +2725,8 @@ void init_prism(geometric_object *o) {
// vertices_p + top_polygon_diff_vectors_scaled_p * z
number theta = (K_PI/2) - fabs(prsm->sidewall_angle);
prsm->vertices_top_p.num_items = num_vertices;
prsm->vertices_top_p.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
if (first_time || !prsm->vertices_top_p.items)
prsm->vertices_top_p.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
CHECK(prsm->vertices_top_p.items, "out of memory");
memcpy(prsm->vertices_top_p.items, prsm->vertices_p.items, num_vertices * sizeof(vector3));
for (nv = 0; nv < num_vertices; nv++) {
Expand Down Expand Up @@ -2783,23 +2806,26 @@ void init_prism(geometric_object *o) {
prsm->vertices_top_p.items[nv].y = py;
}
}
FREE(top_polygon_edges);

prsm->top_polygon_diff_vectors_p.num_items = num_vertices;
prsm->top_polygon_diff_vectors_p.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
if (first_time || !prsm->top_polygon_diff_vectors_p.items)
prsm->top_polygon_diff_vectors_p.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
CHECK(prsm->top_polygon_diff_vectors_p.items, "out of memory");
for (nv = 0; nv < num_vertices; nv++) {
prsm->top_polygon_diff_vectors_p.items[nv] = vector3_minus(prsm->vertices_top_p.items[nv], prsm->vertices_p.items[nv]);
}

prsm->top_polygon_diff_vectors_scaled_p.num_items = num_vertices;
prsm->top_polygon_diff_vectors_scaled_p.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
CHECK(prsm->top_polygon_diff_vectors_scaled_p.items, "out of memory");
if (first_time || !prsm->top_polygon_diff_vectors_scaled_p.items)
prsm->top_polygon_diff_vectors_scaled_p.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
for (nv = 0; nv < num_vertices; nv++) {
prsm->top_polygon_diff_vectors_scaled_p.items[nv] = vector3_scale(1/prsm->height, prsm->top_polygon_diff_vectors_p.items[nv]);
}

prsm->vertices_top.num_items = num_vertices;
prsm->vertices_top.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
if (first_time || !prsm->vertices_top.items)
prsm->vertices_top.items = (vector3 *)malloc(num_vertices * sizeof(vector3));
CHECK(prsm->vertices_top.items, "out of memory");
for (nv = 0; nv < num_vertices; nv++) {
prsm->vertices_top.items[nv] = prism_coordinate_p2c(prsm, prsm->vertices_top_p.items[nv]);
Expand All @@ -2808,7 +2834,8 @@ void init_prism(geometric_object *o) {
// workspace is an internally-stored double-valued array of length num_vertices+2
// that is used by some geometry routines
prsm->workspace.num_items = num_vertices + 2;
prsm->workspace.items = (double *)malloc((num_vertices + 2) * sizeof(double));
if (first_time || !prsm->workspace.items)
prsm->workspace.items = (double *)malloc((num_vertices + 2) * sizeof(double));
}

/***************************************************************/
Expand Down Expand Up @@ -2848,6 +2875,6 @@ geometric_object make_slanted_prism_with_center(material_type material, vector3
prsm->height = height;
prsm->axis = axis;
prsm->sidewall_angle = sidewall_angle;
init_prism(&o);
init_prism(&o, true);
return o;
}