From 81e7c40264520b387358917987d101f5f5ae4705 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 24 Feb 2020 01:32:01 +0100 Subject: [PATCH] Optimize primop calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We now parse function applications as a vector of arguments rather than as a chain of binary applications, e.g. 'substring 1 2 "foo"' is parsed as ExprCall { .fun = , .args = [ <1>, <2>, <"foo"> ] } rather than ExprApp (ExprApp (ExprApp <1>) <2>) <"foo"> This allows primops to be called immediately (if enough arguments are supplied) without having to allocate intermediate tPrimOpApp values. On $ nix-instantiate --dry-run '' -A nixos.tests.simple.x86_64-linux this gives a substantial performance improvement: user CPU time: median = 0.9209 mean = 0.9218 stddev = 0.0073 min = 0.9086 max = 0.9340 [rejected, p=0.00000, Δ=-0.21433±0.00677] elapsed time: median = 1.0585 mean = 1.0584 stddev = 0.0024 min = 1.0523 max = 1.0623 [rejected, p=0.00000, Δ=-0.20594±0.00236] because it reduces the number of tPrimOpApp allocations from 551990 to 42534 (i.e. only small minority of primop calls are partially applied) which in turn reduces time spent in the garbage collector. --- src/libexpr/eval.cc | 272 +++++++++++++++++++++++------------------ src/libexpr/eval.hh | 12 +- src/libexpr/lexer.l | 1 + src/libexpr/nixexpr.cc | 18 ++- src/libexpr/nixexpr.hh | 12 +- src/libexpr/parser.y | 51 +++++--- 6 files changed, 228 insertions(+), 138 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 062d190b6..b4b4f7b5c 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -594,6 +594,8 @@ Value * EvalState::addConstant(const string & name, Value & v) Value * EvalState::addPrimOp(const string & name, size_t arity, PrimOpFun primOp) { + assert(arity <= maxPrimOpArity); + auto name2 = string(name, 0, 2) == "__" ? string(name, 2) : name; Symbol sym = symbols.create(name2); @@ -1251,144 +1253,182 @@ void ExprLambda::eval(EvalState & state, Env & env, Value & v) } -void ExprApp::eval(EvalState & state, Env & env, Value & v) -{ - /* FIXME: vFun prevents GCC from doing tail call optimisation. */ - Value vFun; - e1->eval(state, env, vFun); - state.callFunction(vFun, *(e2->maybeThunk(state, env)), v, pos); -} - - -void EvalState::callPrimOp(Value & fun, Value & arg, Value & v, const Pos & pos) -{ - /* Figure out the number of arguments still needed. */ - size_t argsDone = 0; - Value * primOp = &fun; - while (primOp->isPrimOpApp()) { - argsDone++; - primOp = primOp->primOpApp.left; - } - assert(primOp->isPrimOp()); - auto arity = primOp->primOp->arity; - auto argsLeft = arity - argsDone; - - if (argsLeft == 1) { - /* We have all the arguments, so call the primop. */ - - /* Put all the arguments in an array. */ - Value * vArgs[arity]; - auto n = arity - 1; - vArgs[n--] = &arg; - for (Value * arg = &fun; arg->isPrimOpApp(); arg = arg->primOpApp.left) - vArgs[n--] = arg->primOpApp.right; - - /* And call the primop. */ - nrPrimOpCalls++; - if (countCalls) primOpCalls[primOp->primOp->name]++; - primOp->primOp->fun(*this, pos, vArgs, v); - } else { - Value * fun2 = allocValue(); - *fun2 = fun; - v.mkPrimOpApp(fun2, &arg); - } -} - -void EvalState::callFunction(Value & fun, Value & arg, Value & v, const Pos & pos) +void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & vRes, const Pos & pos) { auto trace = evalSettings.traceFunctionCalls ? std::make_unique(pos) : nullptr; forceValue(fun, pos); - if (fun.isPrimOp() || fun.isPrimOpApp()) { - callPrimOp(fun, arg, v, pos); - return; - } + Value vCur(fun); - if (fun.type() == nAttrs) { - auto found = fun.attrs->find(sFunctor); - if (found != fun.attrs->end()) { - /* fun may be allocated on the stack of the calling function, - * but for functors we may keep a reference, so heap-allocate - * a copy and use that instead. - */ - auto & fun2 = *allocValue(); - fun2 = fun; - /* !!! Should we use the attr pos here? */ - Value v2; - callFunction(*found->value, fun2, v2, pos); - return callFunction(v2, arg, v, pos); - } - } + auto makeAppChain = [&]() + { + vRes = vCur; + for (size_t i = 0; i < nrArgs; ++i) { + auto fun2 = allocValue(); + *fun2 = vRes; + vRes.mkPrimOpApp(fun2, args[i]); + } + }; - if (!fun.isLambda()) - throwTypeError(pos, "attempt to call something which is not a function but %1%", fun); + while (nrArgs > 0) { - ExprLambda & lambda(*fun.lambda.fun); + if (vCur.isLambda()) { - auto size = - (lambda.arg.empty() ? 0 : 1) + - (lambda.hasFormals() ? lambda.formals->formals.size() : 0); - Env & env2(allocEnv(size)); - env2.up = fun.lambda.env; + ExprLambda & lambda(*vCur.lambda.fun); - size_t displ = 0; + auto size = + (lambda.arg.empty() ? 0 : 1) + + (lambda.hasFormals() ? lambda.formals->formals.size() : 0); + Env & env2(allocEnv(size)); + env2.up = vCur.lambda.env; - if (!lambda.hasFormals()) - env2.values[displ++] = &arg; + size_t displ = 0; - else { - forceAttrs(arg, pos); + if (!lambda.hasFormals()) + env2.values[displ++] = args[0]; - if (!lambda.arg.empty()) - env2.values[displ++] = &arg; + else { + forceAttrs(*args[0], pos); - /* For each formal argument, get the actual argument. If - there is no matching actual argument but the formal - argument has a default, use the default. */ - size_t attrsUsed = 0; - for (auto & i : lambda.formals->formals) { - Bindings::iterator j = arg.attrs->find(i.name); - if (j == arg.attrs->end()) { - if (!i.def) throwTypeError(pos, "%1% called without required argument '%2%'", - lambda, i.name); - env2.values[displ++] = i.def->maybeThunk(*this, env2); + if (!lambda.arg.empty()) + env2.values[displ++] = args[0]; + + /* For each formal argument, get the actual argument. If + there is no matching actual argument but the formal + argument has a default, use the default. */ + size_t attrsUsed = 0; + for (auto & i : lambda.formals->formals) { + auto j = args[0]->attrs->get(i.name); + if (!j) { + if (!i.def) throwTypeError(pos, "%1% called without required argument '%2%'", + lambda, i.name); + env2.values[displ++] = i.def->maybeThunk(*this, env2); + } else { + attrsUsed++; + env2.values[displ++] = j->value; + } + } + + /* Check that each actual argument is listed as a formal + argument (unless the attribute match specifies a `...'). */ + if (!lambda.formals->ellipsis && attrsUsed != args[0]->attrs->size()) { + /* Nope, so show the first unexpected argument to the + user. */ + for (auto & i : *args[0]->attrs) + if (lambda.formals->argNames.find(i.name) == lambda.formals->argNames.end()) + throwTypeError(pos, "%1% called with unexpected argument '%2%'", lambda, i.name); + abort(); // can't happen + } + } + + nrFunctionCalls++; + if (countCalls) incrFunctionCall(&lambda); + + /* Evaluate the body. */ + try { + lambda.body->eval(*this, env2, vCur); + } catch (Error & e) { + if (loggerSettings.showTrace.get()) { + addErrorTrace(e, lambda.pos, "while evaluating %s", + (lambda.name.set() + ? "'" + (string) lambda.name + "'" + : "anonymous lambda")); + addErrorTrace(e, pos, "from call site%s", ""); + } + throw; + } + + nrArgs--; + args += 1; + } + + else if (vCur.isPrimOp()) { + + size_t argsLeft = vCur.primOp->arity; + + if (nrArgs < argsLeft) { + /* We don't have enough arguments, so create a tPrimOpApp chain. */ + makeAppChain(); + return; } else { - attrsUsed++; - env2.values[displ++] = j->value; + /* We have all the arguments, so call the primop. */ + nrPrimOpCalls++; + if (countCalls) primOpCalls[vCur.primOp->name]++; + vCur.primOp->fun(*this, pos, args, vCur); + + nrArgs -= argsLeft; + args += argsLeft; } } - /* Check that each actual argument is listed as a formal - argument (unless the attribute match specifies a `...'). */ - if (!lambda.formals->ellipsis && attrsUsed != arg.attrs->size()) { - /* Nope, so show the first unexpected argument to the - user. */ - for (auto & i : *arg.attrs) - if (lambda.formals->argNames.find(i.name) == lambda.formals->argNames.end()) - throwTypeError(pos, "%1% called with unexpected argument '%2%'", lambda, i.name); - abort(); // can't happen + else if (vCur.isPrimOpApp()) { + /* Figure out the number of arguments still needed. */ + size_t argsDone = 0; + Value * primOp = &vCur; + while (primOp->isPrimOpApp()) { + argsDone++; + primOp = primOp->primOpApp.left; + } + assert(primOp->isPrimOp()); + auto arity = primOp->primOp->arity; + auto argsLeft = arity - argsDone; + + if (nrArgs < argsLeft) { + /* We still don't have enough arguments, so extend the tPrimOpApp chain. */ + makeAppChain(); + return; + } else { + /* We have all the arguments, so call the primop with + the previous and new arguments. */ + + Value * vArgs[arity]; + auto n = argsDone; + for (Value * arg = &vCur; arg->isPrimOpApp(); arg = arg->primOpApp.left) + vArgs[--n] = arg->primOpApp.right; + + for (size_t i = 0; i < argsLeft; ++i) + vArgs[argsDone + i] = args[i]; + + nrPrimOpCalls++; + if (countCalls) primOpCalls[primOp->primOp->name]++; + primOp->primOp->fun(*this, pos, vArgs, vCur); + + nrArgs -= argsLeft; + args += argsLeft; + } } + + else if (vCur.type() == nAttrs) { + if (auto functor = vCur.attrs->get(sFunctor)) { + /* 'vCur" may be allocated on the stack of the calling + function, but for functors we may keep a reference, + so heap-allocate a copy and use that instead. */ + Value * args2[] = {allocValue()}; + *args2[0] = vCur; + /* !!! Should we use the attr pos here? */ + callFunction(*functor->value, 1, args2, vCur, pos); + } + } + + else + throwTypeError(pos, "attempt to call something which is not a function but %1%", vCur); } - nrFunctionCalls++; - if (countCalls) incrFunctionCall(&lambda); + vRes = vCur; +} - /* Evaluate the body. This is conditional on showTrace, because - catching exceptions makes this function not tail-recursive. */ - if (loggerSettings.showTrace.get()) - try { - lambda.body->eval(*this, env2, v); - } catch (Error & e) { - addErrorTrace(e, lambda.pos, "while evaluating %s", - (lambda.name.set() - ? "'" + (string) lambda.name + "'" - : "anonymous lambda")); - addErrorTrace(e, pos, "from call site%s", ""); - throw; - } - else - fun.lambda.fun->body->eval(*this, env2, v); + +void ExprCall::eval(EvalState & state, Env & env, Value & v) +{ + Value vFun; + fun->eval(state, env, vFun); + + Value * vArgs[args.size()]; + for (size_t i = 0; i < args.size(); ++i) + vArgs[i] = args[i]->maybeThunk(state, env); + + state.callFunction(vFun, args.size(), vArgs, v, pos); } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 69119599a..f87dcdd8e 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -277,6 +277,8 @@ private: Value * addConstant(const string & name, Value & v); + constexpr static size_t maxPrimOpArity = 3; + Value * addPrimOp(const string & name, size_t arity, PrimOpFun primOp); @@ -316,8 +318,14 @@ public: bool isFunctor(Value & fun); - void callFunction(Value & fun, Value & arg, Value & v, const Pos & pos); - void callPrimOp(Value & fun, Value & arg, Value & v, const Pos & pos); + // FIXME: use std::span + void callFunction(Value & fun, size_t nrArgs, Value * * args, Value & vRes, const Pos & pos); + + void callFunction(Value & fun, Value & arg, Value & vRes, const Pos & pos) + { + Value * args[] = {&arg}; + callFunction(fun, 1, args, vRes, pos); + } /* Automatically call a function for which each argument has a default value or has a binding in the `args' map. */ diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index 51593eccd..c18877e29 100644 --- a/src/libexpr/lexer.l +++ b/src/libexpr/lexer.l @@ -64,6 +64,7 @@ static void adjustLoc(YYLTYPE * loc, const char * s, size_t len) } +// FIXME: optimize static Expr * unescapeStr(SymbolTable & symbols, const char * s, size_t length) { string t; diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 95a353a40..372e323bc 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -143,6 +143,16 @@ void ExprLambda::show(std::ostream & str) const str << ": " << *body << ")"; } +void ExprCall::show(std::ostream & str) const +{ + str << '(' << *fun; + for (auto e : args) { + str << ' '; + str << *e; + } + str << ')'; +} + void ExprLet::show(std::ostream & str) const { str << "(let "; @@ -366,6 +376,13 @@ void ExprLambda::bindVars(const StaticEnv & env) body->bindVars(newEnv); } +void ExprCall::bindVars(const StaticEnv & env) +{ + fun->bindVars(env); + for (auto e : args) + e->bindVars(env); +} + void ExprLet::bindVars(const StaticEnv & env) { StaticEnv newEnv(false, &env, attrs->attrs.size()); @@ -461,5 +478,4 @@ size_t SymbolTable::totalSize() const return n; } - } diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 3bfa5e0d3..fe12890e9 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -248,6 +248,17 @@ struct ExprLambda : Expr COMMON_METHODS }; +struct ExprCall : Expr +{ + Expr * fun; + std::vector args; + Pos pos; + ExprCall(const Pos & pos, Expr * fun, std::vector && args) + : fun(fun), args(args), pos(pos) + { } + COMMON_METHODS +}; + struct ExprLet : Expr { ExprAttrs * attrs; @@ -306,7 +317,6 @@ struct ExprOpNot : Expr void eval(EvalState & state, Env & env, Value & v); \ }; -MakeBinOp(ExprApp, "") MakeBinOp(ExprOpEq, "==") MakeBinOp(ExprOpNEq, "!=") MakeBinOp(ExprOpAnd, "&&") diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 5d0f05206..2e8a04143 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -41,6 +41,12 @@ namespace nix { { }; }; + // Helper to prevent an expensive dynamic_cast call in expr_app. + struct App + { + Expr * e; + bool isCall; + }; } #define YY_DECL int yylex \ @@ -280,10 +286,12 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParseData * data, const char * err char * uri; std::vector * attrNames; std::vector * string_parts; + nix::App app; // bool == whether this is an ExprCall } %type start expr expr_function expr_if expr_op -%type expr_app expr_select expr_simple +%type expr_select expr_simple +%type expr_app %type expr_list %type binds %type formals @@ -353,13 +361,13 @@ expr_if expr_op : '!' expr_op %prec NOT { $$ = new ExprOpNot($2); } - | '-' expr_op %prec NEGATE { $$ = new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__sub")), new ExprInt(0)), $2); } + | '-' expr_op %prec NEGATE { $$ = new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__sub")), {new ExprInt(0), $2}); } | expr_op EQ expr_op { $$ = new ExprOpEq($1, $3); } | expr_op NEQ expr_op { $$ = new ExprOpNEq($1, $3); } - | expr_op '<' expr_op { $$ = new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__lessThan")), $1), $3); } - | expr_op LEQ expr_op { $$ = new ExprOpNot(new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__lessThan")), $3), $1)); } - | expr_op '>' expr_op { $$ = new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__lessThan")), $3), $1); } - | expr_op GEQ expr_op { $$ = new ExprOpNot(new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__lessThan")), $1), $3)); } + | expr_op '<' expr_op { $$ = new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__lessThan")), {$1, $3}); } + | expr_op LEQ expr_op { $$ = new ExprOpNot(new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__lessThan")), {$3, $1})); } + | expr_op '>' expr_op { $$ = new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__lessThan")), {$3, $1}); } + | expr_op GEQ expr_op { $$ = new ExprOpNot(new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__lessThan")), {$1, $3})); } | expr_op AND expr_op { $$ = new ExprOpAnd(CUR_POS, $1, $3); } | expr_op OR expr_op { $$ = new ExprOpOr(CUR_POS, $1, $3); } | expr_op IMPL expr_op { $$ = new ExprOpImpl(CUR_POS, $1, $3); } @@ -367,17 +375,24 @@ expr_op | expr_op '?' attrpath { $$ = new ExprOpHasAttr($1, *$3); } | expr_op '+' expr_op { $$ = new ExprConcatStrings(CUR_POS, false, new vector({$1, $3})); } - | expr_op '-' expr_op { $$ = new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__sub")), $1), $3); } - | expr_op '*' expr_op { $$ = new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__mul")), $1), $3); } - | expr_op '/' expr_op { $$ = new ExprApp(CUR_POS, new ExprApp(new ExprVar(data->symbols.create("__div")), $1), $3); } + | expr_op '-' expr_op { $$ = new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__sub")), {$1, $3}); } + | expr_op '*' expr_op { $$ = new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__mul")), {$1, $3}); } + | expr_op '/' expr_op { $$ = new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__div")), {$1, $3}); } | expr_op CONCAT expr_op { $$ = new ExprOpConcatLists(CUR_POS, $1, $3); } - | expr_app + | expr_app { $$ = $1.e; } ; expr_app - : expr_app expr_select - { $$ = new ExprApp(CUR_POS, $1, $2); } - | expr_select { $$ = $1; } + : expr_app expr_select { + if ($1.isCall) { + ((ExprCall *) $1.e)->args.push_back($2); + $$ = $1; + } else { + $$.e = new ExprCall(CUR_POS, $1.e, {$2}); + $$.isCall = true; + } + } + | expr_select { $$.e = $1; $$.isCall = false; } ; expr_select @@ -388,7 +403,7 @@ expr_select | /* Backwards compatibility: because Nixpkgs has a rarely used function named ‘or’, allow stuff like ‘map or [...]’. */ expr_simple OR_KW - { $$ = new ExprApp(CUR_POS, $1, new ExprVar(CUR_POS, data->symbols.create("or"))); } + { $$ = new ExprCall(CUR_POS, $1, {new ExprVar(CUR_POS, data->symbols.create("or"))}); } | expr_simple { $$ = $1; } ; @@ -412,10 +427,10 @@ expr_simple } | SPATH { string path($1 + 1, strlen($1) - 2); - $$ = new ExprApp(CUR_POS, - new ExprApp(new ExprVar(data->symbols.create("__findFile")), - new ExprVar(data->symbols.create("__nixPath"))), - new ExprString(data->symbols.create(path))); + $$ = new ExprCall(CUR_POS, + new ExprVar(data->symbols.create("__findFile")), + {new ExprVar(data->symbols.create("__nixPath")), + new ExprString(data->symbols.create(path))}); } | URI { static bool noURLLiterals = settings.isExperimentalFeatureEnabled(Xp::NoUrlLiterals);