Merge pull request #5501 from edolstra/optimize-calls

Optimize primop calls

src/libexpr/eval.cc

@@ -583,14 +583,20 @@ Value * EvalState::addConstant(const string & name, Value & v)
 {
     Value * v2 = allocValue();
     *v2 = v;
-    staticBaseEnv.vars[symbols.create(name)] = baseEnvDispl;
+    addConstant(name, v2);
-    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));
     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)
 {
@@ -609,7 +615,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 +641,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;
@@ -785,7 +791,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];
 
@@ -1058,7 +1064,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) {
@@ -1134,7 +1140,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);
 
@@ -1251,105 +1257,54 @@ void ExprLambda::eval(EvalState & state, Env & env, Value & v)
 }
 
 
-void ExprApp::eval(EvalState & state, Env & env, Value & v)
+void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & vRes, const Pos & pos)
-{
-    /* 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)
 {
     auto trace = evalSettings.traceFunctionCalls ? std::make_unique<FunctionCallTrace>(pos) : nullptr;
 
     forceValue(fun, pos);
 
-    if (fun.isPrimOp() || fun.isPrimOpApp()) {
+    Value vCur(fun);
-        callPrimOp(fun, arg, v, pos);
-        return;
-    }
 
-    if (fun.type() == nAttrs) {
+    auto makeAppChain = [&]()
-      auto found = fun.attrs->find(sFunctor);
+    {
-      if (found != fun.attrs->end()) {
+        vRes = vCur;
-        /* fun may be allocated on the stack of the calling function,
+        for (size_t i = 0; i < nrArgs; ++i) {
-         * but for functors we may keep a reference, so heap-allocate
+            auto fun2 = allocValue();
-         * a copy and use that instead.
+            *fun2 = vRes;
-         */
+            vRes.mkPrimOpApp(fun2, args[i]);
-        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);
-      }
         }
+    };
 
-    if (!fun.isLambda())
+    while (nrArgs > 0) {
-        throwTypeError(pos, "attempt to call something which is not a function but %1%", fun);
 
-    ExprLambda & lambda(*fun.lambda.fun);
+        if (vCur.isLambda()) {
+
+            ExprLambda & lambda(*vCur.lambda.fun);
 
             auto size =
                 (lambda.arg.empty() ? 0 : 1) +
                 (lambda.hasFormals() ? lambda.formals->formals.size() : 0);
             Env & env2(allocEnv(size));
-    env2.up = fun.lambda.env;
+            env2.up = vCur.lambda.env;
 
-    size_t displ = 0;
+            Displacement displ = 0;
 
             if (!lambda.hasFormals())
-        env2.values[displ++] = &arg;
+                env2.values[displ++] = args[0];
 
             else {
-        forceAttrs(arg, pos);
+                forceAttrs(*args[0], pos);
 
                 if (!lambda.arg.empty())
-            env2.values[displ++] = &arg;
+                    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) {
-            Bindings::iterator j = arg.attrs->find(i.name);
+                    auto j = args[0]->attrs->get(i.name);
-            if (j == arg.attrs->end()) {
+                    if (!j) {
                         if (!i.def) throwTypeError(pos, "%1% called without required argument '%2%'",
                             lambda, i.name);
                         env2.values[displ++] = i.def->maybeThunk(*this, env2);
@@ -1361,10 +1316,10 @@ void EvalState::callFunction(Value & fun, Value & arg, Value & v, const Pos & po
 
                 /* 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()) {
+                if (!lambda.formals->ellipsis && attrsUsed != args[0]->attrs->size()) {
                     /* Nope, so show the first unexpected argument to the
                        user. */
-            for (auto & i : *arg.attrs)
+                    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
@@ -1374,21 +1329,110 @@ void EvalState::callFunction(Value & fun, Value & arg, Value & v, const Pos & po
             nrFunctionCalls++;
             if (countCalls) incrFunctionCall(&lambda);
 
-    /* Evaluate the body.  This is conditional on showTrace, because
+            /* Evaluate the body. */
-       catching exceptions makes this function not tail-recursive. */
-    if (loggerSettings.showTrace.get())
             try {
-            lambda.body->eval(*this, env2, v);
+                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 {
+                /* 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;
+            }
+        }
+
+        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
-        fun.lambda.fun->body->eval(*this, env2, v);
+            throwTypeError(pos, "attempt to call something which is not a function but %1%", vCur);
+    }
+
+    vRes = vCur;
+}
+
+
+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);
 }
 
 

src/libexpr/eval.hh

@@ -277,6 +277,8 @@ private:
 
     Value * addConstant(const string & name, Value & v);
 
+    void addConstant(const string & name, Value * v);
+
     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);
+    // FIXME: use std::span
-    void callPrimOp(Value & fun, Value & arg, Value & v, const Pos & pos);
+    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. */

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;

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 ";
@@ -263,13 +273,13 @@ 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) {
             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 +321,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;
+        Displacement 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 +354,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;
+    Displacement 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);
@@ -359,13 +376,22 @@ 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);
+    StaticEnv newEnv(false, &env, attrs->attrs.size());
 
-    unsigned int displ = 0;
+    Displacement 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);
@@ -379,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) {
@@ -452,5 +478,4 @@ size_t SymbolTable::totalSize() const
     return n;
 }
 
-
 }

src/libexpr/nixexpr.hh

@@ -4,8 +4,6 @@
 #include "symbol-table.hh"
 #include "error.hh"
 
-#include <map>
-
 
 namespace nix {
 
@@ -135,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;
@@ -150,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;
+    Level level;
-    unsigned int displ;
+    Displacement displ;
 
     ExprVar(const Symbol & name) : name(name) { };
     ExprVar(const Pos & pos, const Symbol & name) : pos(pos), name(name) { };
@@ -185,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() { };
@@ -250,6 +251,17 @@ struct ExprLambda : Expr
     COMMON_METHODS
 };
 
+struct ExprCall : Expr
+{
+    Expr * fun;
+    std::vector<Expr *> args;
+    Pos pos;
+    ExprCall(const Pos & pos, Expr * fun, std::vector<Expr *> && args)
+        : fun(fun), args(args), pos(pos)
+    { }
+    COMMON_METHODS
+};
+
 struct ExprLet : Expr
 {
     ExprAttrs * attrs;
@@ -308,7 +320,6 @@ struct ExprOpNot : Expr
         void eval(EvalState & state, Env & env, Value & v); \
     };
 
-MakeBinOp(ExprApp, "")
 MakeBinOp(ExprOpEq, "==")
 MakeBinOp(ExprOpNEq, "!=")
 MakeBinOp(ExprOpAnd, "&&")
@@ -342,9 +353,28 @@ struct StaticEnv
 {
     bool isWith;
     const StaticEnv * up;
-    typedef std::map<Symbol, unsigned int> Vars;
+
+    // Note: these must be in sorted order.
+    typedef std::vector<std::pair<Symbol, Displacement>> 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();
+    }
 };
 
 

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 \
@@ -126,14 +132,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 {
@@ -280,10 +286,12 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParseData * data, const char * err
   char * uri;
   std::vector<nix::AttrName> * attrNames;
   std::vector<nix::Expr *> * string_parts;
+  nix::App app; // bool == whether this is an ExprCall
 }
 
 %type <e> start expr expr_function expr_if expr_op
-%type <e> expr_app expr_select expr_simple
+%type <e> expr_select expr_simple
+%type <app> expr_app
 %type <list> expr_list
 %type <attrs> binds
 %type <formals> 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 '<' expr_op { $$ = new ExprCall(CUR_POS, 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 LEQ expr_op { $$ = new ExprOpNot(new ExprCall(CUR_POS, 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 '>' expr_op { $$ = new ExprCall(CUR_POS, 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 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<Expr *>({$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 ExprCall(CUR_POS, 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 ExprCall(CUR_POS, 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("__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
+  : expr_app expr_select {
-    { $$ = new ExprApp(CUR_POS, $1, $2); }
+      if ($1.isCall) {
-  | expr_select { $$ = $1; }
+          ((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 ExprCall(CUR_POS,
-          new ExprApp(new ExprVar(data->symbols.create("__findFile")),
+          new ExprVar(data->symbols.create("__findFile")),
-              new ExprVar(data->symbols.create("__nixPath"))),
+          {new ExprVar(data->symbols.create("__nixPath")),
-          new ExprString(data->symbols.create(path)));
+           new ExprString(data->symbols.create(path))});
   }
   | URI {
       static bool noURLLiterals = settings.isExperimentalFeatureEnabled(Xp::NoUrlLiterals);
@@ -483,7 +498,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 +507,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)); }

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);
 
@@ -1880,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" :
@@ -1890,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;
@@ -2692,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;
+            Value * vs []{vCur, args[2]->listElems()[n]};
-            state.callFunction(*args[0], *vCur, vTmp, pos);
             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 {
@@ -2816,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;
+        Value * vs[] = {a, b};
-        state.callFunction(*args[0], *a, vTmp1, pos);
+        Value vBool;
-        state.callFunction(vTmp1, *b, vTmp2, pos);
+        state.callFunction(*args[0], 2, vs, vBool, pos);
-        return state.forceBool(vTmp2, pos);
+        return state.forceBool(vBool, pos);
     };
 
     /* FIXME: std::sort can segfault if the comparator is not a strict
@@ -3727,14 +3726,20 @@ void EvalState::createBaseEnv()
     /* Add a wrapper around the derivation primop that computes the
        `drvPath' and `outPath' attributes lazily. */
     sDerivationNix = symbols.create("//builtin/derivation.nix");
-    eval(parse(
+    auto vDerivation = allocValue();
-        #include "primops/derivation.nix.gen.hh"
+    addConstant("derivation", vDerivation);
-        , foFile, sDerivationNix, "/", staticBaseEnv), v);
-    addConstant("derivation", v);
 
     /* 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);
 }
 
 

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);
 }

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