From 8d1f4b0a61c8b51d55fddfd64142b5a1b477838b Mon Sep 17 00:00:00 2001 From: Teemu Murtola Date: Wed, 19 May 2010 13:31:58 +0200 Subject: [PATCH] Big rework of selection compilation/evaluation. Overhead from subexpression handling is now much smaller in most cases and memory management for selection method parameters is done within the selection engine for better control in cases where reallocation is necessary or memory pooling might be useful. Also fixed a bug introduced in commit 217a5f0 for complex subexpressions. --- include/selmethod.h | 10 +- src/gmxlib/selection/compiler.c | 446 ++++++++++++++++++++++----------- src/gmxlib/selection/evaluate.c | 135 ++++++---- src/gmxlib/selection/evaluate.h | 11 +- src/gmxlib/selection/mempool.c | 7 + src/gmxlib/selection/mempool.h | 3 + src/gmxlib/selection/params.c | 87 +++++-- src/gmxlib/selection/selelem.c | 140 ++++++----- src/gmxlib/selection/selelem.h | 3 +- src/gmxlib/selection/selmethod.c | 13 - src/gmxlib/selection/sm_compare.c | 27 +- src/gmxlib/selection/sm_distance.c | 1 - src/gmxlib/selection/sm_insolidangle.c | 2 - src/gmxlib/selection/sm_keywords.c | 51 +--- src/gmxlib/selection/sm_merge.c | 2 - src/gmxlib/selection/sm_permute.c | 2 - src/gmxlib/selection/sm_position.c | 1 - src/gmxlib/selection/sm_same.c | 2 - 18 files changed, 582 insertions(+), 361 deletions(-) diff --git a/include/selmethod.h b/include/selmethod.h index 9afead10e4..fdd22412cd 100644 --- a/include/selmethod.h +++ b/include/selmethod.h @@ -211,7 +211,7 @@ * In general, any of the callbacks can be NULL, but the presence of * parameters or other callbacks imposes some restrictions: * - sel_datafunc() should be provided if the method takes parameters. - * - sel_initfunc() and sel_freefunc() should be provided if the method takes + * - sel_initfunc() should be provided if the method takes * any parameters with the \ref SPAR_VARNUM or \ref SPAR_ATOMVAL flags, * except if those parameters have a \ref POS_VALUE. * - sel_outinitfunc() should be provided for \ref POS_VALUE methods @@ -411,8 +411,8 @@ typedef void (*sel_posfunc)(struct gmx_ana_poscalc_coll_t *pcc, void *data); * If a parameter had the \ref SPAR_VARNUM or \ref SPAR_ATOMVAL flag (and * is not \ref POS_VALUE), a pointer to the memory allocated for the values is * found in \c gmx_ana_selparam_t::val. - * The pointer should be stored by this function, otherwise the memory - * (and the values) are lost. + * The pointer should be stored by this function, otherwise the values + * cannot be accessed. * For \ref SPAR_VARNUM parameters, the number of values can be accessed * through \c gmx_ana_selparam_t::val. For parameters with \ref SPAR_DYNAMIC, * the number is the maximum number of values (the actual number can be @@ -477,8 +477,8 @@ typedef int (*sel_outinitfunc)(t_topology *top, gmx_ana_selvalue_t *out, * The data structure itself should not be freed; this is handled automatically. * If there is no dynamically allocated data within the structure, * this function is not needed. - * The value pointers for \ref SPAR_VARNUM and \ref SPAR_ATOMVAL parameters - * stored in sel_initfunc() should also be freed. + * Any memory pointers received as values of parameters are managed externally, + * and should not be freed. * Pointers set as the value pointer of \ref SPAR_ENUMVAL parameters should not * be freed. */ diff --git a/src/gmxlib/selection/compiler.c b/src/gmxlib/selection/compiler.c index d7b5dbe7f9..867ed07268 100644 --- a/src/gmxlib/selection/compiler.c +++ b/src/gmxlib/selection/compiler.c @@ -33,9 +33,7 @@ * * \todo * Better error handling and memory management in error situations. - * For example, memory handling in atom-valued method parameters within common - * subexpressions may currently result in memory leaks. - * Also, the main compilation function leaves the selection collection in + * At least, the main compilation function leaves the selection collection in * a bad state if an error occurs. * * \todo @@ -293,6 +291,8 @@ enum SEL_CDATA_EVALMAX = 8, /** Whether memory has been allocated for \p gmin and \p gmax. */ SEL_CDATA_MINMAXALLOC = 16, + /** Whether subexpressions use simple pass evaluation functions. */ + SEL_CDATA_SIMPLESUBEXPR = 32, }; /*! \internal \brief @@ -395,9 +395,7 @@ alloc_selection_data(t_selelem *sel, int isize, bool bChildEval) nalloc = 1; } /* Allocate memory for sel->v.u if needed */ - if ((sel->flags & SEL_ALLOCVAL) - || (sel->type == SEL_SUBEXPRREF && sel->u.param - && (sel->u.param->flags & (SPAR_VARNUM | SPAR_ATOMVAL)))) + if (sel->flags & SEL_ALLOCVAL) { _gmx_selvalue_reserve(&sel->v, nalloc); } @@ -901,28 +899,26 @@ optimize_arithmetic_expressions(t_selelem *sel) ********************************************************************/ /*! \brief - * Prepares the selection (sub)tree for evaluation. + * Sets the evaluation functions for the selection (sub)tree. * - * \param[in,out] sel Root of the selection subtree to prepare. + * \param[in,out] sel Root of the selection subtree to process. * \returns TRUE on success, FALSE if any subexpression fails. * * This function sets the evaluation function (\c t_selelem::evaluate) * for the selection elements. - * It also allocates memory for the \p sel->v.u.g or \p sel->v.u.p - * structure if required. */ static bool -init_item_evaluation(t_selelem *sel) +init_item_evalfunc(t_selelem *sel) { - t_selelem *child; - - /* Process children */ + /* Process children. */ if (sel->type != SEL_SUBEXPRREF) { + t_selelem *child; + child = sel->child; while (child) { - if (!init_item_evaluation(child)) + if (!init_item_evalfunc(child)) { return FALSE; } @@ -930,16 +926,6 @@ init_item_evaluation(t_selelem *sel) } } - /* Make sure that the group/position structure is allocated */ - if (!sel->v.u.ptr && (sel->flags & SEL_ALLOCVAL)) - { - if (sel->v.type == GROUP_VALUE || sel->v.type == POS_VALUE) - { - _gmx_selvalue_reserve(&sel->v, 1); - sel->v.nr = 1; - } - } - /* Set the evaluation function */ switch (sel->type) { @@ -987,12 +973,16 @@ init_item_evaluation(t_selelem *sel) break; case SEL_SUBEXPR: - sel->evaluate = &_gmx_sel_evaluate_subexpr; + sel->evaluate = (sel->refcount == 2 + ? &_gmx_sel_evaluate_subexpr_simple + : &_gmx_sel_evaluate_subexpr); break; case SEL_SUBEXPRREF: sel->name = sel->child->name; - sel->evaluate = &_gmx_sel_evaluate_subexprref; + sel->evaluate = (sel->child->refcount == 2 + ? &_gmx_sel_evaluate_subexprref_simple + : &_gmx_sel_evaluate_subexprref); break; } @@ -1008,12 +998,6 @@ init_item_evaluation(t_selelem *sel) static void setup_memory_pooling(t_selelem *sel, gmx_sel_mempool_t *mempool) { - bool bPoolChildren = FALSE; - - if (sel->type == SEL_BOOLEAN) - { - bPoolChildren = TRUE; - } if (sel->type != SEL_SUBEXPRREF) { t_selelem *child; @@ -1021,9 +1005,15 @@ setup_memory_pooling(t_selelem *sel, gmx_sel_mempool_t *mempool) child = sel->child; while (child) { - if (bPoolChildren && (child->flags & SEL_DYNAMIC)) + if ((sel->type == SEL_BOOLEAN && (child->flags & SEL_DYNAMIC)) + || (sel->type == SEL_SUBEXPR && sel->refcount > 2)) { child->mempool = mempool; + if (child->type == SEL_SUBEXPRREF + && child->child->refcount == 2) + { + child->child->child->mempool = mempool; + } } setup_memory_pooling(child, mempool); child = child->next; @@ -1031,6 +1021,78 @@ setup_memory_pooling(t_selelem *sel, gmx_sel_mempool_t *mempool) } } +/*! \brief + * Prepares the selection (sub)tree for evaluation. + * + * \param[in,out] sel Root of the selection subtree to prepare. + * + * It also allocates memory for the \p sel->v.u.g or \p sel->v.u.p + * structure if required. + */ +static void +init_item_evaloutput(t_selelem *sel) +{ + /* Process children. */ + if (sel->type != SEL_SUBEXPRREF) + { + t_selelem *child; + + child = sel->child; + while (child) + { + init_item_evaloutput(child); + child = child->next; + } + } + + if (sel->type == SEL_SUBEXPR && sel->refcount == 2) + { + sel->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); + if (sel->v.type == GROUP_VALUE || sel->v.type == POS_VALUE) + { + _gmx_selvalue_setstore(&sel->v, sel->child->v.u.ptr); + } + } + else if (sel->type == SEL_SUBEXPR + && (sel->cdata->flags & SEL_CDATA_FULLEVAL)) + { + sel->evaluate = &_gmx_sel_evaluate_subexpr_staticeval; + sel->cdata->evaluate = sel->evaluate; + sel->child->mempool = NULL; + sel->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); + if (sel->v.type == GROUP_VALUE || sel->v.type == POS_VALUE) + { + _gmx_selvalue_setstore(&sel->v, sel->child->v.u.ptr); + } + } + else if (sel->type == SEL_SUBEXPRREF && sel->child->refcount == 2) + { + if (sel->v.u.ptr) + { + _gmx_selvalue_setstore(&sel->child->v, sel->v.u.ptr); + _gmx_selelem_free_values(sel->child->child); + sel->child->child->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); + sel->child->child->flags |= (sel->flags & SEL_ALLOCDATA); + _gmx_selvalue_setstore(&sel->child->child->v, sel->v.u.ptr); + } + else if (sel->v.type == GROUP_VALUE || sel->v.type == POS_VALUE) + { + _gmx_selvalue_setstore(&sel->v, sel->child->child->v.u.ptr); + } + sel->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); + } + + /* Make sure that the group/position structure is allocated. */ + if (!sel->v.u.ptr && (sel->flags & SEL_ALLOCVAL)) + { + if (sel->v.type == GROUP_VALUE || sel->v.type == POS_VALUE) + { + _gmx_selvalue_reserve(&sel->v, 1); + sel->v.nr = 1; + } + } +} + /******************************************************************** * COMPILER DATA INITIALIZATION PASS ********************************************************************/ @@ -1060,6 +1122,14 @@ init_item_compilerdata(t_selelem *sel) if (sel->type == SEL_SUBEXPR) { sel->cdata->flags |= SEL_CDATA_EVALMAX; + if (sel->refcount == 2) + { + sel->cdata->flags |= SEL_CDATA_SIMPLESUBEXPR; + } + } + if (sel->type == SEL_SUBEXPRREF && sel->child->refcount == 2) + { + sel->cdata->flags |= SEL_CDATA_SIMPLESUBEXPR; } /* Set the full evaluation flag for subexpressions that require it; * the subexpression has already been initialized, so we can simply @@ -1069,7 +1139,7 @@ init_item_compilerdata(t_selelem *sel) child = sel->child; while (child) { - if (!(child->flags & SEL_ATOMVAL)) + if (!(child->flags & SEL_ATOMVAL) && child->child) { child->child->cdata->flags |= SEL_CDATA_FULLEVAL; } @@ -1123,28 +1193,6 @@ init_item_compilerdata(t_selelem *sel) child = child->next; } } - - /* Initialize the minimum and maximum evaluation groups */ - if (sel->type != SEL_ROOT && sel->v.type != NO_VALUE) - { - if (sel->type == SEL_SUBEXPR) - { - sel->cdata->gmin = sel->child->cdata->gmin; - sel->cdata->gmax = sel->child->cdata->gmax; - } - else if (sel->v.type == GROUP_VALUE - && (sel->cdata->flags & SEL_CDATA_STATIC)) - { - sel->cdata->gmin = sel->v.u.g; - sel->cdata->gmax = sel->v.u.g; - } - else - { - sel->cdata->flags |= SEL_CDATA_MINMAXALLOC; - snew(sel->cdata->gmin, 1); - snew(sel->cdata->gmax, 1); - } - } } /*! \brief @@ -1221,6 +1269,50 @@ init_item_staticeval(t_selelem *sel) } } +/*! \brief + * Initializes the gmin and gmax fields of the compiler data structure. + * + * \param sel Root of the selection subtree to process. + */ +static void +init_item_minmax_groups(t_selelem *sel) +{ + /* Process children. */ + if (sel->type != SEL_SUBEXPRREF) + { + t_selelem *child; + + child = sel->child; + while (child) + { + init_item_minmax_groups(child); + child = child->next; + } + } + + /* Initialize the minimum and maximum evaluation groups. */ + if (sel->type != SEL_ROOT && sel->v.type != NO_VALUE) + { + if (sel->v.type == GROUP_VALUE + && (sel->cdata->flags & SEL_CDATA_STATIC)) + { + sel->cdata->gmin = sel->v.u.g; + sel->cdata->gmax = sel->v.u.g; + } + else if (sel->type == SEL_SUBEXPR) + { + sel->cdata->gmin = sel->child->cdata->gmin; + sel->cdata->gmax = sel->child->cdata->gmax; + } + else + { + sel->cdata->flags |= SEL_CDATA_MINMAXALLOC; + snew(sel->cdata->gmin, 1); + snew(sel->cdata->gmax, 1); + } + } +} + /******************************************************************** * EVALUATION GROUP INITIALIZATION ********************************************************************/ @@ -1238,7 +1330,6 @@ static void initialize_evalgrps(gmx_ana_selcollection_t *sc) { t_selelem *root; - int i; root = sc->root; while (root) @@ -1309,22 +1400,29 @@ mark_subexpr_dynamic(t_selelem *sel, bool bDynamic) static void release_subexpr_memory(t_selelem *sel) { - t_selelem *child; - - child = sel->child; - while (child) + if (sel->type == SEL_SUBEXPR) { - release_subexpr_memory(child); - child = child->next; + if (sel->refcount == 2) + { + release_subexpr_memory(sel->child); + sel->name = NULL; + _gmx_selelem_free_chain(sel->child); + _gmx_selelem_free_values(sel); + _gmx_selelem_free_exprdata(sel); + _gmx_selelem_free_compiler_data(sel); + sel->child = NULL; + } } - - if (sel->type == SEL_SUBEXPR && sel->refcount == 2) + else { - _gmx_selelem_free_values(sel); - _gmx_selelem_free_exprdata(sel); - _gmx_selelem_free_compiler_data(sel); - _gmx_selelem_free_chain(sel->child); - sel->child = NULL; + t_selelem *child; + + child = sel->child; + while (child) + { + release_subexpr_memory(child); + child = child->next; + } } } @@ -1340,6 +1438,37 @@ release_subexpr_memory(t_selelem *sel) static void make_static(t_selelem *sel) { + /* If this is a subexpression reference and the data is stored in the + * child, we transfer data ownership before doing anything else. */ + if (sel->type == SEL_SUBEXPRREF + && (sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR)) + { + if (sel->child->child->flags & SEL_ALLOCDATA) + { + sel->flags |= SEL_ALLOCDATA; + sel->child->child->flags &= ~SEL_ALLOCDATA; + } + if (sel->child->child->flags & SEL_ALLOCVAL) + { + sel->flags |= SEL_ALLOCVAL; + sel->v.nalloc = sel->child->child->v.nalloc; + sel->child->child->flags &= ~SEL_ALLOCVAL; + sel->child->child->v.nalloc = -1; + } + } + /* When we reach here for parameter elements, the value is already + * stored in the parent element, so make sure that it is not freed + * through this element. */ + if (sel->type == SEL_SUBEXPRREF && sel->u.param) + { + sel->u.param->val.nalloc = sel->v.nalloc; + sel->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); + sel->v.nalloc = -1; + } + /* Free the children. */ + release_subexpr_memory(sel); + _gmx_selelem_free_chain(sel->child); + sel->child = NULL; /* Free the expression data as it is no longer needed */ _gmx_selelem_free_exprdata(sel); /* Make the item static */ @@ -1347,10 +1476,6 @@ make_static(t_selelem *sel) sel->type = SEL_CONST; sel->evaluate = NULL; sel->cdata->evaluate = NULL; - /* Free the children */ - release_subexpr_memory(sel); - _gmx_selelem_free_chain(sel->child); - sel->child = NULL; /* Set the group value. * free_exprdata above frees the cgrp group, so we can just override it. */ if (sel->v.type == GROUP_VALUE) @@ -1456,18 +1581,6 @@ init_method(t_selelem *sel, t_topology *top, int isize) && (bAtomVal || !(sel->flags & SEL_METHODINIT))) { sel->flags |= SEL_METHODINIT; - /* The allocation flags are cleared first to not to free anything if - * initialization fails. */ - child = sel->child; - if (sel->type == SEL_MODIFIER && sel->v.type != NO_VALUE) - { - child = child->next; - } - while (child) - { - child->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); - child = child->next; - } rc = sel->u.expr.method->init(top, sel->u.expr.method->nparams, sel->u.expr.method->param, sel->u.expr.mdata); if (rc != 0) @@ -1573,6 +1686,7 @@ evaluate_boolean_static_part(gmx_sel_evaluate_t *data, t_selelem *sel, _gmx_selvalue_reserve(&child->v, 1); gmx_ana_index_copy(child->v.u.g, sel->v.u.g, TRUE); init_item_compilerdata(child); + init_item_minmax_groups(child); child->cdata->flags &= ~SEL_CDATA_STATICEVAL; child->cdata->flags |= sel->cdata->flags & SEL_CDATA_STATICEVAL; child->next = next; @@ -1853,6 +1967,7 @@ analyze_static(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) { return rc; } + sel->v.nr = (sel->flags & SEL_SINGLEVAL) ? 1 : g->isize; gmx_ana_index_copy(&gmax, g, TRUE); } break; @@ -1862,7 +1977,21 @@ analyze_static(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) break; case SEL_SUBEXPR: - if (sel->u.cgrp.isize == 0) + if (sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR) + { + rc = sel->cdata->evaluate(data, sel, g); + _gmx_selvalue_setstore(&sel->v, sel->child->v.u.ptr); + } + else if (sel->cdata->flags & SEL_CDATA_FULLEVAL) + { + if (sel->u.cgrp.isize == 0) + { + gmx_ana_index_reserve(&sel->u.cgrp, g->isize); + rc = sel->cdata->evaluate(data, sel, g); + _gmx_selvalue_setstore(&sel->v, sel->child->v.u.ptr); + } + } + else if (sel->u.cgrp.isize == 0) { gmx_ana_index_reserve(&sel->u.cgrp, g->isize); if (bDelayAlloc) @@ -1882,7 +2011,6 @@ analyze_static(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) } else { - alloc_selection_data(sel->child, g->isize, FALSE); rc = sel->cdata->evaluate(data, sel, g); } } @@ -1895,35 +2023,37 @@ analyze_static(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) gmx_ana_index_reserve(&sel->u.cgrp, isize); if (sel->v.type == GROUP_VALUE || (sel->flags & SEL_ATOMVAL)) { - alloc_selection_data(sel->child, isize, FALSE); - alloc_selection_data(sel, isize, FALSE); + alloc_selection_data(sel, isize, FALSE); } - rc = sel->cdata->evaluate(data, sel, g); - } - else - { - rc = sel->cdata->evaluate(data, sel, g); } + rc = sel->cdata->evaluate(data, sel, g); } break; case SEL_SUBEXPRREF: - /* The subexpression should have been evaluated if g is NULL - * (i.e., this is a method parameter or a direct value of a - * selection). */ - if (!g) + if (sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR) { - alloc_selection_data(sel, sel->child->cdata->gmax->isize, TRUE); + rc = sel->cdata->evaluate(data, sel, g); + if (sel->child->child->flags & SEL_ALLOCVAL) + { + _gmx_selvalue_setstore(&sel->v, sel->child->child->v.u.ptr); + } } - /* TODO: This is not general enough if/when position references - * can be evaluated more than once (that is, if there are position - * methods that do not have SMETH_SINGLEVAL or SMETH_VARNUMVAL). */ - if (sel->v.type == POS_VALUE && !(sel->flags & SEL_OUTINIT)) + else if (sel->child->cdata->flags & SEL_CDATA_FULLEVAL) { - gmx_ana_pos_copy(sel->v.u.p, sel->child->child->v.u.p, TRUE); - sel->flags |= SEL_OUTINIT; + /* The subexpression should have been evaluated if g is NULL + * (i.e., this is a method parameter or a direct value of a + * selection). */ + if (!g) + { + alloc_selection_data(sel, sel->child->cdata->gmax->isize, TRUE); + } + rc = sel->cdata->evaluate(data, sel, g); + } + else + { + rc = sel->cdata->evaluate(data, sel, g); } - rc = sel->cdata->evaluate(data, sel, g); if (rc != 0) { return rc; @@ -1988,14 +2118,15 @@ analyze_static(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) gmx_ana_index_deinit(&gmax); /* Make sure that enough data storage has been allocated */ - /* TODO: Constant expressions could be handled here more intelligently */ - if (sel->type != SEL_ROOT && (sel->cdata->flags & SEL_CDATA_STATICEVAL)) + if (sel->type != SEL_ROOT + && ((sel->cdata->flags & SEL_CDATA_STATICEVAL) + || (!(sel->flags & SEL_DYNAMIC) + && !(sel->cdata->flags & SEL_CDATA_STATIC)))) { alloc_selection_data(sel, sel->cdata->gmax->isize, TRUE); /* Make sure that the new value pointer is stored if required */ store_param_val(sel); } - return 0; } @@ -2107,11 +2238,9 @@ init_root_item(t_selelem *root, gmx_ana_index_t *gall) /* Process subexpressions */ if (root->child->type == SEL_SUBEXPR) { - if (root->child->v.type == POS_VALUE) - { - /* Position values only need to be evaluated once, by the root */ - } - else if (!(root->child->cdata->flags & SEL_CDATA_STATICEVAL)) + if (!(root->child->cdata->flags & SEL_CDATA_STATICEVAL) + || ((root->child->cdata->flags & SEL_CDATA_SIMPLESUBEXPR) + && !(root->child->cdata->flags & SEL_CDATA_FULLEVAL))) { /* Subexpressions with non-static evaluation group should not be * evaluated by the root. */ @@ -2176,46 +2305,63 @@ init_root_item(t_selelem *root, gmx_ana_index_t *gall) * referenced once. */ static void -optimize_item_subexpressions(t_selelem *sel) +postprocess_item_subexpressions(t_selelem *sel) { - t_selelem *child; - - /* Call recursively for all children unless the children have already been processed */ + /* Process children. */ if (sel->type != SEL_SUBEXPRREF) { + t_selelem *child; + child = sel->child; while (child) { - optimize_item_subexpressions(child); + postprocess_item_subexpressions(child); child = child->next; } } - /* Optimize the evaluation of subexpressions that are used only once */ - if (sel->type == SEL_SUBEXPRREF && (sel->cdata->flags & SEL_CDATA_STATICEVAL) - && sel->child->refcount == 2) + /* Replace the evaluation function of statically evaluated subexpressions + * for which the static group was not known in advance. */ + if (sel->type == SEL_SUBEXPR && sel->refcount > 2 + && (sel->cdata->flags & SEL_CDATA_STATICEVAL) + && !(sel->cdata->flags & SEL_CDATA_FULLEVAL)) { - /* The evaluation functions are not always needed, and they do some - * extra work, but it should be neglible compared to other factors - * in the evaluation, so set them always for simplicity. */ - sel->evaluate = &_gmx_sel_evaluate_subexprref_pass; + sel->evaluate = &_gmx_sel_evaluate_subexpr_staticeval; if (sel->cdata) { sel->cdata->evaluate = sel->evaluate; } - /* Replace the value of the child */ - _gmx_selelem_free_values(sel->child); - sel->child->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); + sel->child->mempool = NULL; _gmx_selvalue_setstore(&sel->child->v, sel->v.u.ptr); - sel->child->evaluate = &_gmx_sel_evaluate_subexpr_pass; - if (sel->child->cdata) + sel->child->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); + } + + /* Adjust memory allocation flags for subexpressions that are used only + * once. This is not strictly necessary, but we do it to have the memory + * managed consistently for all types of subexpressions. */ + if (sel->type == SEL_SUBEXPRREF + && (sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR)) + { + if (sel->child->child->flags & SEL_ALLOCVAL) { - sel->child->cdata->evaluate = sel->child->evaluate; + sel->flags |= SEL_ALLOCVAL; + sel->flags |= (sel->child->child->flags & SEL_ALLOCDATA); + sel->v.nalloc = sel->child->child->v.nalloc; + sel->child->child->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); + sel->child->child->v.nalloc = -1; } - /* Replace the value of the grandchild */ - _gmx_selelem_free_values(sel->child->child); - sel->child->child->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); - _gmx_selvalue_setstore(&sel->child->child->v, sel->v.u.ptr); + } + + /* Do the same for subexpressions that are evaluated at once for all atoms. */ + if (sel->type == SEL_SUBEXPR + && !(sel->cdata->flags & SEL_CDATA_SIMPLESUBEXPR) + && (sel->cdata->flags & SEL_CDATA_FULLEVAL)) + { + sel->flags |= SEL_ALLOCVAL; + sel->flags |= (sel->child->flags & SEL_ALLOCDATA); + sel->v.nalloc = sel->child->v.nalloc; + sel->child->flags &= ~(SEL_ALLOCVAL | SEL_ALLOCDATA); + sel->child->v.nalloc = -1; } } @@ -2452,8 +2598,8 @@ gmx_ana_selcollection_compile(gmx_ana_selcollection_t *sc) /* FIXME: Clean up the collection */ return -1; } - /* Initialize evaluation */ - if (!init_item_evaluation(item)) + /* Initialize evaluation function. */ + if (!init_item_evalfunc(item)) { /* FIXME: Clean up the collection */ return -1; @@ -2464,6 +2610,22 @@ gmx_ana_selcollection_compile(gmx_ana_selcollection_t *sc) init_item_staticeval(item); item = item->next; } + /* Initialize evaluation output. + * Requires compiler flags for the full tree. */ + item = sc->root; + while (item) + { + init_item_evaloutput(item); + item = item->next; + } + /* Initialize minimum/maximum index groups. + * Requires evaluation output for the full tree. */ + item = sc->root; + while (item) + { + init_item_minmax_groups(item); + item = item->next; + } /* Initialize the evaluation index groups */ initialize_evalgrps(sc); @@ -2502,7 +2664,7 @@ gmx_ana_selcollection_compile(gmx_ana_selcollection_t *sc) { mark_subexpr_dynamic(item->child, FALSE); item->child->u.cgrp.isize = 0; - /* We won't clear item->child->child->v.u.g here, because it may + /* We won't clear item->child->v.u.g here, because it may * be static, and hence actually point to item->child->cdata->gmax, * which is used below. We could also check whether this is the * case and only clear the group otherwise, but because the value @@ -2540,7 +2702,7 @@ gmx_ana_selcollection_compile(gmx_ana_selcollection_t *sc) while (item) { init_root_item(item, &sc->gall); - optimize_item_subexpressions(item); + postprocess_item_subexpressions(item); rc = init_item_comg(item, sc->pcc, post, flags); if (rc != 0) { diff --git a/src/gmxlib/selection/evaluate.c b/src/gmxlib/selection/evaluate.c index 79395b29c3..699abfed88 100644 --- a/src/gmxlib/selection/evaluate.c +++ b/src/gmxlib/selection/evaluate.c @@ -154,7 +154,7 @@ gmx_ana_selcollection_evaluate(gmx_ana_selcollection_t *sc, * _gmx_sel_evaluate_subexpr(). */ if (sel->child->v.type == GROUP_VALUE) { - sel->child->child->v.u.g->isize = 0; + sel->child->v.u.g->isize = 0; } } if (sel->evaluate) @@ -311,15 +311,15 @@ _gmx_sel_evaluate_static(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index * \returns 0 on success, a non-zero error code on error. * * Evaluates the child element (there should be exactly one) in \p g. - * The number of values is copied from the child to \p sel->v.nr, but - * otherwise the value of \p sel is not touched. + * The compiler has taken care that the child actually stores the evaluated + * value in the value pointer of this element. * * This function is used as \c t_selelem::evaluate for \ref SEL_SUBEXPR * elements that are used only once, and hence do not need full subexpression * handling. */ int -_gmx_sel_evaluate_subexpr_pass(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) +_gmx_sel_evaluate_subexpr_simple(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) { int rc; @@ -336,6 +336,44 @@ _gmx_sel_evaluate_subexpr_pass(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana } /*! + * \param[in] data Data for the current frame. + * \param[in] sel Selection element being evaluated. + * \param[in] g Group for which \p sel should be evaluated. + * \returns 0 on success, a non-zero error code on error. + * + * If this is the first call for this frame, evaluates the child element + * there should be exactly one in \p g. + * The compiler has taken care that the child actually stores the evaluated + * value in the value pointer of this element. + * + * This function is used as \c t_selelem::evaluate for \ref SEL_SUBEXPR + * elements that have a static evaluation group, and hence do not need full + * subexpression handling. + */ +int +_gmx_sel_evaluate_subexpr_staticeval(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) +{ + if (sel->u.cgrp.isize == 0) + { + char *name; + int rc; + + rc = sel->child->evaluate(data, sel->child, g); + if (rc != 0) + { + return rc; + } + sel->v.nr = sel->child->v.nr; + /* We need to keep the name for the cgrp across the copy to avoid + * problems if g has a name set. */ + name = sel->u.cgrp.name; + gmx_ana_index_copy(&sel->u.cgrp, g, FALSE); + sel->u.cgrp.name = name; + } + return 0; +} + +/*! * \param[in] data Data for the current frame. * \param[in] sel Selection element being evaluated. * \param[in] g Group for which \p sel should be evaluated. @@ -351,6 +389,9 @@ _gmx_sel_evaluate_subexpr_pass(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana * The call to gmx_ana_index_difference() can take quite a lot of unnecessary * time if the subexpression is evaluated either several times for the same * group or for completely distinct groups. + * However, in the majority of cases, these situations occur when + * _gmx_sel_evaluate_subexpr_staticeval() can be used, so this should not be a + * major problem. */ int _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) @@ -361,11 +402,16 @@ _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_inde if (sel->u.cgrp.isize == 0) { char *name; + /* We need to check for the presence because the compiler may clear * it temporarily. */ if (sel->child->evaluate) { + void *old_ptr = sel->child->v.u.ptr; + int old_nalloc = sel->child->v.nalloc; + _gmx_selvalue_setstore(&sel->child->v, sel->v.u.ptr); rc = sel->child->evaluate(data, sel->child, g); + _gmx_selvalue_setstore_alloc(&sel->child->v, old_ptr, old_nalloc); if (rc != 0) { return rc; @@ -380,33 +426,24 @@ _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_inde } else { - if (sel->v.type == GROUP_VALUE) + /* We allocate some extra memory here to avoid some computation. */ + rc = _gmx_sel_mempool_alloc_group(data->mp, &gmiss, g->isize); + if (rc != 0) { - gmx_ana_index_set(&gmiss, 0, sel->child->v.u.g->index + sel->child->v.u.g->isize, NULL, 0); + return rc; } - else + gmx_ana_index_difference(&gmiss, g, &sel->u.cgrp); + if (gmiss.isize == 0) { - gmx_ana_index_set(&gmiss, 0, sel->u.cgrp.index + sel->u.cgrp.isize, NULL, 0); + _gmx_sel_mempool_free_group(data->mp, &gmiss); } - gmx_ana_index_difference(&gmiss, g, &sel->u.cgrp); } if (gmiss.isize > 0) { - /* We use the value of sel to store the old value of the child before - * evaluating the missing values. */ - if (sel->v.type == GROUP_VALUE) - { - atom_id *tmp = sel->child->v.u.g->index; - sel->child->v.u.g->index = sel->v.u.g->index; - sel->v.u.g->index = tmp; - sel->v.u.g->isize = sel->child->v.u.g->isize; - sel->child->v.u.g->isize = 0; - } - else + rc = _gmx_selelem_mempool_reserve(sel->child, gmiss.isize); + if (rc != 0) { - void *tmp = sel->child->v.u.ptr; - sel->child->v.u.ptr = sel->v.u.ptr; - sel->v.u.ptr = tmp; + return rc; } /* Evaluate the missing values for the child */ rc = sel->child->evaluate(data, sel->child, &gmiss); @@ -417,8 +454,7 @@ _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_inde /* Merge the missing values to the existing ones. */ if (sel->v.type == GROUP_VALUE) { - gmx_ana_index_merge(sel->child->v.u.g, sel->child->v.u.g, sel->v.u.g); - gmx_ana_index_merge(&sel->u.cgrp, &sel->u.cgrp, &gmiss); + gmx_ana_index_merge(sel->v.u.g, sel->child->v.u.g, sel->v.u.g); } else { @@ -435,11 +471,11 @@ _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_inde { if (i < 0 || (j >= 0 && sel->u.cgrp.index[i] < gmiss.index[j])) { - sel->child->v.u.i[k] = sel->child->v.u.i[j--]; + sel->v.u.i[k] = sel->v.u.i[j--]; } else { - sel->child->v.u.i[k] = sel->v.u.i[i--]; + sel->v.u.i[k] = sel->child->v.u.i[i--]; } } break; @@ -449,11 +485,11 @@ _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_inde { if (i < 0 || (j >= 0 && sel->u.cgrp.index[i] < gmiss.index[j])) { - sel->child->v.u.r[k] = sel->child->v.u.r[j--]; + sel->v.u.r[k] = sel->v.u.r[j--]; } else { - sel->child->v.u.r[k] = sel->v.u.r[i--]; + sel->v.u.r[k] = sel->child->v.u.r[i--]; } } break; @@ -463,11 +499,11 @@ _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_inde { if (i < 0 || (j >= 0 && sel->u.cgrp.index[i] < gmiss.index[j])) { - sel->child->v.u.s[k] = sel->child->v.u.s[j--]; + sel->v.u.s[k] = sel->v.u.s[j--]; } else { - sel->child->v.u.s[k] = sel->v.u.s[i--]; + sel->v.u.s[k] = sel->child->v.u.s[i--]; } } break; @@ -482,10 +518,10 @@ _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_inde gmx_bug("internal error"); return -1; } - /* TODO: With some additional storage, we could do a merge here */ - sel->u.cgrp.isize += gmiss.isize; - gmx_ana_index_sort(&sel->u.cgrp); } + gmx_ana_index_merge(&sel->u.cgrp, &sel->u.cgrp, &gmiss); + _gmx_selelem_mempool_release(sel->child); + _gmx_sel_mempool_free_group(data->mp, &gmiss); } return 0; } @@ -496,15 +532,30 @@ _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_inde * \param[in] g Group for which \p sel should be evaluated. * \returns 0 for success. * + * Sets the value pointers of the child and its child to point to the same + * memory as the value pointer of this element to avoid copying, and then + * evaluates evaluates the child. + * * This function is used as \c t_selelem:evaluate for \ref SEL_SUBEXPRREF - * elements for which the actual value is evaluated directly by the - * subexpression, but for which the number of values needs to be passed - * forward. + * elements for which the \ref SEL_SUBEXPR does not have other references. */ int -_gmx_sel_evaluate_subexprref_pass(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) +_gmx_sel_evaluate_subexprref_simple(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g) { - sel->v.nr = sel->child->child->v.nr; + if (g) + { + int rc; + + _gmx_selvalue_setstore(&sel->child->v, sel->v.u.ptr); + _gmx_selvalue_setstore_alloc(&sel->child->child->v, sel->v.u.ptr, + sel->child->child->v.nalloc); + rc = sel->child->evaluate(data, sel->child, g); + if (rc != 0) + { + return rc; + } + } + sel->v.nr = sel->child->v.nr; if (sel->u.param) { sel->u.param->val.nr = sel->v.nr; @@ -538,9 +589,7 @@ _gmx_sel_evaluate_subexprref(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_i t_selelem *expr; int i, j; - /* We need to check for the presence because the compiler may clear - * it temporarily. */ - if (g && sel->child->evaluate) + if (g) { int rc; @@ -550,7 +599,7 @@ _gmx_sel_evaluate_subexprref(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_i return rc; } } - expr = sel->child->child; + expr = sel->child; switch (sel->v.type) { case INT_VALUE: diff --git a/src/gmxlib/selection/evaluate.h b/src/gmxlib/selection/evaluate.h index 4a195813b2..99c15c7169 100644 --- a/src/gmxlib/selection/evaluate.h +++ b/src/gmxlib/selection/evaluate.h @@ -100,15 +100,18 @@ _gmx_sel_evaluate_arithmetic(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_i /*! \name Subexpression evaluation functions */ /*@{*/ -/** Evaluates a subexpression, only handling the number of values. */ +/** Evaluates a subexpression when there is only one reference. */ int -_gmx_sel_evaluate_subexpr_pass(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g); +_gmx_sel_evaluate_subexpr_simple(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g); +/** Evaluates a subexpression when the evaluation group is static. */ +int +_gmx_sel_evaluate_subexpr_staticeval(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g); /** Evaluates a subexpression. */ int _gmx_sel_evaluate_subexpr(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g); -/** Evaluates a subexpression reference, only handling the number of values. */ +/** Evaluates a subexpression reference when there are no other references. */ int -_gmx_sel_evaluate_subexprref_pass(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g); +_gmx_sel_evaluate_subexprref_simple(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g); /** Evaluates a subexpression reference. */ int _gmx_sel_evaluate_subexprref(gmx_sel_evaluate_t *data, t_selelem *sel, gmx_ana_index_t *g); diff --git a/src/gmxlib/selection/mempool.c b/src/gmxlib/selection/mempool.c index b845d445a8..215575c015 100644 --- a/src/gmxlib/selection/mempool.c +++ b/src/gmxlib/selection/mempool.c @@ -201,3 +201,10 @@ _gmx_sel_mempool_alloc_group(gmx_sel_mempool_t *mp, gmx_ana_index_t *g, return _gmx_sel_mempool_alloc(mp, (void **)&g->index, sizeof(*g->index)*isize); } + +void +_gmx_sel_mempool_free_group(gmx_sel_mempool_t *mp, gmx_ana_index_t *g) +{ + _gmx_sel_mempool_free(mp, g->index); + g->index = NULL; +} diff --git a/src/gmxlib/selection/mempool.h b/src/gmxlib/selection/mempool.h index 548a46fb62..ca6abba689 100644 --- a/src/gmxlib/selection/mempool.h +++ b/src/gmxlib/selection/mempool.h @@ -62,5 +62,8 @@ _gmx_sel_mempool_reserve(gmx_sel_mempool_t *mp, size_t size); int _gmx_sel_mempool_alloc_group(gmx_sel_mempool_t *mp, struct gmx_ana_index_t *g, int isize); +/** Convenience function for freeing an index group from a memory pool. */ +void +_gmx_sel_mempool_free_group(gmx_sel_mempool_t *mp, struct gmx_ana_index_t *g); #endif diff --git a/src/gmxlib/selection/params.c b/src/gmxlib/selection/params.c index 0ae9774405..e46863a1b5 100644 --- a/src/gmxlib/selection/params.c +++ b/src/gmxlib/selection/params.c @@ -180,6 +180,44 @@ convert_values(t_selexpr_value *values, e_selvalue_t type, void *scanner) } /*! \brief + * Adds a child element for a parameter, keeping the parameter order. + * + * \param[in,out] root Root element to which the child is added. + * \param[in] child Child to add. + * \param[in] param Parameter for which this child is a value. + * + * Puts \p child in the child list of \p root such that the list remains + * in the same order as the corresponding parameters. + */ +static void +place_child(t_selelem *root, t_selelem *child, gmx_ana_selparam_t *param) +{ + gmx_ana_selparam_t *ps; + int n; + + ps = root->u.expr.method->param; + n = param - ps; + /* Put the child element in the correct place */ + if (!root->child || n < root->child->u.param - ps) + { + child->next = root->child; + root->child = child; + } + else + { + t_selelem *prev; + + prev = root->child; + while (prev->next && prev->next->u.param - ps >= n) + { + prev = prev->next; + } + child->next = prev->next; + prev->next = child; + } +} + +/*! \brief * Comparison function for sorting integer ranges. * * \param[in] a Pointer to the first range. @@ -419,13 +457,15 @@ parse_values_range(int nval, t_selexpr_value *values, gmx_ana_selparam_t *param) * \param[in] nval Number of values in \p values. * \param[in] values Pointer to the list of values. * \param param Parameter to parse. + * \param root Selection element to which child expressions are added. * \returns TRUE if the values were parsed successfully, FALSE otherwise. * * For integer ranges, the sequence of numbers from the first to second value * is stored, each as a separate value. */ static bool -parse_values_varnum(int nval, t_selexpr_value *values, gmx_ana_selparam_t *param) +parse_values_varnum(int nval, t_selexpr_value *values, + gmx_ana_selparam_t *param, t_selelem *root) { t_selexpr_value *value; int i, j; @@ -519,6 +559,26 @@ parse_values_varnum(int nval, t_selexpr_value *values, gmx_ana_selparam_t *param *param->nvalptr = param->val.nr; } param->nvalptr = NULL; + /* Create a dummy child element to store the string values. + * This element is responsible for freeing the values, but carries no + * other function. */ + if (param->val.type == STR_VALUE) + { + t_selelem *child; + + child = _gmx_selelem_create(SEL_CONST); + _gmx_selelem_set_vtype(child, STR_VALUE); + child->name = param->name; + child->flags &= ~SEL_ALLOCVAL; + child->flags |= SEL_FLAGSSET | SEL_VARNUMVAL | SEL_ALLOCDATA; + child->v.nr = param->val.nr; + _gmx_selvalue_setstore(&child->v, param->val.u.s); + /* Because the child is not group-valued, the u union is not used + * for anything, so we can abuse it by storing the parameter value + * as place_child() expects, but this is really ugly... */ + child->u.param = param; + place_child(root, child, param); + } return TRUE; } @@ -539,8 +599,6 @@ parse_values_varnum(int nval, t_selexpr_value *values, gmx_ana_selparam_t *param static t_selelem * add_child(t_selelem *root, gmx_ana_selparam_t *param, t_selelem *expr) { - gmx_ana_selparam_t *ps; - int n; t_selelem *child; int rc; @@ -549,8 +607,6 @@ add_child(t_selelem *root, gmx_ana_selparam_t *param, t_selelem *expr) gmx_bug("unsupported root element for selection parameter parser"); return NULL; } - ps = root->u.expr.method->param; - n = param - ps; /* Create a subexpression reference element if necessary */ if (expr->type == SEL_SUBEXPRREF) { @@ -591,23 +647,7 @@ add_child(t_selelem *root, gmx_ana_selparam_t *param, t_selelem *expr) param->flags &= ~SPAR_DYNAMIC; } /* Put the child element in the correct place */ - if (!root->child || n < root->child->u.param - ps) - { - child->next = root->child; - root->child = child; - } - else - { - t_selelem *prev; - - prev = root->child; - while (prev->next && prev->next->u.param - ps >= n) - { - prev = prev->next; - } - child->next = prev->next; - prev->next = child; - } + place_child(root, child, param); return child; on_error: @@ -712,6 +752,7 @@ set_expr_value_store(t_selelem *sel, gmx_ana_selparam_t *param, int i) gmx_bug("internal error"); return FALSE; } + sel->v.nr = 1; sel->v.nalloc = -1; return TRUE; } @@ -1171,7 +1212,7 @@ _gmx_sel_parse_params(t_selexpr_param *pparams, int nparam, gmx_ana_selparam_t * } else { - rc = parse_values_varnum(pparam->nval, pparam->value, oparam); + rc = parse_values_varnum(pparam->nval, pparam->value, oparam, root); } } else if (oparam->flags & SPAR_ENUMVAL) diff --git a/src/gmxlib/selection/selelem.c b/src/gmxlib/selection/selelem.c index 311f5923d8..d886f5613b 100644 --- a/src/gmxlib/selection/selelem.c +++ b/src/gmxlib/selection/selelem.c @@ -200,12 +200,22 @@ _gmx_selelem_mempool_reserve(t_selelem *sel, int count) } switch (sel->v.type) { + case INT_VALUE: + rc = _gmx_sel_mempool_alloc(sel->mempool, (void **)&sel->v.u.i, + sizeof(*sel->v.u.i)*count); + break; + + case REAL_VALUE: + rc = _gmx_sel_mempool_alloc(sel->mempool, (void **)&sel->v.u.r, + sizeof(*sel->v.u.r)*count); + break; + case GROUP_VALUE: rc = _gmx_sel_mempool_alloc_group(sel->mempool, sel->v.u.g, count); break; default: - gmx_bug("mem pooling not implemented for non-group values"); + gmx_incons("mem pooling not implemented for requested type"); return -1; } return rc; @@ -226,16 +236,20 @@ _gmx_selelem_mempool_release(t_selelem *sel) } switch (sel->v.type) { + case INT_VALUE: + case REAL_VALUE: + _gmx_sel_mempool_free(sel->mempool, sel->v.u.ptr); + break; + case GROUP_VALUE: if (sel->v.u.g) { - _gmx_sel_mempool_free(sel->mempool, sel->v.u.g->index); - sel->v.u.g->index = NULL; + _gmx_sel_mempool_free_group(sel->mempool, sel->v.u.g); } break; default: - gmx_bug("mem pooling not implemented for non-group values"); + gmx_incons("mem pooling not implemented for requested type"); break; } } @@ -290,77 +304,76 @@ _gmx_selelem_free_values(t_selelem *sel) sfree(sel->v.u.ptr); } _gmx_selvalue_setstore(&sel->v, NULL); + if (sel->type == SEL_SUBEXPRREF && sel->u.param) + { + sel->u.param->val.u.ptr = NULL; + } } /*! * \param[in] method Method to free. * \param[in] mdata Method data to free. - * \param[in] bFreeParamData If TRUE, free also the values of parameters. */ void -_gmx_selelem_free_method(gmx_ana_selmethod_t *method, void *mdata, - bool bFreeParamData) +_gmx_selelem_free_method(gmx_ana_selmethod_t *method, void *mdata) { - /* Free method data */ - if (mdata) + sel_freefunc free_func = NULL; + + /* Save the pointer to the free function. */ + if (method && method->free) { - if (method && method->free) - { - method->free(mdata); - } - sfree(mdata); + free_func = method->free; } - /* Free the method itself */ + + /* Free the method itself. + * Has to be done before freeing the method data, because parameter + * values are typically stored in the method data, and here we may + * access them. */ if (method) { - int i; + int i, j; - /* If the method has not yet been initialized, we must free the - * memory allocated for parameter values here. */ - if (bFreeParamData) + /* Free the memory allocated for the parameters that are not managed + * by the selection method itself. */ + for (i = 0; i < method->nparams; ++i) { - for (i = 0; i < method->nparams; ++i) - { - gmx_ana_selparam_t *param = &method->param[i]; + gmx_ana_selparam_t *param = &method->param[i]; - if ((param->flags & (SPAR_VARNUM | SPAR_ATOMVAL)) - && param->val.type != GROUP_VALUE - && param->val.type != POS_VALUE) + if (param->val.u.ptr) + { + if (param->val.type == GROUP_VALUE) { - /* We don't need to check for enum values here, because - * SPAR_ENUMVAL cannot be combined with the flags - * required above. If it ever will be, this results - * in a double free within this function, which should - * be relatively easy to debug. - */ - if (param->val.type == STR_VALUE) + for (j = 0; j < param->val.nr; ++j) { - int j; - - for (j = 0; j < param->val.nr; ++j) - { - sfree(param->val.u.s[j]); - } + gmx_ana_index_deinit(¶m->val.u.g[j]); + } + } + else if (param->val.type == POS_VALUE) + { + for (j = 0; j < param->val.nr; ++j) + { + gmx_ana_pos_deinit(¶m->val.u.p[j]); } - sfree(param->val.u.ptr); } - } - } - /* And even if it is, the arrays allocated for enum values need - * to be freed. */ - for (i = 0; i < method->nparams; ++i) - { - gmx_ana_selparam_t *param = &method->param[i]; - if (param->flags & SPAR_ENUMVAL) - { - sfree(param->val.u.ptr); + if (param->val.nalloc > 0) + { + sfree(param->val.u.ptr); + } } - } sfree(method->param); sfree(method); } + /* Free method data. */ + if (mdata) + { + if (free_func) + { + free_func(mdata); + } + sfree(mdata); + } } /*! @@ -371,8 +384,7 @@ _gmx_selelem_free_exprdata(t_selelem *sel) { if (sel->type == SEL_EXPRESSION || sel->type == SEL_MODIFIER) { - _gmx_selelem_free_method(sel->u.expr.method, sel->u.expr.mdata, - !(sel->flags & SEL_METHODINIT)); + _gmx_selelem_free_method(sel->u.expr.method, sel->u.expr.mdata); sel->u.expr.mdata = NULL; sel->u.expr.method = NULL; /* Free position data */ @@ -410,8 +422,6 @@ _gmx_selelem_free_exprdata(t_selelem *sel) void _gmx_selelem_free(t_selelem *sel) { - t_selelem *child, *prev; - /* Decrement the reference counter and do nothing if references remain */ sel->refcount--; if (sel->refcount > 0) @@ -419,14 +429,10 @@ _gmx_selelem_free(t_selelem *sel) return; } - /* Free the children */ - child = sel->child; - while (child) - { - prev = child; - child = child->next; - _gmx_selelem_free(prev); - } + /* Free the children. + * Must be done before freeing other data, because the children may hold + * references to data in this element. */ + _gmx_selelem_free_chain(sel->child); /* Free value storage */ _gmx_selelem_free_values(sel); @@ -476,12 +482,14 @@ print_evaluation_func(FILE *fp, t_selelem *sel) fprintf(fp, "root"); else if (sel->evaluate == &_gmx_sel_evaluate_static) fprintf(fp, "static"); - else if (sel->evaluate == &_gmx_sel_evaluate_subexpr_pass) - fprintf(fp, "subexpr_pass"); + else if (sel->evaluate == &_gmx_sel_evaluate_subexpr_simple) + fprintf(fp, "subexpr_simple"); + else if (sel->evaluate == &_gmx_sel_evaluate_subexpr_staticeval) + fprintf(fp, "subexpr_staticeval"); else if (sel->evaluate == &_gmx_sel_evaluate_subexpr) fprintf(fp, "subexpr"); - else if (sel->evaluate == &_gmx_sel_evaluate_subexprref_pass) - fprintf(fp, "ref_pass"); + else if (sel->evaluate == &_gmx_sel_evaluate_subexprref_simple) + fprintf(fp, "ref_simple"); else if (sel->evaluate == &_gmx_sel_evaluate_subexprref) fprintf(fp, "ref"); else if (sel->evaluate == &_gmx_sel_evaluate_method) diff --git a/src/gmxlib/selection/selelem.h b/src/gmxlib/selection/selelem.h index 2b68f0a7d1..e49edbef72 100644 --- a/src/gmxlib/selection/selelem.h +++ b/src/gmxlib/selection/selelem.h @@ -332,8 +332,7 @@ extern void _gmx_selelem_free_values(t_selelem *sel); /** Frees the memory allocated for a selection method. */ extern void -_gmx_selelem_free_method(struct gmx_ana_selmethod_t *method, void *mdata, - bool bFreeParamData); +_gmx_selelem_free_method(struct gmx_ana_selmethod_t *method, void *mdata); /** Frees the memory allocated for the \c t_selelem::u field. */ extern void _gmx_selelem_free_exprdata(t_selelem *sel); diff --git a/src/gmxlib/selection/selmethod.c b/src/gmxlib/selection/selmethod.c index ab97c3bcb9..14482957b5 100644 --- a/src/gmxlib/selection/selmethod.c +++ b/src/gmxlib/selection/selmethod.c @@ -418,7 +418,6 @@ check_callbacks(FILE *fp, gmx_ana_selmethod_t *method) { bool bOk = TRUE; bool bNeedInit; - bool bNeedFree; int i; /* Make some checks on init_data and free */ @@ -455,19 +454,12 @@ check_callbacks(FILE *fp, gmx_ana_selmethod_t *method) /* Loop through the parameters to determine if initialization callbacks * are needed. */ bNeedInit = FALSE; - bNeedFree = FALSE; for (i = 0; i < method->nparams; ++i) { - if (method->param[i].val.type == POS_VALUE - || method->param[i].val.type == GROUP_VALUE) - { - bNeedFree = TRUE; - } if (method->param[i].val.type != POS_VALUE && (method->param[i].flags & (SPAR_VARNUM | SPAR_ATOMVAL))) { bNeedInit = TRUE; - bNeedFree = TRUE; } } /* Check that the callbacks required by the parameters are present */ @@ -476,11 +468,6 @@ check_callbacks(FILE *fp, gmx_ana_selmethod_t *method) report_error(fp, method->name, "error: init should be provided"); bOk = FALSE; } - if (bNeedFree && !method->free) - { - report_error(fp, method->name, "error: free should be provided"); - bOk = FALSE; - } return bOk; } diff --git a/src/gmxlib/selection/sm_compare.c b/src/gmxlib/selection/sm_compare.c index dfd796ae1a..54c932b457 100644 --- a/src/gmxlib/selection/sm_compare.c +++ b/src/gmxlib/selection/sm_compare.c @@ -61,6 +61,10 @@ typedef enum #define CMP_DYNAMICVAL 2 /** The value is real. */ #define CMP_REALVAL 4 +/** The integer array is allocated. */ +#define CMP_ALLOCINT 16 +/** The real array is allocated. */ +#define CMP_ALLOCREAL 32 /*! \internal \brief * Data structure for comparison expression operand values. @@ -325,7 +329,7 @@ convert_int_real(int n, t_compare_value *val) /* Free the previous value if one is present. */ sfree(val->r); val->r = rv; - val->flags |= CMP_REALVAL; + val->flags |= CMP_REALVAL | CMP_ALLOCREAL; } /* \brief @@ -379,6 +383,7 @@ convert_real_int(int n, t_compare_value *val, e_comparison_t cmpt, bool bRight) sfree(val->i); val->i = iv; val->flags &= ~CMP_REALVAL; + val->flags |= CMP_ALLOCINT; return 0; } @@ -470,10 +475,22 @@ free_data_compare(void *data) t_methoddata_compare *d = (t_methoddata_compare *)data; sfree(d->cmpop); - sfree(d->left.i); - sfree(d->left.r); - sfree(d->right.i); - sfree(d->right.r); + if (d->left.flags & CMP_ALLOCINT) + { + sfree(d->left.i); + } + if (d->left.flags & CMP_ALLOCREAL) + { + sfree(d->left.r); + } + if (d->right.flags & CMP_ALLOCINT) + { + sfree(d->right.i); + } + if (d->right.flags & CMP_ALLOCREAL) + { + sfree(d->right.r); + } } /*! diff --git a/src/gmxlib/selection/sm_distance.c b/src/gmxlib/selection/sm_distance.c index 0a669539fa..0846650e71 100644 --- a/src/gmxlib/selection/sm_distance.c +++ b/src/gmxlib/selection/sm_distance.c @@ -230,7 +230,6 @@ free_data_common(void *data) { t_methoddata_distance *d = (t_methoddata_distance *)data; - gmx_ana_pos_deinit(&d->p); gmx_ana_nbsearch_free(d->nb); } diff --git a/src/gmxlib/selection/sm_insolidangle.c b/src/gmxlib/selection/sm_insolidangle.c index 55ac6334e9..bc5ab8e611 100644 --- a/src/gmxlib/selection/sm_insolidangle.c +++ b/src/gmxlib/selection/sm_insolidangle.c @@ -388,8 +388,6 @@ free_data_insolidangle(void *data) t_methoddata_insolidangle *d = (t_methoddata_insolidangle *)data; int i; - gmx_ana_pos_deinit(&d->center); - gmx_ana_pos_deinit(&d->span); if (d->tbin) { for (i = 0; i < d->ntbins; ++i) diff --git a/src/gmxlib/selection/sm_keywords.c b/src/gmxlib/selection/sm_keywords.c index e5a89dc094..109027d718 100644 --- a/src/gmxlib/selection/sm_keywords.c +++ b/src/gmxlib/selection/sm_keywords.c @@ -71,12 +71,6 @@ init_kwreal(t_topology *top, int npar, gmx_ana_selparam_t *param, void *data); /** Initializes data for string keyword evaluation. */ static int init_kwstr(t_topology *top, int npar, gmx_ana_selparam_t *param, void *data); -/** Frees the memory allocated for integer keyword evaluation. */ -static void -free_data_kwint(void *data); -/** Frees the memory allocated for real keyword evaluation. */ -static void -free_data_kwreal(void *data); /** Frees the memory allocated for string keyword evaluation. */ static void free_data_kwstr(void *data); @@ -184,7 +178,7 @@ gmx_ana_selmethod_t sm_keyword_int = { NULL, &init_kwint, NULL, - &free_data_kwint, + NULL, NULL, &evaluate_keyword_int, NULL, @@ -199,7 +193,7 @@ gmx_ana_selmethod_t sm_keyword_real = { NULL, &init_kwreal, NULL, - &free_data_kwreal, + NULL, NULL, &evaluate_keyword_real, NULL, @@ -296,20 +290,6 @@ init_kwint(t_topology *top, int npar, gmx_ana_selparam_t *param, void *data) } /*! - * \param data Data to free (should point to a \ref t_methoddata_kwint). - * - * Frees the memory allocated for t_methoddata_kwint::r. - */ -static void -free_data_kwint(void *data) -{ - t_methoddata_kwint *d = (t_methoddata_kwint *)data; - - sfree(d->v); - sfree(d->r); -} - -/*! * See sel_updatefunc() for description of the parameters. * \p data should point to a \c t_methoddata_kwint. * @@ -401,20 +381,6 @@ init_kwreal(t_topology *top, int npar, gmx_ana_selparam_t *param, void *data) } /*! - * \param data Data to free (should point to a \ref t_methoddata_kwreal). - * - * Frees the memory allocated for t_methoddata_kwreal::r. - */ -static void -free_data_kwreal(void *data) -{ - t_methoddata_kwreal *d = (t_methoddata_kwreal *)data; - - sfree(d->v); - sfree(d->r); -} - -/*! * See sel_updatefunc() for description of the parameters. * \p data should point to a \c t_methoddata_kwreal. * @@ -535,10 +501,6 @@ init_kwstr(t_topology *top, int npar, gmx_ana_selparam_t *param, void *data) fprintf(stderr, "WARNING: error in regular expression,\n" " will match '%s' as a simple string\n", s); } - else - { - sfree(s); - } sfree(buf); #else bRegExp = FALSE; @@ -552,7 +514,6 @@ init_kwstr(t_topology *top, int npar, gmx_ana_selparam_t *param, void *data) } d->m[i].bRegExp = bRegExp; } - sfree(param[1].val.u.s); return 0; } @@ -567,7 +528,6 @@ free_data_kwstr(void *data) t_methoddata_kwstr *d = (t_methoddata_kwstr *)data; int i; - sfree(d->v); for (i = 0; i < d->n; ++i) { if (d->m[i].bRegExp) @@ -578,10 +538,6 @@ free_data_kwstr(void *data) regfree(&d->m[i].u.r); #endif } - else - { - sfree(d->m[i].u.s); - } } sfree(d->m); } @@ -683,8 +639,7 @@ free_data_kweval(void *data) { t_methoddata_kweval *d = (t_methoddata_kweval *)data; - _gmx_selelem_free_method(d->kwmethod, d->kwmdata, FALSE); - gmx_ana_index_deinit(&d->g); + _gmx_selelem_free_method(d->kwmethod, d->kwmdata); } /*! diff --git a/src/gmxlib/selection/sm_merge.c b/src/gmxlib/selection/sm_merge.c index e3ac612759..34d2b0d740 100644 --- a/src/gmxlib/selection/sm_merge.c +++ b/src/gmxlib/selection/sm_merge.c @@ -299,8 +299,6 @@ free_data_merge(void *data) { t_methoddata_merge *d = (t_methoddata_merge *)data; - gmx_ana_pos_deinit(&d->p1); - gmx_ana_pos_deinit(&d->p2); gmx_ana_index_deinit(&d->g); } diff --git a/src/gmxlib/selection/sm_permute.c b/src/gmxlib/selection/sm_permute.c index 16ec41edf5..a318912eb2 100644 --- a/src/gmxlib/selection/sm_permute.c +++ b/src/gmxlib/selection/sm_permute.c @@ -216,9 +216,7 @@ free_data_permute(void *data) { t_methoddata_permute *d = (t_methoddata_permute *)data; - gmx_ana_pos_deinit(&d->p); gmx_ana_index_deinit(&d->g); - sfree(d->perm); sfree(d->rperm); } diff --git a/src/gmxlib/selection/sm_position.c b/src/gmxlib/selection/sm_position.c index 2ba8f56154..a58d1854cf 100644 --- a/src/gmxlib/selection/sm_position.c +++ b/src/gmxlib/selection/sm_position.c @@ -352,7 +352,6 @@ free_data_pos(void *data) t_methoddata_pos *d = (t_methoddata_pos *)data; sfree(d->type); - gmx_ana_index_deinit(&d->g); gmx_ana_poscalc_free(d->pc); } diff --git a/src/gmxlib/selection/sm_same.c b/src/gmxlib/selection/sm_same.c index 37b2029ce1..f7b881394d 100644 --- a/src/gmxlib/selection/sm_same.c +++ b/src/gmxlib/selection/sm_same.c @@ -287,8 +287,6 @@ free_data_same(void *data) { t_methoddata_same *d = (t_methoddata_same *)data; - sfree(d->val.ptr); - sfree(d->as.ptr); sfree(d->as_s_sorted); } -- 2.11.4.GIT