From ab35cbd675610a52513f746051e98d0a50815bc1 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 21 Feb 2020 18:31:16 +0100 Subject: [PATCH 1/7] StaticEnv: Use std::vector instead of std::map --- src/libexpr/eval.cc | 6 +++--- src/libexpr/nixexpr.cc | 25 +++++++++++++++++-------- src/libexpr/nixexpr.hh | 25 +++++++++++++++++++++---- src/libexpr/parser.y | 8 ++++---- src/libexpr/primops.cc | 10 ++++++++-- src/nix/repl.cc | 3 ++- 6 files changed, 55 insertions(+), 22 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index db1e7e56d..062d190b6 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -583,7 +583,7 @@ Value * EvalState::addConstant(const string & name, Value & v) { Value * v2 = allocValue(); *v2 = v; - staticBaseEnv.vars[symbols.create(name)] = baseEnvDispl; + staticBaseEnv.vars.emplace_back(symbols.create(name), baseEnvDispl); baseEnv.values[baseEnvDispl++] = v2; string name2 = string(name, 0, 2) == "__" ? string(name, 2) : name; baseEnv.values[0]->attrs->push_back(Attr(symbols.create(name2), v2)); @@ -609,7 +609,7 @@ Value * EvalState::addPrimOp(const string & name, Value * v = allocValue(); v->mkPrimOp(new PrimOp { .fun = primOp, .arity = arity, .name = sym }); - staticBaseEnv.vars[symbols.create(name)] = baseEnvDispl; + staticBaseEnv.vars.emplace_back(symbols.create(name), baseEnvDispl); baseEnv.values[baseEnvDispl++] = v; baseEnv.values[0]->attrs->push_back(Attr(sym, v)); return v; @@ -635,7 +635,7 @@ Value * EvalState::addPrimOp(PrimOp && primOp) Value * v = allocValue(); v->mkPrimOp(new PrimOp(std::move(primOp))); - staticBaseEnv.vars[envName] = baseEnvDispl; + staticBaseEnv.vars.emplace_back(envName, baseEnvDispl); baseEnv.values[baseEnvDispl++] = v; baseEnv.values[0]->attrs->push_back(Attr(primOp.name, v)); return v; diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 0d0f3e469..95a353a40 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -269,7 +269,7 @@ void ExprVar::bindVars(const StaticEnv & env) if (curEnv->isWith) { if (withLevel == -1) withLevel = level; } else { - StaticEnv::Vars::const_iterator i = curEnv->vars.find(name); + auto i = curEnv->find(name); if (i != curEnv->vars.end()) { fromWith = false; this->level = level; @@ -311,14 +311,16 @@ void ExprOpHasAttr::bindVars(const StaticEnv & env) void ExprAttrs::bindVars(const StaticEnv & env) { const StaticEnv * dynamicEnv = &env; - StaticEnv newEnv(false, &env); + StaticEnv newEnv(false, &env, recursive ? attrs.size() : 0); if (recursive) { dynamicEnv = &newEnv; unsigned int displ = 0; for (auto & i : attrs) - newEnv.vars[i.first] = i.second.displ = displ++; + newEnv.vars.emplace_back(i.first, i.second.displ = displ++); + + // No need to sort newEnv since attrs is in sorted order. for (auto & i : attrs) i.second.e->bindVars(i.second.inherited ? env : newEnv); @@ -342,15 +344,20 @@ void ExprList::bindVars(const StaticEnv & env) void ExprLambda::bindVars(const StaticEnv & env) { - StaticEnv newEnv(false, &env); + StaticEnv newEnv( + false, &env, + (hasFormals() ? formals->formals.size() : 0) + + (arg.empty() ? 0 : 1)); unsigned int displ = 0; - if (!arg.empty()) newEnv.vars[arg] = displ++; + if (!arg.empty()) newEnv.vars.emplace_back(arg, displ++); if (hasFormals()) { for (auto & i : formals->formals) - newEnv.vars[i.name] = displ++; + newEnv.vars.emplace_back(i.name, displ++); + + newEnv.sort(); for (auto & i : formals->formals) if (i.def) i.def->bindVars(newEnv); @@ -361,11 +368,13 @@ void ExprLambda::bindVars(const StaticEnv & env) void ExprLet::bindVars(const StaticEnv & env) { - StaticEnv newEnv(false, &env); + StaticEnv newEnv(false, &env, attrs->attrs.size()); unsigned int displ = 0; for (auto & i : attrs->attrs) - newEnv.vars[i.first] = i.second.displ = displ++; + newEnv.vars.emplace_back(i.first, i.second.displ = displ++); + + // No need to sort newEnv since attrs->attrs is in sorted order. for (auto & i : attrs->attrs) i.second.e->bindVars(i.second.inherited ? env : newEnv); diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 851e875bd..3bfa5e0d3 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -4,8 +4,6 @@ #include "symbol-table.hh" #include "error.hh" -#include - namespace nix { @@ -342,9 +340,28 @@ struct StaticEnv { bool isWith; const StaticEnv * up; - typedef std::map Vars; + + // Note: these must be in sorted order. + typedef std::vector> Vars; Vars vars; - StaticEnv(bool isWith, const StaticEnv * up) : isWith(isWith), up(up) { }; + + StaticEnv(bool isWith, const StaticEnv * up, size_t expectedSize = 0) : isWith(isWith), up(up) { + vars.reserve(expectedSize); + }; + + void sort() + { + std::sort(vars.begin(), vars.end(), + [](const Vars::value_type & a, const Vars::value_type & b) { return a.first < b.first; }); + } + + Vars::const_iterator find(const Symbol & name) const + { + Vars::value_type key(name, 0); + auto i = std::lower_bound(vars.begin(), vars.end(), key); + if (i != vars.end() && i->first == name) return i; + return vars.end(); + } }; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 813ff2fc3..5d0f05206 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -126,14 +126,14 @@ static void addAttr(ExprAttrs * attrs, AttrPath & attrPath, auto j2 = jAttrs->attrs.find(ad.first); if (j2 != jAttrs->attrs.end()) // Attr already defined in iAttrs, error. dupAttr(ad.first, j2->second.pos, ad.second.pos); - jAttrs->attrs[ad.first] = ad.second; + jAttrs->attrs.emplace(ad.first, ad.second); } } else { dupAttr(attrPath, pos, j->second.pos); } } else { // This attr path is not defined. Let's create it. - attrs->attrs[i->symbol] = ExprAttrs::AttrDef(e, pos); + attrs->attrs.emplace(i->symbol, ExprAttrs::AttrDef(e, pos)); e->setName(i->symbol); } } else { @@ -483,7 +483,7 @@ binds if ($$->attrs.find(i.symbol) != $$->attrs.end()) dupAttr(i.symbol, makeCurPos(@3, data), $$->attrs[i.symbol].pos); Pos pos = makeCurPos(@3, data); - $$->attrs[i.symbol] = ExprAttrs::AttrDef(new ExprVar(CUR_POS, i.symbol), pos, true); + $$->attrs.emplace(i.symbol, ExprAttrs::AttrDef(new ExprVar(CUR_POS, i.symbol), pos, true)); } } | binds INHERIT '(' expr ')' attrs ';' @@ -492,7 +492,7 @@ binds for (auto & i : *$6) { if ($$->attrs.find(i.symbol) != $$->attrs.end()) dupAttr(i.symbol, makeCurPos(@6, data), $$->attrs[i.symbol].pos); - $$->attrs[i.symbol] = ExprAttrs::AttrDef(new ExprSelect(CUR_POS, $4, i.symbol), makeCurPos(@6, data)); + $$->attrs.emplace(i.symbol, ExprAttrs::AttrDef(new ExprSelect(CUR_POS, $4, i.symbol), makeCurPos(@6, data))); } } | { $$ = new ExprAttrs(makeCurPos(@0, data)); } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 6b3cafec8..8a573f35c 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -184,14 +184,17 @@ static void import(EvalState & state, const Pos & pos, Value & vPath, Value * vS Env * env = &state.allocEnv(vScope->attrs->size()); env->up = &state.baseEnv; - StaticEnv staticEnv(false, &state.staticBaseEnv); + StaticEnv staticEnv(false, &state.staticBaseEnv, vScope->attrs->size()); unsigned int displ = 0; for (auto & attr : *vScope->attrs) { - staticEnv.vars[attr.name] = displ; + staticEnv.vars.emplace_back(attr.name, displ); env->values[displ++] = attr.value; } + // No need to call staticEnv.sort(), because + // args[0]->attrs is already sorted. + printTalkative("evaluating file '%1%'", realPath); Expr * e = state.parseExprFromFile(resolveExprPath(realPath), staticEnv); @@ -3726,6 +3729,7 @@ void EvalState::createBaseEnv() /* Add a wrapper around the derivation primop that computes the `drvPath' and `outPath' attributes lazily. */ + staticBaseEnv.sort(); sDerivationNix = symbols.create("//builtin/derivation.nix"); eval(parse( #include "primops/derivation.nix.gen.hh" @@ -3735,6 +3739,8 @@ void EvalState::createBaseEnv() /* Now that we've added all primops, sort the `builtins' set, because attribute lookups expect it to be sorted. */ baseEnv.values[0]->attrs->sort(); + + staticBaseEnv.sort(); } diff --git a/src/nix/repl.cc b/src/nix/repl.cc index 9c0d22438..07bf32a0a 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -644,7 +644,8 @@ void NixRepl::addVarToScope(const Symbol & name, Value & v) { if (displ >= envSize) throw Error("environment full; cannot add more variables"); - staticEnv.vars[name] = displ; + staticEnv.vars.emplace_back(name, displ); + staticEnv.sort(); env->values[displ++] = &v; varNames.insert((string) name); } From 81e7c40264520b387358917987d101f5f5ae4705 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 24 Feb 2020 01:32:01 +0100 Subject: [PATCH 2/7] 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); From bcf47800067243b124569da9019077c81bf71cd5 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 24 Feb 2020 14:33:01 +0100 Subject: [PATCH 3/7] Add level / displacement types --- src/libexpr/eval.cc | 8 ++++---- src/libexpr/nixexpr.cc | 10 +++++----- src/libexpr/nixexpr.hh | 11 +++++++---- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index b4b4f7b5c..641687f2a 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -787,7 +787,7 @@ void mkPath(Value & v, const char * s) inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval) { - for (size_t l = var.level; l; --l, env = env->up) ; + for (auto l = var.level; l; --l, env = env->up) ; if (!var.fromWith) return env->values[var.displ]; @@ -1060,7 +1060,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) /* The recursive attributes are evaluated in the new environment, while the inherited attributes are evaluated in the original environment. */ - size_t displ = 0; + Displacement displ = 0; for (auto & i : attrs) { Value * vAttr; if (hasOverrides && !i.second.inherited) { @@ -1136,7 +1136,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) /* The recursive attributes are evaluated in the new environment, while the inherited attributes are evaluated in the original environment. */ - size_t displ = 0; + Displacement displ = 0; for (auto & i : attrs->attrs) env2.values[displ++] = i.second.e->maybeThunk(state, i.second.inherited ? env : env2); @@ -1283,7 +1283,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & Env & env2(allocEnv(size)); env2.up = vCur.lambda.env; - size_t displ = 0; + Displacement displ = 0; if (!lambda.hasFormals()) env2.values[displ++] = args[0]; diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 372e323bc..57c2f6e44 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -273,7 +273,7 @@ void ExprVar::bindVars(const StaticEnv & env) /* Check whether the variable appears in the environment. If so, set its level and displacement. */ const StaticEnv * curEnv; - unsigned int level; + Level level; int withLevel = -1; for (curEnv = &env, level = 0; curEnv; curEnv = curEnv->up, level++) { if (curEnv->isWith) { @@ -326,7 +326,7 @@ void ExprAttrs::bindVars(const StaticEnv & env) if (recursive) { dynamicEnv = &newEnv; - unsigned int displ = 0; + Displacement displ = 0; for (auto & i : attrs) newEnv.vars.emplace_back(i.first, i.second.displ = displ++); @@ -359,7 +359,7 @@ void ExprLambda::bindVars(const StaticEnv & env) (hasFormals() ? formals->formals.size() : 0) + (arg.empty() ? 0 : 1)); - unsigned int displ = 0; + Displacement displ = 0; if (!arg.empty()) newEnv.vars.emplace_back(arg, displ++); @@ -387,7 +387,7 @@ void ExprLet::bindVars(const StaticEnv & env) { StaticEnv newEnv(false, &env, attrs->attrs.size()); - unsigned int displ = 0; + Displacement displ = 0; for (auto & i : attrs->attrs) newEnv.vars.emplace_back(i.first, i.second.displ = displ++); @@ -405,7 +405,7 @@ void ExprWith::bindVars(const StaticEnv & env) level so that `lookupVar' can look up variables in the previous `with' if this one doesn't contain the desired attribute. */ const StaticEnv * curEnv; - unsigned int level; + Level level; prevWith = 0; for (curEnv = &env, level = 1; curEnv; curEnv = curEnv->up, level++) if (curEnv->isWith) { diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index fe12890e9..13256272c 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -133,6 +133,9 @@ struct ExprPath : Expr Value * maybeThunk(EvalState & state, Env & env); }; +typedef uint32_t Level; +typedef uint32_t Displacement; + struct ExprVar : Expr { Pos pos; @@ -148,8 +151,8 @@ struct ExprVar : Expr value is obtained by getting the attribute named `name' from the set stored in the environment that is `level' levels up from the current one.*/ - unsigned int level; - unsigned int displ; + Level level; + Displacement displ; ExprVar(const Symbol & name) : name(name) { }; ExprVar(const Pos & pos, const Symbol & name) : pos(pos), name(name) { }; @@ -183,7 +186,7 @@ struct ExprAttrs : Expr bool inherited; Expr * e; Pos pos; - unsigned int displ; // displacement + Displacement displ; // displacement AttrDef(Expr * e, const Pos & pos, bool inherited=false) : inherited(inherited), e(e), pos(pos) { }; AttrDef() { }; @@ -352,7 +355,7 @@ struct StaticEnv const StaticEnv * up; // Note: these must be in sorted order. - typedef std::vector> Vars; + typedef std::vector> Vars; Vars vars; StaticEnv(bool isWith, const StaticEnv * up, size_t expectedSize = 0) : isWith(isWith), up(up) { From cbfbf71e0821fd79ae4837e7fe03c051437f43dd Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 2 Mar 2020 17:22:31 +0100 Subject: [PATCH 4/7] Use callFunction() with an array for some calls with arity > 1 --- src/libexpr/primops.cc | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 8a573f35c..9b166d63c 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1883,9 +1883,6 @@ static void addPath( Value arg1; mkString(arg1, path); - Value fun2; - state.callFunction(*filterFun, arg1, fun2, noPos); - Value arg2; mkString(arg2, S_ISREG(st.st_mode) ? "regular" : @@ -1893,8 +1890,9 @@ static void addPath( S_ISLNK(st.st_mode) ? "symlink" : "unknown" /* not supported, will fail! */); + Value * args []{&arg1, &arg2}; Value res; - state.callFunction(fun2, arg2, res, noPos); + state.callFunction(*filterFun, 2, args, res, pos); return state.forceBool(res, pos); }) : defaultPathFilter; @@ -2695,10 +2693,9 @@ static void prim_foldlStrict(EvalState & state, const Pos & pos, Value * * args, Value * vCur = args[1]; for (unsigned int n = 0; n < args[2]->listSize(); ++n) { - Value vTmp; - state.callFunction(*args[0], *vCur, vTmp, pos); + Value * vs []{vCur, args[2]->listElems()[n]}; vCur = n == args[2]->listSize() - 1 ? &v : state.allocValue(); - state.callFunction(vTmp, *args[2]->listElems()[n], *vCur, pos); + state.callFunction(*args[0], 2, vs, *vCur, pos); } state.forceValue(v, pos); } else { @@ -2819,17 +2816,16 @@ static void prim_sort(EvalState & state, const Pos & pos, Value * * args, Value v.listElems()[n] = args[1]->listElems()[n]; } - auto comparator = [&](Value * a, Value * b) { /* Optimization: if the comparator is lessThan, bypass callFunction. */ if (args[0]->isPrimOp() && args[0]->primOp->fun == prim_lessThan) return CompareValues()(a, b); - Value vTmp1, vTmp2; - state.callFunction(*args[0], *a, vTmp1, pos); - state.callFunction(vTmp1, *b, vTmp2, pos); - return state.forceBool(vTmp2, pos); + Value * vs[] = {a, b}; + Value vBool; + state.callFunction(*args[0], 2, vs, vBool, pos); + return state.forceBool(vBool, pos); }; /* FIXME: std::sort can segfault if the comparator is not a strict From acd6bddec706e2f180279ca79a0c1a4abac416d6 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 2 Mar 2020 18:15:06 +0100 Subject: [PATCH 5/7] Fix derivation primop --- src/libexpr/eval.cc | 14 ++++++++++---- src/libexpr/eval.hh | 2 ++ src/libexpr/primops.cc | 13 ++++++++----- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 641687f2a..d371a3fb8 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -583,14 +583,20 @@ Value * EvalState::addConstant(const string & name, Value & v) { Value * v2 = allocValue(); *v2 = v; - staticBaseEnv.vars.emplace_back(symbols.create(name), baseEnvDispl); - baseEnv.values[baseEnvDispl++] = v2; - string name2 = string(name, 0, 2) == "__" ? string(name, 2) : name; - baseEnv.values[0]->attrs->push_back(Attr(symbols.create(name2), v2)); + addConstant(name, v2); return v2; } +void EvalState::addConstant(const string & name, Value * v) +{ + staticBaseEnv.vars.emplace_back(symbols.create(name), baseEnvDispl); + baseEnv.values[baseEnvDispl++] = v; + string name2 = string(name, 0, 2) == "__" ? string(name, 2) : name; + baseEnv.values[0]->attrs->push_back(Attr(symbols.create(name2), v)); +} + + Value * EvalState::addPrimOp(const string & name, size_t arity, PrimOpFun primOp) { diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index f87dcdd8e..65cf04b57 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -277,6 +277,8 @@ private: Value * addConstant(const string & name, Value & v); + void addConstant(const string & name, Value * v); + constexpr static size_t maxPrimOpArity = 3; Value * addPrimOp(const string & name, diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 9b166d63c..e4107dbe1 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -3725,18 +3725,21 @@ void EvalState::createBaseEnv() /* Add a wrapper around the derivation primop that computes the `drvPath' and `outPath' attributes lazily. */ - staticBaseEnv.sort(); sDerivationNix = symbols.create("//builtin/derivation.nix"); - eval(parse( - #include "primops/derivation.nix.gen.hh" - , foFile, sDerivationNix, "/", staticBaseEnv), v); - addConstant("derivation", v); + auto vDerivation = allocValue(); + addConstant("derivation", vDerivation); /* Now that we've added all primops, sort the `builtins' set, because attribute lookups expect it to be sorted. */ baseEnv.values[0]->attrs->sort(); staticBaseEnv.sort(); + + /* Note: we have to initialize the 'derivation' constant *after* + building baseEnv/staticBaseEnv because it uses 'builtins'. */ + eval(parse( + #include "primops/derivation.nix.gen.hh" + , foFile, sDerivationNix, "/", staticBaseEnv), *vDerivation); } From 05560f6350f5e330c286e048b8fbbb24dafbf16b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 2 Mar 2020 18:16:49 +0100 Subject: [PATCH 6/7] Fix function-trace test case --- tests/function-trace.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/function-trace.sh b/tests/function-trace.sh index 3b7f364e3..0b7f49d82 100755 --- a/tests/function-trace.sh +++ b/tests/function-trace.sh @@ -60,8 +60,6 @@ function-trace exited (string):1:1 at expect_trace '(x: x) 1 2' " function-trace entered (string):1:1 at function-trace exited (string):1:1 at -function-trace entered (string):1:1 at -function-trace exited (string):1:1 at " # Not a function From 40925337a972991468335b90ce4f685f7102d830 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 2 Mar 2020 22:41:56 +0100 Subject: [PATCH 7/7] Remove maxPrimOpArity --- src/libexpr/eval.cc | 2 -- src/libexpr/eval.hh | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index d371a3fb8..402de78ad 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -600,8 +600,6 @@ void 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); diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 65cf04b57..1aab8e166 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -279,8 +279,6 @@ private: void addConstant(const string & name, Value * v); - constexpr static size_t maxPrimOpArity = 3; - Value * addPrimOp(const string & name, size_t arity, PrimOpFun primOp);