From bccd66424aa275a468d7001b180683983eca0e06 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 9 May 2019 14:02:29 -0400 Subject: [PATCH] types: per comment, avoid copy when t is not bound As mentioned in #9378. Fix recursion issue mentioned in #25796 by using inst_datatype_inner instead of inst_datatype, so that we shouldn't be making copies of any non-bound objects (anything maybe-cacheable) now. --- src/jltypes.c | 154 ++++++++++++++++++++++++-------------------------- 1 file changed, 74 insertions(+), 80 deletions(-) diff --git a/src/jltypes.c b/src/jltypes.c index 057f66d42d53c..63d4813df1423 100644 --- a/src/jltypes.c +++ b/src/jltypes.c @@ -819,8 +819,21 @@ static int within_typevar(jl_value_t *t, jl_value_t *vlb, jl_value_t *vub) struct _jl_typestack_t; typedef struct _jl_typestack_t jl_typestack_t; -static jl_value_t *inst_datatype(jl_datatype_t *dt, jl_svec_t *p, jl_value_t **iparams, size_t ntp, - int cacheable, jl_typestack_t *stack); +static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value_t **iparams, size_t ntp, + int cacheable, jl_typestack_t *stack, jl_typeenv_t *env); + +// Build an environment mapping a TypeName's parameters to parameter values. +// This is the environment needed for instantiating a type's supertype and field types. +static jl_value_t *inst_datatype_env(jl_value_t *dt, jl_svec_t *p, jl_value_t **iparams, size_t ntp, + int cacheable, jl_typestack_t *stack, jl_typeenv_t *env, int c) +{ + if (jl_is_datatype(dt)) + return inst_datatype_inner((jl_datatype_t*)dt, p, iparams, ntp, cacheable, stack, env); + assert(jl_is_unionall(dt)); + jl_unionall_t *ua = (jl_unionall_t*)dt; + jl_typeenv_t e = { ua->var, iparams[c], env }; + return inst_datatype_env(ua->body, p, iparams, ntp, cacheable, stack, &e, c + 1); +} jl_value_t *jl_apply_type(jl_value_t *tc, jl_value_t **params, size_t n) { @@ -836,12 +849,13 @@ jl_value_t *jl_apply_type(jl_value_t *tc, jl_value_t **params, size_t n) if (jl_is_datatype(u) && n == jl_nparams((jl_datatype_t*)u) && ((jl_datatype_t*)u)->name->wrapper == tc) { int cacheable = 1; - for(i=0; i < n; i++) { + for (i = 0; i < n; i++) { if (jl_has_free_typevars(params[i])) { - cacheable = 0; break; + cacheable = 0; + break; } } - return inst_datatype((jl_datatype_t*)u, NULL, params, n, cacheable, NULL); + return inst_datatype_env(tc, NULL, params, n, cacheable, NULL, NULL, 0); } } JL_GC_PUSH1(&tc); @@ -1068,6 +1082,8 @@ static jl_value_t *normalize_vararg(jl_value_t *va) return va; } +static jl_value_t *_jl_instantiate_type_in_env(jl_value_t *ty, jl_unionall_t *env, jl_value_t **vals, jl_typeenv_t *prev, jl_typestack_t *stack); + static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value_t **iparams, size_t ntp, int cacheable, jl_typestack_t *stack, jl_typeenv_t *env) { @@ -1262,6 +1278,7 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value if (cacheable && !ndt->abstract) ndt->uid = jl_assign_type_uid(); + jl_datatype_t *primarydt = ((jl_datatype_t*)jl_unwrap_unionall(tn->wrapper)); if (istuple || isnamedtuple) { ndt->super = jl_any_type; } @@ -1273,10 +1290,12 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value jl_cache_type_(ndt); } jl_svec_t *ftypes = dt->types; + if (ftypes == NULL) + ftypes = primarydt->types; if (ftypes == NULL || dt->super == NULL) { // in the process of creating this type definition: // need to instantiate the super and types fields later - assert(inside_typedef && !istuple && !isnamedtuple); + assert((inside_typedef || primarydt->super) && !istuple && !isnamedtuple); arraylist_push(&partial_inst, ndt); } else if (!isnamedtuple && !istuple) { @@ -1287,7 +1306,10 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value } else if (cacheable) { // recursively instantiate the types of the fields - ndt->types = inst_ftypes(ftypes, env, stack); + if (dt->types == NULL) + ndt->types = jl_compute_fieldtypes(ndt); + else + ndt->types = inst_ftypes(ftypes, env, stack); jl_gc_wb(ndt, ndt->types); } } @@ -1315,25 +1337,6 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value return (jl_value_t*)ndt; } -// Build an environment mapping a TypeName's parameters to parameter values. -// This is the environment needed for instantiating a type's supertype and field types. -static jl_value_t *inst_datatype_env(jl_value_t *dt, jl_svec_t *p, jl_value_t **iparams, size_t ntp, - int cacheable, jl_typestack_t *stack, jl_typeenv_t *env, int c) -{ - if (jl_is_datatype(dt)) - return inst_datatype_inner((jl_datatype_t*)dt, p, iparams, ntp, cacheable, stack, env); - assert(jl_is_unionall(dt)); - jl_unionall_t *ua = (jl_unionall_t*)dt; - jl_typeenv_t e = { ua->var, iparams[c], env }; - return inst_datatype_env(ua->body, p, iparams, ntp, cacheable, stack, &e, c+1); -} - -static jl_value_t *inst_datatype(jl_datatype_t *dt, jl_svec_t *p, jl_value_t **iparams, size_t ntp, - int cacheable, jl_typestack_t *stack) -{ - return inst_datatype_env(dt->name->wrapper, p, iparams, ntp, cacheable, stack, NULL, 0); -} - static jl_tupletype_t *jl_apply_tuple_type_v_(jl_value_t **p, size_t np, jl_svec_t *params) { int cacheable = 1; @@ -1342,9 +1345,7 @@ static jl_tupletype_t *jl_apply_tuple_type_v_(jl_value_t **p, size_t np, jl_svec if (!jl_is_concrete_type(p[i])) cacheable = 0; } - jl_datatype_t *ndt = (jl_datatype_t*)inst_datatype(jl_anytuple_type, params, p, np, - cacheable, NULL); - return ndt; + return (jl_datatype_t*)inst_datatype_inner(jl_anytuple_type, params, p, np, cacheable, NULL, NULL); } JL_DLLEXPORT jl_tupletype_t *jl_apply_tuple_type(jl_svec_t *params) @@ -1359,12 +1360,12 @@ JL_DLLEXPORT jl_tupletype_t *jl_apply_tuple_type_v(jl_value_t **p, size_t np) jl_datatype_t *jl_inst_concrete_tupletype(jl_svec_t *p) { - return (jl_datatype_t*)inst_datatype(jl_anytuple_type, p, jl_svec_data(p), jl_svec_len(p), 1, NULL); + return (jl_datatype_t*)inst_datatype_inner(jl_anytuple_type, p, jl_svec_data(p), jl_svec_len(p), 1, NULL, NULL); } jl_datatype_t *jl_inst_concrete_tupletype_v(jl_value_t **p, size_t np) { - return (jl_datatype_t*)inst_datatype(jl_anytuple_type, NULL, p, np, 1, NULL); + return (jl_datatype_t*)inst_datatype_inner(jl_anytuple_type, NULL, p, np, 1, NULL, NULL); } static jl_svec_t *inst_ftypes(jl_svec_t *p, jl_typeenv_t *env, jl_typestack_t *stack) @@ -1393,7 +1394,7 @@ static jl_value_t *inst_tuple_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_ jl_svec_t *tp = tt->parameters; size_t ntp = jl_svec_len(tp); // Instantiate NTuple{3,Int} - // Note this does not instantiate Tuple{Vararg{Int,3}}; that's done in inst_datatype + // Note this does not instantiate Tuple{Vararg{Int,3}}; that's done in inst_datatype_inner if (jl_is_va_tuple(tt) && ntp == 1) { // If this is a Tuple{Vararg{T,N}} with known N, expand it to // a fixed-length tuple @@ -1419,12 +1420,13 @@ static jl_value_t *inst_tuple_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_ jl_value_t **iparams; int onstack = ntp < jl_page_size/sizeof(jl_value_t*); JL_GC_PUSHARGS(iparams, onstack ? ntp : 1); - jl_svec_t *ip_heap=NULL; + jl_svec_t *ip_heap = NULL; if (!onstack) { ip_heap = jl_alloc_svec(ntp); iparams[0] = (jl_value_t*)ip_heap; iparams = jl_svec_data(ip_heap); } + int bound = 0; int cacheable = 1; if (jl_is_va_tuple(tt)) cacheable = 0; @@ -1435,12 +1437,14 @@ static jl_value_t *inst_tuple_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_ iparams[i] = pi; if (ip_heap) jl_gc_wb(ip_heap, pi); + bound |= (pi != elt); if (cacheable && !jl_is_concrete_type(pi)) cacheable = 0; } - jl_value_t *result = inst_datatype((jl_datatype_t*)tt, ip_heap, iparams, ntp, cacheable, stack); + if (bound) + t = inst_datatype_inner(tt, ip_heap, iparams, ntp, cacheable, stack, env); JL_GC_POP(); - return result; + return t; } static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t *stack, int check) @@ -1451,44 +1455,38 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t while (e != NULL) { if (e->var == (jl_tvar_t*)t) { jl_value_t *val = e->val; - // TODO jb/subtype this seems unnecessary - //if (check && !jl_is_typevar(val) && !within_typevar(val, (jl_tvar_t*)t)) { - // jl_type_error_rt("type parameter", - // jl_symbol_name(((jl_tvar_t*)t)->name), t, val); - //} return val; } e = e->prev; } - return (jl_value_t*)t; + return t; } if (jl_is_unionall(t)) { - if (!jl_has_free_typevars(t)) - return t; jl_unionall_t *ua = (jl_unionall_t*)t; - jl_value_t *res=NULL, *lb=ua->var->lb, *ub=ua->var->ub; - JL_GC_PUSH3(&lb, &ub, &res); - res = jl_new_struct(jl_unionall_type, ua->var, NULL); - if (jl_has_bound_typevars(ua->var->lb, env)) - lb = inst_type_w_(ua->var->lb, env, stack, check); - if (jl_has_bound_typevars(ua->var->ub, env)) - ub = inst_type_w_(ua->var->ub, env, stack, check); - if (lb != ua->var->lb || ub != ua->var->ub) { - ((jl_unionall_t*)res)->var = jl_new_typevar(ua->var->name, lb, ub); - jl_gc_wb(res, ((jl_unionall_t*)res)->var); + jl_value_t *lb = NULL; + jl_value_t *var = NULL; + jl_value_t *newbody = NULL; + JL_GC_PUSH3(&lb, &var, &newbody); + lb = inst_type_w_(ua->var->lb, env, stack, check); + var = inst_type_w_(ua->var->ub, env, stack, check); + if (lb != ua->var->lb || var != ua->var->ub) { + var = (jl_value_t*)jl_new_typevar(ua->var->name, lb, var); + } + else { + var = (jl_value_t*)ua->var; } - jl_typeenv_t newenv = { ua->var, (jl_value_t*)((jl_unionall_t*)res)->var, env }; - jl_value_t *newbody = inst_type_w_(ua->body, &newenv, stack, check); + jl_typeenv_t newenv = { ua->var, var, env }; + newbody = inst_type_w_(ua->body, &newenv, stack, check); if (newbody == (jl_value_t*)jl_emptytuple_type) { // NTuple{0} => Tuple{} can make a typevar disappear - res = (jl_value_t*)jl_emptytuple_type; + t = (jl_value_t*)jl_emptytuple_type; } - else { - ((jl_unionall_t*)res)->body = newbody; - jl_gc_wb(res, newbody); + else if (newbody != ua->body || var != (jl_value_t*)ua->var) { + // if t's parameters are not bound in the environment, return it uncopied (#9378) + t = jl_new_struct(jl_unionall_type, var, newbody); } JL_GC_POP(); - return res; + return t; } if (jl_is_uniontype(t)) { jl_uniontype_t *u = (jl_uniontype_t*)t; @@ -1496,23 +1494,19 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t jl_value_t *b = NULL; JL_GC_PUSH2(&a, &b); b = inst_type_w_(u->b, env, stack, check); - jl_value_t *res; - if (a == u->a && b == u->b) { - res = t; - } - else { + if (a != u->a || b != u->b) { jl_value_t *uargs[2] = {a, b}; - res = jl_type_union(uargs, 2); + t = jl_type_union(uargs, 2); } JL_GC_POP(); - return res; + return t; } if (!jl_is_datatype(t)) return t; jl_datatype_t *tt = (jl_datatype_t*)t; jl_svec_t *tp = tt->parameters; if (tp == jl_emptysvec) - return (jl_value_t*)t; + return t; jl_typename_t *tn = tt->name; if (tn == jl_tuple_typename) return inst_tuple_w_(t, env, stack, check); @@ -1520,19 +1514,19 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t jl_value_t **iparams; JL_GC_PUSHARGS(iparams, ntp); int cacheable = 1, bound = 0; - for(i=0; i < ntp; i++) { + for (i = 0; i < ntp; i++) { jl_value_t *elt = jl_svecref(tp, i); - iparams[i] = inst_type_w_(elt, env, stack, check); - bound |= (iparams[i] != elt); - if (cacheable && jl_has_free_typevars(iparams[i])) + jl_value_t *pi = inst_type_w_(elt, env, stack, check); + iparams[i] = pi; + bound |= (pi != elt); + if (cacheable && jl_has_free_typevars(pi)) cacheable = 0; } // if t's parameters are not bound in the environment, return it uncopied (#9378) - if (!bound) { JL_GC_POP(); return (jl_value_t*)t; } - - jl_value_t *result = inst_datatype((jl_datatype_t*)tt, NULL, iparams, ntp, cacheable, stack); + if (bound) + t = inst_datatype_inner(tt, NULL, iparams, ntp, cacheable, stack, env); JL_GC_POP(); - return result; + return t; } static jl_value_t *instantiate_with(jl_value_t *t, jl_value_t **env, size_t n, jl_typeenv_t *te) @@ -1549,13 +1543,13 @@ jl_value_t *jl_instantiate_type_with(jl_value_t *t, jl_value_t **env, size_t n) return instantiate_with(t, env, n, NULL); } -static jl_value_t *_jl_instantiate_type_in_env(jl_value_t *ty, jl_unionall_t *env, jl_value_t **vals, jl_typeenv_t *prev) +static jl_value_t *_jl_instantiate_type_in_env(jl_value_t *ty, jl_unionall_t *env, jl_value_t **vals, jl_typeenv_t *prev, jl_typestack_t *stack) { jl_typeenv_t en = { env->var, vals[0], prev }; if (jl_is_unionall(env->body)) - return _jl_instantiate_type_in_env(ty, (jl_unionall_t*)env->body, vals + 1, &en); + return _jl_instantiate_type_in_env(ty, (jl_unionall_t*)env->body, vals + 1, &en, stack); else - return inst_type_w_(ty, &en, NULL, 1); + return inst_type_w_(ty, &en, stack, 1); } JL_DLLEXPORT jl_value_t *jl_instantiate_type_in_env(jl_value_t *ty, jl_unionall_t *env, jl_value_t **vals) @@ -1563,7 +1557,7 @@ JL_DLLEXPORT jl_value_t *jl_instantiate_type_in_env(jl_value_t *ty, jl_unionall_ jl_value_t *typ = ty; if (jl_is_unionall(env)) { JL_TRY { - typ = _jl_instantiate_type_in_env(ty, env, vals, NULL); + typ = _jl_instantiate_type_in_env(ty, env, vals, NULL, NULL); } JL_CATCH { typ = jl_bottom_type;