From 40304b5ab61d4c6ff1bf812bf84ada83dbf0b548 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Fri, 1 Nov 2019 18:55:45 -0400 Subject: [PATCH 1/3] Lock (`codegen_lock`) during typedef to prevent concurrency violation. As far as I can tell, this doesn't much affect parallel performance, and prevents this exception from triggering if you attempt to define new functions from multiple threads: ``` ERROR: TaskFailedException: cannot eval a new struct type definition while defining another type ``` --- src/interpreter.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/interpreter.c b/src/interpreter.c index 396b590beed2b..1db2d8249f553 100644 --- a/src/interpreter.c +++ b/src/interpreter.c @@ -128,6 +128,7 @@ SECT_INTERP void jl_set_datatype_super(jl_datatype_t *tt, jl_value_t *super) static void eval_abstracttype(jl_expr_t *ex, interpreter_state *s) { jl_value_t **args = jl_array_ptr_data(ex->args); + JL_LOCK(&codegen_lock); if (inside_typedef) jl_error("cannot eval a new abstract type definition while defining another type"); jl_value_t *name = args[0]; @@ -156,9 +157,11 @@ static void eval_abstracttype(jl_expr_t *ex, interpreter_state *s) super = eval_value(args[2], s); jl_set_datatype_super(dt, super); jl_reinstantiate_inner_types(dt); + JL_UNLOCK(&codegen_lock); } JL_CATCH { jl_reset_instantiate_inner_types(dt); + JL_UNLOCK(&codegen_lock); b->value = temp; jl_rethrow(); } @@ -172,6 +175,7 @@ static void eval_abstracttype(jl_expr_t *ex, interpreter_state *s) static void eval_primitivetype(jl_expr_t *ex, interpreter_state *s) { jl_value_t **args = (jl_value_t**)jl_array_ptr_data(ex->args); + JL_LOCK(&codegen_lock); if (inside_typedef) jl_error("cannot eval a new primitive type definition while defining another type"); jl_value_t *name = args[0]; @@ -207,9 +211,11 @@ static void eval_primitivetype(jl_expr_t *ex, interpreter_state *s) super = eval_value(args[3], s); jl_set_datatype_super(dt, super); jl_reinstantiate_inner_types(dt); + JL_UNLOCK(&codegen_lock); } JL_CATCH { jl_reset_instantiate_inner_types(dt); + JL_UNLOCK(&codegen_lock); b->value = temp; jl_rethrow(); } @@ -223,6 +229,7 @@ static void eval_primitivetype(jl_expr_t *ex, interpreter_state *s) static void eval_structtype(jl_expr_t *ex, interpreter_state *s) { jl_value_t **args = jl_array_ptr_data(ex->args); + JL_LOCK(&codegen_lock); if (inside_typedef) jl_error("cannot eval a new struct type definition while defining another type"); jl_value_t *name = args[0]; @@ -268,9 +275,11 @@ static void eval_structtype(jl_expr_t *ex, interpreter_state *s) } } jl_reinstantiate_inner_types(dt); + JL_UNLOCK(&codegen_lock); } JL_CATCH { jl_reset_instantiate_inner_types(dt); + JL_UNLOCK(&codegen_lock); b->value = temp; jl_rethrow(); } From 3c3e60040b2b2c258b915f2325bd26402cff7e77 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Thu, 7 Nov 2019 14:43:27 -0500 Subject: [PATCH 2/3] Fix my mistake: move the UNLOCK to the same scope as LOCK --- src/interpreter.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/interpreter.c b/src/interpreter.c index 1db2d8249f553..6ce262ac4206f 100644 --- a/src/interpreter.c +++ b/src/interpreter.c @@ -157,11 +157,9 @@ static void eval_abstracttype(jl_expr_t *ex, interpreter_state *s) super = eval_value(args[2], s); jl_set_datatype_super(dt, super); jl_reinstantiate_inner_types(dt); - JL_UNLOCK(&codegen_lock); } JL_CATCH { jl_reset_instantiate_inner_types(dt); - JL_UNLOCK(&codegen_lock); b->value = temp; jl_rethrow(); } @@ -170,6 +168,7 @@ static void eval_abstracttype(jl_expr_t *ex, interpreter_state *s) jl_checked_assignment(b, w); } JL_GC_POP(); + JL_UNLOCK(&codegen_lock); } static void eval_primitivetype(jl_expr_t *ex, interpreter_state *s) @@ -211,11 +210,9 @@ static void eval_primitivetype(jl_expr_t *ex, interpreter_state *s) super = eval_value(args[3], s); jl_set_datatype_super(dt, super); jl_reinstantiate_inner_types(dt); - JL_UNLOCK(&codegen_lock); } JL_CATCH { jl_reset_instantiate_inner_types(dt); - JL_UNLOCK(&codegen_lock); b->value = temp; jl_rethrow(); } @@ -224,6 +221,7 @@ static void eval_primitivetype(jl_expr_t *ex, interpreter_state *s) jl_checked_assignment(b, w); } JL_GC_POP(); + JL_UNLOCK(&codegen_lock); } static void eval_structtype(jl_expr_t *ex, interpreter_state *s) @@ -275,11 +273,9 @@ static void eval_structtype(jl_expr_t *ex, interpreter_state *s) } } jl_reinstantiate_inner_types(dt); - JL_UNLOCK(&codegen_lock); } JL_CATCH { jl_reset_instantiate_inner_types(dt); - JL_UNLOCK(&codegen_lock); b->value = temp; jl_rethrow(); } @@ -291,6 +287,7 @@ static void eval_structtype(jl_expr_t *ex, interpreter_state *s) } JL_GC_POP(); + JL_UNLOCK(&codegen_lock); } // method definition form From 0830f8a2bf3d4222baca1bda3582f77b24e5630d Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Thu, 7 Nov 2019 14:49:42 -0500 Subject: [PATCH 3/3] Change the interpreter locks to use "new" `toplevel_lock`, which just aliases the existing codegen_lock --- src/interpreter.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/interpreter.c b/src/interpreter.c index 6ce262ac4206f..36b65fc585f41 100644 --- a/src/interpreter.c +++ b/src/interpreter.c @@ -34,6 +34,13 @@ int jl_is_toplevel_only_expr(jl_value_t *e); // type definition forms +// TODO: separate the codegen and toplevel locks +// currently using a coarser lock seems like +// the best way to avoid acquisition priority +// ordering violations +//static jl_mutex_t toplevel_lock; +#define toplevel_lock codegen_lock + extern int inside_typedef; // this is a heuristic for allowing "redefining" a type to something identical @@ -128,7 +135,7 @@ SECT_INTERP void jl_set_datatype_super(jl_datatype_t *tt, jl_value_t *super) static void eval_abstracttype(jl_expr_t *ex, interpreter_state *s) { jl_value_t **args = jl_array_ptr_data(ex->args); - JL_LOCK(&codegen_lock); + JL_LOCK(&toplevel_lock); if (inside_typedef) jl_error("cannot eval a new abstract type definition while defining another type"); jl_value_t *name = args[0]; @@ -168,13 +175,13 @@ static void eval_abstracttype(jl_expr_t *ex, interpreter_state *s) jl_checked_assignment(b, w); } JL_GC_POP(); - JL_UNLOCK(&codegen_lock); + JL_UNLOCK(&toplevel_lock); } static void eval_primitivetype(jl_expr_t *ex, interpreter_state *s) { jl_value_t **args = (jl_value_t**)jl_array_ptr_data(ex->args); - JL_LOCK(&codegen_lock); + JL_LOCK(&toplevel_lock); if (inside_typedef) jl_error("cannot eval a new primitive type definition while defining another type"); jl_value_t *name = args[0]; @@ -221,13 +228,13 @@ static void eval_primitivetype(jl_expr_t *ex, interpreter_state *s) jl_checked_assignment(b, w); } JL_GC_POP(); - JL_UNLOCK(&codegen_lock); + JL_UNLOCK(&toplevel_lock); } static void eval_structtype(jl_expr_t *ex, interpreter_state *s) { jl_value_t **args = jl_array_ptr_data(ex->args); - JL_LOCK(&codegen_lock); + JL_LOCK(&toplevel_lock); if (inside_typedef) jl_error("cannot eval a new struct type definition while defining another type"); jl_value_t *name = args[0]; @@ -287,7 +294,7 @@ static void eval_structtype(jl_expr_t *ex, interpreter_state *s) } JL_GC_POP(); - JL_UNLOCK(&codegen_lock); + JL_UNLOCK(&toplevel_lock); } // method definition form