From 01caa86d7f7077a0bf118331ba37853c3cf91ca3 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 24 Feb 2020 14:36:03 -0500 Subject: [PATCH 1/3] Reintroduce logic for freeing memory on run-time errors --- src/execution_plan/ops/op_project.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/execution_plan/ops/op_project.c b/src/execution_plan/ops/op_project.c index 2594ee5c31..069902eb9b 100644 --- a/src/execution_plan/ops/op_project.c +++ b/src/execution_plan/ops/op_project.c @@ -52,6 +52,9 @@ static Record ProjectConsume(OpBase *opBase) { } Record projection = OpBase_CreateRecord(opBase); + // Track the inherited Record and the newly-allocated Record so that they may be freed if execution fails. + OpBase_AddVolatileRecord(opBase, r); + OpBase_AddVolatileRecord(opBase, projection); for(uint i = 0; i < op->exp_count; i++) { AR_ExpNode *exp = op->exps[i]; @@ -66,6 +69,7 @@ static Record ProjectConsume(OpBase *opBase) { Record_Add(projection, rec_idx, v); } + OpBase_RemoveVolatileRecords(opBase); // No exceptions encountered, Records are not dangling. OpBase_DeleteRecord(r); return projection; } From d2684654987e2d71ec00bea89fe5af128c45d580 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 26 Feb 2020 16:37:14 -0500 Subject: [PATCH 2/3] Remove VolatileRecord logic for freeing after run-time errors --- src/execution_plan/ops/op.c | 20 -------------------- src/execution_plan/ops/op.h | 8 -------- src/execution_plan/ops/op_project.c | 4 ---- src/execution_plan/ops/op_update.c | 3 --- 4 files changed, 35 deletions(-) diff --git a/src/execution_plan/ops/op.c b/src/execution_plan/ops/op.c index dd11bcc53e..035e5c74db 100644 --- a/src/execution_plan/ops/op.c +++ b/src/execution_plan/ops/op.c @@ -29,7 +29,6 @@ void OpBase_Init(OpBase *op, OPType type, const char *name, fpInit init, fpConsu op->parent = NULL; op->stats = NULL; op->op_initialized = false; - op->dangling_records = NULL; op->modifies = NULL; op->writer = writer; @@ -126,17 +125,6 @@ Record OpBase_Profile(OpBase *op) { return r; } -void OpBase_AddVolatileRecord(OpBase *op, const Record r) { - if(op->dangling_records == NULL) op->dangling_records = array_new(Record, 1); - op->dangling_records = array_append(op->dangling_records, r); -} - -void OpBase_RemoveVolatileRecords(OpBase *op) { - if(!op->dangling_records) return; - - array_clear(op->dangling_records); -} - bool OpBase_IsWriter(OpBase *op) { return op->writer; } @@ -166,14 +154,6 @@ void OpBase_Free(OpBase *op) { if(op->children) rm_free(op->children); if(op->modifies) array_free(op->modifies); if(op->stats) rm_free(op->stats); - // If we are storing dangling references to Records, free them now. - if(op->dangling_records) { - uint count = array_len(op->dangling_records); - for(uint i = 0; i < count; i ++) { - OpBase_DeleteRecord(op->dangling_records[i]); - } - array_free(op->dangling_records); - } rm_free(op); } diff --git a/src/execution_plan/ops/op.h b/src/execution_plan/ops/op.h index 12fa0d1e23..60269f1c9f 100644 --- a/src/execution_plan/ops/op.h +++ b/src/execution_plan/ops/op.h @@ -93,7 +93,6 @@ struct OpBase { struct OpBase **children; // Child operations. const char **modifies; // List of entities this op modifies. OpStats *stats; // Profiling statistics. - Record *dangling_records; // Records allocated by this operation that must be freed. struct OpBase *parent; // Parent operations. const struct ExecutionPlan *plan; // ExecutionPlan this operation is part of. bool writer; // Indicates this is a writer operation. @@ -112,13 +111,6 @@ int OpBase_ToString(const OpBase *op, char *buff, uint buff_len); OpBase *OpBase_Clone(const struct ExecutionPlan *plan, const OpBase *op); -/* If an operation holds the sole reference to a Record it is evaluating, - * that reference should be tracked so that it may be freed in the event of a run-time error. */ -void OpBase_AddVolatileRecord(OpBase *op, const Record r); -/* No errors were encountered while processing these Records; the references - * may be released. */ -void OpBase_RemoveVolatileRecords(OpBase *op); - /* Mark alias as being modified by operation. * Returns the ID associated with alias. */ int OpBase_Modifies(OpBase *op, const char *alias); diff --git a/src/execution_plan/ops/op_project.c b/src/execution_plan/ops/op_project.c index 069902eb9b..2594ee5c31 100644 --- a/src/execution_plan/ops/op_project.c +++ b/src/execution_plan/ops/op_project.c @@ -52,9 +52,6 @@ static Record ProjectConsume(OpBase *opBase) { } Record projection = OpBase_CreateRecord(opBase); - // Track the inherited Record and the newly-allocated Record so that they may be freed if execution fails. - OpBase_AddVolatileRecord(opBase, r); - OpBase_AddVolatileRecord(opBase, projection); for(uint i = 0; i < op->exp_count; i++) { AR_ExpNode *exp = op->exps[i]; @@ -69,7 +66,6 @@ static Record ProjectConsume(OpBase *opBase) { Record_Add(projection, rec_idx, v); } - OpBase_RemoveVolatileRecords(opBase); // No exceptions encountered, Records are not dangling. OpBase_DeleteRecord(r); return projection; } diff --git a/src/execution_plan/ops/op_update.c b/src/execution_plan/ops/op_update.c index cb97af544f..541db38d9f 100644 --- a/src/execution_plan/ops/op_update.c +++ b/src/execution_plan/ops/op_update.c @@ -178,8 +178,6 @@ static Record UpdateConsume(OpBase *opBase) { while((r = OpBase_Consume(child))) { /* Evaluate each update expression and store result * for later execution. */ - // Track the inherited Record and the newly-allocated Record so that they may be freed if execution fails. - OpBase_AddVolatileRecord(opBase, r); EntityUpdateEvalCtx *update_expression = op->update_expressions; for(uint i = 0; i < op->update_expressions_count; i++, update_expression++) { SIValue new_value = SI_CloneValue(AR_EXP_Evaluate(update_expression->exp, r)); @@ -198,7 +196,6 @@ static Record UpdateConsume(OpBase *opBase) { // Record not going to be used, discard. OpBase_DeleteRecord(r); } - OpBase_RemoveVolatileRecords(opBase); // No exceptions encountered, Records are not dangling. } /* Done reading, we're not going to call consume any longer From 2cc6e204c1fdc6892405a976b70ab284d6f754c8 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 26 Feb 2020 17:00:02 -0500 Subject: [PATCH 3/3] Fix memory leaks on run-time errors in OpProject --- src/execution_plan/ops/op_project.c | 32 +++++++++++++++++++++-------- src/execution_plan/ops/op_project.h | 2 ++ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/execution_plan/ops/op_project.c b/src/execution_plan/ops/op_project.c index 2594ee5c31..435d41a7f3 100644 --- a/src/execution_plan/ops/op_project.c +++ b/src/execution_plan/ops/op_project.c @@ -20,6 +20,8 @@ OpBase *NewProjectOp(const ExecutionPlan *plan, AR_ExpNode **exps) { op->singleResponse = false; op->exp_count = array_len(exps); op->record_offsets = array_new(uint, op->exp_count); + op->r = NULL; + op->projection = NULL; // Set our Op operations OpBase_Init((OpBase *)op, OPType_PROJECT, "Project", NULL, ProjectConsume, @@ -37,25 +39,24 @@ OpBase *NewProjectOp(const ExecutionPlan *plan, AR_ExpNode **exps) { static Record ProjectConsume(OpBase *opBase) { OpProject *op = (OpProject *)opBase; - Record r = NULL; if(op->op.childCount) { OpBase *child = op->op.children[0]; - r = OpBase_Consume(child); - if(!r) return NULL; + op->r = OpBase_Consume(child); + if(!op->r) return NULL; } else { // QUERY: RETURN 1+2 // Return a single record followed by NULL on the second call. if(op->singleResponse) return NULL; op->singleResponse = true; - r = OpBase_CreateRecord(opBase); + op->r = OpBase_CreateRecord(opBase); } - Record projection = OpBase_CreateRecord(opBase); + op->projection = OpBase_CreateRecord(opBase); for(uint i = 0; i < op->exp_count; i++) { AR_ExpNode *exp = op->exps[i]; - SIValue v = AR_EXP_Evaluate(exp, r); + SIValue v = AR_EXP_Evaluate(exp, op->r); int rec_idx = op->record_offsets[i]; /* Persisting a value is only necessary here if 'v' refers to a scalar held in Record 'r'. * Graph entities don't need to be persisted here as Record_Add will copy them internally. @@ -63,10 +64,15 @@ static Record ProjectConsume(OpBase *opBase) { * MATCH (a) WITH toUpper(a.name) AS e RETURN e * TODO This is a rare case; the logic of when to persist can be improved. */ if(!(v.type & SI_GRAPHENTITY)) SIValue_Persist(&v); - Record_Add(projection, rec_idx, v); + Record_Add(op->projection, rec_idx, v); } - OpBase_DeleteRecord(r); + OpBase_DeleteRecord(op->r); + op->r = NULL; + + // Emit the projected Record once. + Record projection = op->projection; + op->projection = NULL; return projection; } @@ -83,5 +89,15 @@ static void ProjectFree(OpBase *ctx) { array_free(op->record_offsets); op->record_offsets = NULL; } + + if(op->r) { + OpBase_DeleteRecord(op->r); + op->r = NULL; + } + + if(op->projection) { + OpBase_DeleteRecord(op->projection); + op->projection = NULL; + } } diff --git a/src/execution_plan/ops/op_project.h b/src/execution_plan/ops/op_project.h index 79fe5fc728..69ca44d23e 100644 --- a/src/execution_plan/ops/op_project.h +++ b/src/execution_plan/ops/op_project.h @@ -12,6 +12,8 @@ typedef struct { OpBase op; + Record r; // Input Record being read from (stored to free if we encounter an error). + Record projection; // Record projected by this operation (stored to free if we encounter an error). AR_ExpNode **exps; // Projected expressions (including order exps). uint *record_offsets; // Record IDs corresponding to each projection (including order exps). bool singleResponse; // When no child operations, return NULL after a first response.