From 193050de8781265f27b91dc0b17e888d441cd3ed Mon Sep 17 00:00:00 2001 From: Aurelien Bouteiller Date: Tue, 23 Apr 2024 17:38:24 -0400 Subject: [PATCH] bugfix: handle the case where the adt is persistent --- parsec/data.c | 21 +++++++++++++++---- parsec/data_dist/matrix/matrix.h | 7 ++++--- parsec/interfaces/ptg/ptg-compiler/jdf2c.c | 5 ++++- tests/apps/haar_tree/main.c | 13 +++++------- tests/collections/reshape/common.h | 3 +++ .../reshape/testing_avoidable_reshape.c | 1 - ...ting_remote_multiple_outs_same_pred_flow.c | 3 --- tests/collections/reshape/testing_reshape.c | 12 ----------- tests/dsl/ptg/complex_deps.jdf | 11 +++++----- tests/dsl/ptg/startup.jdf | 14 ++++++------- 10 files changed, 44 insertions(+), 46 deletions(-) diff --git a/parsec/data.c b/parsec/data.c index 48013b68f..d5892cd6e 100644 --- a/parsec/data.c +++ b/parsec/data.c @@ -568,7 +568,8 @@ parsec_data_destroy( parsec_data_t *data ) /* adt constructor is split in two stages: when the adt is constructed * with OBJ_CONSTRUCT (or allocated with OBJ_NEW), we initialize the - * adt object. The user code must then call the parameterized constructor + * adt object with static zero values. + * The user code must then call the parameterized constructor * (below) that attaches the datatype and creates the associated arena. * * When the adt object is destructed (or released), the implicit destructor @@ -583,11 +584,23 @@ int parsec_arena_datatype_construct(parsec_arena_datatype_t *adt, adt->arena = PARSEC_OBJ_NEW(parsec_arena_t); parsec_arena_construct(adt->arena, elem_size, alignment); + adt->opaque_dtt = opaque_dtt; + return PARSEC_SUCCESS; +} + +/* This constructor must only initialize fields with static values. + * When the user is using a persistent adt, the PTG generator will + * construct_zero the static arenas, and the user will overwrite them + * without calling destruct, which is valid only if this function does + * not allocate memory. + */ +static void parsec_arena_datatype_construct_zero(parsec_object_t *obj) { + parsec_arena_datatype_t *adt = (parsec_arena_datatype_t *)obj; + adt->arena = NULL; adt->ht_item.next_item = NULL; /* keep Coverity happy */ adt->ht_item.hash64 = 0; /* keep Coverity happy */ adt->ht_item.key = 0; /* keep Coverity happy */ - adt->opaque_dtt = opaque_dtt; - return PARSEC_SUCCESS; + adt->opaque_dtt = NULL; } static void parsec_arena_datatype_destruct(parsec_object_t *obj) { @@ -596,5 +609,5 @@ static void parsec_arena_datatype_destruct(parsec_object_t *obj) { } PARSEC_OBJ_CLASS_INSTANCE(parsec_arena_datatype_t, parsec_object_t, - NULL, + parsec_arena_datatype_construct_zero, parsec_arena_datatype_destruct); diff --git a/parsec/data_dist/matrix/matrix.h b/parsec/data_dist/matrix/matrix.h index 1f6794d50..bc796b797 100644 --- a/parsec/data_dist/matrix/matrix.h +++ b/parsec/data_dist/matrix/matrix.h @@ -366,9 +366,10 @@ int parsec_matrix_arena_datatype_destruct_free_type(parsec_arena_datatype_t *adt parsec_matrix_arena_datatype_construct_alloc_type( (_adt_), (_oldtype_), PARSEC_MATRIX_FULL, 0, (_m_), (_n_), (_ld_), PARSEC_ARENA_ALIGNMENT_SSE, -1 ), \ (_adt_)) -#define parsec_matrix_adt_free(_adt_) do {\ - parsec_matrix_arena_datatype_destruct_free_type(_adt_); \ - free(_adt_); \ +#define parsec_matrix_adt_free(_padt_) do {\ + parsec_matrix_arena_datatype_destruct_free_type(*(_padt_)); \ + free(*(_padt_)); \ + (*_padt_) = NULL; \ } while(0) diff --git a/parsec/interfaces/ptg/ptg-compiler/jdf2c.c b/parsec/interfaces/ptg/ptg-compiler/jdf2c.c index bb7e41b53..f017b0447 100644 --- a/parsec/interfaces/ptg/ptg-compiler/jdf2c.c +++ b/parsec/interfaces/ptg/ptg-compiler/jdf2c.c @@ -4758,7 +4758,10 @@ static void jdf_generate_constructor( const jdf_t* jdf ) * stages, one in the generated code runs the generic constructor, then * the user code calls the parameterized constructor (that attaches the * datatype). Thus, for symmetry the user code is also responsible for calling - * the destructor. */ + * the destructor. + * The statically constructed arena_datatype_t can be overwritten by + * the user without leaking memory. + */ coutput(" for(i = 0; i < (uint32_t)__parsec_tp->super.arenas_datatypes_size; i++) {\n" " PARSEC_OBJ_CONSTRUCT(&__parsec_tp->super.arenas_datatypes[i], parsec_arena_datatype_t);\n" " }\n"); diff --git a/tests/apps/haar_tree/main.c b/tests/apps/haar_tree/main.c index 381b2c426..bf37f39bf 100644 --- a/tests/apps/haar_tree/main.c +++ b/tests/apps/haar_tree/main.c @@ -143,7 +143,7 @@ int main(int argc, char *argv[]) parsec_matrix_block_cyclic_t fakeDesc; parsec_project_taskpool_t *project; parsec_walk_taskpool_t *walker; - parsec_arena_datatype_t adt; + parsec_arena_datatype_t *adt = PARSEC_OBJ_NEW(parsec_arena_datatype_t); int do_checks = 0, be_verbose = 0; int pargc = 0, i; char **pargv; @@ -216,7 +216,7 @@ int main(int argc, char *argv[]) 0, 0, world, world, 1, world, 1, 1, 0, 0); - parsec_matrix_adt_construct_rect( &adt, + parsec_matrix_adt_construct_rect( adt, parsec_datatype_float_t, 2, 1, 2); #if defined(HAVE_MPI) @@ -224,8 +224,7 @@ int main(int argc, char *argv[]) #endif project = parsec_project_new(treeA, world, (parsec_data_collection_t*)&fakeDesc, 1e-3, be_verbose, 1.0); - project->arenas_datatypes[PARSEC_project_DEFAULT_ADT_IDX] = adt; - PARSEC_OBJ_RETAIN(adt.arena); + project->arenas_datatypes[PARSEC_project_DEFAULT_ADT_IDX] = *adt; rc = parsec_context_add_taskpool(parsec, &project->super); PARSEC_CHECK_ERROR(rc, "parsec_context_add_taskpool"); rc = parsec_context_start(parsec); @@ -233,7 +232,6 @@ int main(int argc, char *argv[]) rc = parsec_context_wait(parsec); PARSEC_CHECK_ERROR(rc, "parsec_context_wait"); - project->arenas_datatypes[PARSEC_project_DEFAULT_ADT_IDX].arena = NULL; parsec_taskpool_free(&project->super); ret = 0; @@ -247,7 +245,7 @@ int main(int argc, char *argv[]) rs, print_node_fn, print_link_fn, be_verbose); } - walker->arenas_datatypes[PARSEC_walk_DEFAULT_ADT_IDX] = adt; + walker->arenas_datatypes[PARSEC_walk_DEFAULT_ADT_IDX] = *adt; rc = parsec_context_add_taskpool(parsec, &walker->super); PARSEC_CHECK_ERROR(rc, "parsec_context_add_taskpool"); rc = parsec_context_start(parsec); @@ -291,9 +289,8 @@ int main(int argc, char *argv[]) } } #endif /* defined(HAVE_MPI) */ - parsec_matrix_arena_datatype_destruct_free_type( & adt ); + parsec_matrix_adt_free( &adt ); - walker->arenas_datatypes[PARSEC_walk_DEFAULT_ADT_IDX].arena = NULL; parsec_taskpool_free(&walker->super); tree_dist_free(treeA); diff --git a/tests/collections/reshape/common.h b/tests/collections/reshape/common.h index 2b0498afa..ec8972169 100644 --- a/tests/collections/reshape/common.h +++ b/tests/collections/reshape/common.h @@ -131,6 +131,9 @@ int reshape_print(parsec_execution_stream_t *es, parsec_arena_datatype_t adt_default; \ parsec_arena_datatype_t adt_lower; \ parsec_arena_datatype_t adt_upper; \ + PARSEC_OBJ_CONSTRUCT(&adt_default, parsec_arena_datatype_t); \ + PARSEC_OBJ_CONSTRUCT(&adt_lower, parsec_arena_datatype_t); \ + PARSEC_OBJ_CONSTRUCT(&adt_upper, parsec_arena_datatype_t); \ parsec_matrix_adt_construct_rect( &adt_default, \ parsec_datatype_int_t, MB, NB, MB ); \ parsec_matrix_adt_construct_lower( &adt_lower, \ diff --git a/tests/collections/reshape/testing_avoidable_reshape.c b/tests/collections/reshape/testing_avoidable_reshape.c index 365eaa7f9..39ca3f221 100644 --- a/tests/collections/reshape/testing_avoidable_reshape.c +++ b/tests/collections/reshape/testing_avoidable_reshape.c @@ -92,7 +92,6 @@ int main(int argc, char *argv[]) parsec_datatype_t tmp = adt_default.opaque_dtt; ctp->arenas_datatypes[PARSEC_avoidable_reshape_DEFAULT_ADT_IDX].opaque_dtt = dcA.super.super.default_dtt; #endif - PARSEC_OBJ_RETAIN(adt_default.arena); DO_RUN(ctp); DO_CHECK(avoidable_reshape, dcA, dcA_check); diff --git a/tests/collections/reshape/testing_remote_multiple_outs_same_pred_flow.c b/tests/collections/reshape/testing_remote_multiple_outs_same_pred_flow.c index 5366cb91b..baed1066e 100644 --- a/tests/collections/reshape/testing_remote_multiple_outs_same_pred_flow.c +++ b/tests/collections/reshape/testing_remote_multiple_outs_same_pred_flow.c @@ -116,9 +116,6 @@ int main(int argc, char *argv[]) ctp->arenas_datatypes[PARSEC_remote_multiple_outs_same_pred_flow_DEFAULT_ADT_IDX] = adt_default; ctp->arenas_datatypes[PARSEC_remote_multiple_outs_same_pred_flow_LOWER_TILE_ADT_IDX] = adt_lower; ctp->arenas_datatypes[PARSEC_remote_multiple_outs_same_pred_flow_UPPER_TILE_ADT_IDX] = adt_upper; - PARSEC_OBJ_RETAIN(adt_default.arena); - PARSEC_OBJ_RETAIN(adt_lower.arena); - PARSEC_OBJ_RETAIN(adt_upper.arena); DO_RUN(ctp); DO_CHECK(remote_multiple_outs_same_pred_flow, dcA, dcA_check); diff --git a/tests/collections/reshape/testing_reshape.c b/tests/collections/reshape/testing_reshape.c index 9f2433507..b5c9d48ca 100644 --- a/tests/collections/reshape/testing_reshape.c +++ b/tests/collections/reshape/testing_reshape.c @@ -92,8 +92,6 @@ int main(int argc, char *argv[]) ctp->arenas_datatypes[PARSEC_local_no_reshape_DEFAULT_ADT_IDX] = adt_default; ctp->arenas_datatypes[PARSEC_local_no_reshape_LOWER_TILE_ADT_IDX] = adt_lower; - PARSEC_OBJ_RETAIN(adt_default.arena); - PARSEC_OBJ_RETAIN(adt_lower.arena); DO_RUN(ctp); DO_CHECK(local_no_reshape, dcA, dcA_check); @@ -124,8 +122,6 @@ int main(int argc, char *argv[]) ctp->arenas_datatypes[PARSEC_local_read_reshape_DEFAULT_ADT_IDX] = adt_default; ctp->arenas_datatypes[PARSEC_local_read_reshape_LOWER_TILE_ADT_IDX] = adt_lower; - PARSEC_OBJ_RETAIN(adt_default.arena); - PARSEC_OBJ_RETAIN(adt_lower.arena); DO_RUN(ctp); DO_CHECK(local_read_reshape, dcA, dcA_check); @@ -157,8 +153,6 @@ int main(int argc, char *argv[]) ctp->arenas_datatypes[PARSEC_local_output_reshape_DEFAULT_ADT_IDX] = adt_default; ctp->arenas_datatypes[PARSEC_local_output_reshape_LOWER_TILE_ADT_IDX] = adt_lower; - PARSEC_OBJ_RETAIN(adt_default.arena); - PARSEC_OBJ_RETAIN(adt_lower.arena); DO_RUN(ctp); DO_CHECK(local_output_reshape, dcA, dcA_check); @@ -190,8 +184,6 @@ int main(int argc, char *argv[]) ctp->arenas_datatypes[PARSEC_local_input_reshape_DEFAULT_ADT_IDX] = adt_default; ctp->arenas_datatypes[PARSEC_local_input_reshape_LOWER_TILE_ADT_IDX] = adt_lower; - PARSEC_OBJ_RETAIN(adt_default.arena); - PARSEC_OBJ_RETAIN(adt_lower.arena); DO_RUN(ctp); DO_CHECK(local_input_reshape, dcA, dcA_check); @@ -222,8 +214,6 @@ int main(int argc, char *argv[]) ctp->arenas_datatypes[PARSEC_remote_read_reshape_DEFAULT_ADT_IDX] = adt_default; ctp->arenas_datatypes[PARSEC_remote_read_reshape_LOWER_TILE_ADT_IDX] = adt_lower; - PARSEC_OBJ_RETAIN(adt_default.arena); - PARSEC_OBJ_RETAIN(adt_lower.arena); DO_RUN(ctp); DO_CHECK(remote_read_reshape, dcA, dcA_check); @@ -252,8 +242,6 @@ int main(int argc, char *argv[]) ctp->arenas_datatypes[PARSEC_remote_no_re_reshape_DEFAULT_ADT_IDX] = adt_default; ctp->arenas_datatypes[PARSEC_remote_no_re_reshape_LOWER_TILE_ADT_IDX] = adt_lower; - PARSEC_OBJ_RETAIN(adt_default.arena); - PARSEC_OBJ_RETAIN(adt_lower.arena); DO_RUN(ctp); DO_CHECK(remote_no_re_reshape, dcA, dcA_check); diff --git a/tests/dsl/ptg/complex_deps.jdf b/tests/dsl/ptg/complex_deps.jdf index d47f2ac2e..df31fe438 100644 --- a/tests/dsl/ptg/complex_deps.jdf +++ b/tests/dsl/ptg/complex_deps.jdf @@ -111,7 +111,7 @@ int main( int argc, char** argv ) { parsec_complex_deps_taskpool_t* tp; parsec_matrix_block_cyclic_t descA; - parsec_arena_datatype_t adt; + parsec_arena_datatype_t *adt = PARSEC_OBJ_NEW(parsec_arena_datatype_t); parsec_datatype_t otype; parsec_context_t *parsec; int ni = NN, nk = NN, verbose = 0, i = 1, rc; @@ -167,15 +167,14 @@ int main( int argc, char** argv ) descA.super.bsiz * parsec_datadist_getsizeoftype(TYPE) ); parsec_translate_matrix_type(TYPE, &otype); - parsec_matrix_adt_construct_rect(&adt, otype, + parsec_matrix_adt_construct_rect(adt, otype, descA.super.mb, descA.super.nb, descA.super.mb); /* Heat up the engine: small tasks */ tp = parsec_complex_deps_new( &descA, ni, nk ); assert( NULL != tp ); - tp->arenas_datatypes[PARSEC_complex_deps_DEFAULT_ADT_IDX] = adt; - PARSEC_OBJ_RETAIN(adt.arena); + tp->arenas_datatypes[PARSEC_complex_deps_DEFAULT_ADT_IDX] = *adt; rc = parsec_context_add_taskpool( parsec, (parsec_taskpool_t*)tp ); PARSEC_CHECK_ERROR(rc, "parsec_context_add_taskpool"); @@ -191,7 +190,7 @@ int main( int argc, char** argv ) tp = parsec_complex_deps_new( &descA, ni, nk ); assert( NULL != tp ); TIMER_START(time_elapsed); - tp->arenas_datatypes[PARSEC_complex_deps_DEFAULT_ADT_IDX] = adt; + tp->arenas_datatypes[PARSEC_complex_deps_DEFAULT_ADT_IDX] = *adt; rc = parsec_context_add_taskpool( parsec, (parsec_taskpool_t*)tp ); PARSEC_CHECK_ERROR(rc, "parsec_context_add_taskpool"); TIMER_STOP(time_elapsed); @@ -213,7 +212,7 @@ int main( int argc, char** argv ) TIMER_STOP(time_elapsed); printf("DAG execution in %ld micro-sec\n", time_elapsed); - parsec_matrix_arena_datatype_destruct_free_type( & adt ); + parsec_matrix_adt_free( & adt ); free(descA.mat); diff --git a/tests/dsl/ptg/startup.jdf b/tests/dsl/ptg/startup.jdf index 0f7651210..ef42d5d36 100644 --- a/tests/dsl/ptg/startup.jdf +++ b/tests/dsl/ptg/startup.jdf @@ -90,7 +90,7 @@ int main( int argc, char** argv ) { parsec_startup_taskpool_t* tp; parsec_matrix_block_cyclic_t descA; - parsec_arena_datatype_t adt; + parsec_arena_datatype_t *adt = PARSEC_OBJ_NEW(parsec_arena_datatype_t); parsec_datatype_t dt; parsec_context_t *parsec; int ni = NN, nj = NN, nk = NN, verbose = 0, i = 1, rc; @@ -145,7 +145,7 @@ int main( int argc, char** argv ) parsec_datadist_getsizeoftype(TYPE) ); parsec_translate_matrix_type(TYPE, &dt); - parsec_matrix_adt_construct_rect(&adt, dt, + parsec_matrix_adt_construct_rect(adt, dt, descA.super.mb, descA.super.nb, descA.super.mb); srandom((int)getpid()); /* Start the random generator */ @@ -157,10 +157,9 @@ int main( int argc, char** argv ) /* Heat up the engine: small tasks no priority */ tp = parsec_startup_new( &descA, ni, nj, nk ); assert( NULL != tp ); - tp->arenas_datatypes[PARSEC_startup_DEFAULT_ADT_IDX] = adt; - PARSEC_OBJ_RETAIN(adt.arena); - + tp->arenas_datatypes[PARSEC_startup_DEFAULT_ADT_IDX] = *adt; tp->_g_pri = 0; + rc = parsec_context_add_taskpool( parsec, (parsec_taskpool_t*)tp ); PARSEC_CHECK_ERROR(rc, "parsec_context_add_taskpool"); rc = parsec_context_wait(parsec); @@ -177,8 +176,7 @@ int main( int argc, char** argv ) assert( NULL != tp ); TIMER_START(time_elapsed); - tp->arenas_datatypes[PARSEC_startup_DEFAULT_ADT_IDX] = adt; - PARSEC_OBJ_RETAIN(adt.arena); + tp->arenas_datatypes[PARSEC_startup_DEFAULT_ADT_IDX] = *adt; tp->_g_pri = priorities[i].prio; rc = parsec_context_add_taskpool( parsec, (parsec_taskpool_t*)tp ); @@ -203,7 +201,7 @@ int main( int argc, char** argv ) } free(descA.mat); - parsec_matrix_arena_datatype_destruct_free_type( & adt ); + parsec_matrix_adt_free(&adt); parsec_fini( &parsec);