Merge pull request #9582 from pennae/misc-opts

a packet of small optimizations

(cherry picked from commit ee439734e924eb337a869ff2e48aff8b989198bc)
Change-Id: I125d870710750a32a0dece48f39a3e9132b0d023

src/libexpr/eval-inline.hh

@@ -83,13 +83,6 @@ Env & EvalState::allocEnv(size_t size)
 
 [[gnu::always_inline]]
 void EvalState::forceValue(Value & v, const PosIdx pos)
-{
-    forceValue(v, [&]() { return pos; });
-}
-
-
-template<typename Callable>
-void EvalState::forceValue(Value & v, Callable getPos)
 {
     if (v.isThunk()) {
         Env * env = v.thunk.env;
@@ -100,15 +93,12 @@ void EvalState::forceValue(Value & v, Callable getPos)
             expr->eval(*this, *env, v);
         } catch (...) {
             v.mkThunk(env, expr);
+            tryFixupBlackHolePos(v, pos);
             throw;
         }
     }
-    else if (v.isApp()) {
-        PosIdx pos = getPos();
+    else if (v.isApp())
         callFunction(*v.app.left, *v.app.right, v, pos);
-    }
-    else if (v.isBlackhole())
-        error("infinite recursion encountered").atPos(getPos()).template debugThrow<EvalError>();
 }
 
 

src/libexpr/eval.cc

@@ -158,7 +158,17 @@ void Value::print(const SymbolTable &symbols, std::ostream &str,
         break;
     case tThunk:
     case tApp:
-        str << "<CODE>";
+        if (!isBlackhole()) {
+            str << "<CODE>";
+        } else {
+            // Although we know for sure that it's going to be an infinite recursion
+            // when this value is accessed _in the current context_, it's likely
+            // that the user will misinterpret a simpler «infinite recursion» output
+            // as a definitive statement about the value, while in fact it may be
+            // a valid value after `builtins.trace` and perhaps some other steps
+            // have completed.
+            str << "«potential infinite recursion»";
+        }
         break;
     case tLambda:
         str << "<LAMBDA>";
@@ -175,15 +185,6 @@ void Value::print(const SymbolTable &symbols, std::ostream &str,
     case tFloat:
         str << fpoint;
         break;
-    case tBlackhole:
-        // Although we know for sure that it's going to be an infinite recursion
-        // when this value is accessed _in the current context_, it's likely
-        // that the user will misinterpret a simpler «infinite recursion» output
-        // as a definitive statement about the value, while in fact it may be
-        // a valid value after `builtins.trace` and perhaps some other steps
-        // have completed.
-        str << "«potential infinite recursion»";
-        break;
     default:
         printError("Nix evaluator internal error: Value::print(): invalid value type %1%", internalType);
         abort();
@@ -251,9 +252,8 @@ std::string showType(const Value & v)
         case tPrimOpApp:
             return fmt("the partially applied built-in function '%s'", std::string(getPrimOp(v)->primOp->name));
         case tExternal: return v.external->showType();
-        case tThunk: return "a thunk";
+        case tThunk: return v.isBlackhole() ? "a black hole" : "a thunk";
         case tApp: return "a function application";
-        case tBlackhole: return "a black hole";
     default:
         return std::string(showType(v.type()));
     }
@@ -1666,15 +1666,15 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                 return;
             } else {
                 /* We have all the arguments, so call the primop. */
-                auto name = vCur.primOp->name;
+                auto * fn = vCur.primOp;
 
                 nrPrimOpCalls++;
-                if (countCalls) primOpCalls[name]++;
+                if (countCalls) primOpCalls[fn->name]++;
 
                 try {
-                    vCur.primOp->fun(*this, vCur.determinePos(noPos), args, vCur);
+                    fn->fun(*this, vCur.determinePos(noPos), args, vCur);
                 } catch (Error & e) {
-                    addErrorTrace(e, pos, "while calling the '%1%' builtin", name);
+                    addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
                     throw;
                 }
 
@@ -1711,18 +1711,18 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
                 for (size_t i = 0; i < argsLeft; ++i)
                     vArgs[argsDone + i] = args[i];
 
-                auto name = primOp->primOp->name;
+                auto fn = primOp->primOp;
                 nrPrimOpCalls++;
-                if (countCalls) primOpCalls[name]++;
+                if (countCalls) primOpCalls[fn->name]++;
 
                 try {
                     // TODO:
                     // 1. Unify this and above code. Heavily redundant.
                     // 2. Create a fake env (arg1, arg2, etc.) and a fake expr (arg1: arg2: etc: builtins.name arg1 arg2 etc)
                     //    so the debugger allows to inspect the wrong parameters passed to the builtin.
-                    primOp->primOp->fun(*this, vCur.determinePos(noPos), vArgs, vCur);
+                    fn->fun(*this, vCur.determinePos(noPos), vArgs, vCur);
                 } catch (Error & e) {
-                    addErrorTrace(e, pos, "while calling the '%1%' builtin", name);
+                    addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name);
                     throw;
                 }
 
@@ -2076,6 +2076,29 @@ void ExprPos::eval(EvalState & state, Env & env, Value & v)
 }
 
 
+void ExprBlackHole::eval(EvalState & state, Env & env, Value & v)
+{
+    state.error("infinite recursion encountered")
+        .debugThrow<InfiniteRecursionError>();
+}
+
+// always force this to be separate, otherwise forceValue may inline it and take
+// a massive perf hit
+[[gnu::noinline]]
+void EvalState::tryFixupBlackHolePos(Value & v, PosIdx pos)
+{
+    if (!v.isBlackhole())
+        return;
+    auto e = std::current_exception();
+    try {
+        std::rethrow_exception(e);
+    } catch (InfiniteRecursionError & e) {
+        e.err.errPos = positions[pos];
+    } catch (...) {
+    }
+}
+
+
 void EvalState::forceValueDeep(Value & v)
 {
     std::set<const Value *> seen;
@@ -2085,7 +2108,7 @@ void EvalState::forceValueDeep(Value & v)
     recurse = [&](Value & v) {
         if (!seen.insert(&v).second) return;
 
-        forceValue(v, [&]() { return v.determinePos(noPos); });
+        forceValue(v, v.determinePos(noPos));
 
         if (v.type() == nAttrs) {
             for (auto & i : *v.attrs)

src/libexpr/eval.hh

@@ -467,8 +467,7 @@ public:
      */
     inline void forceValue(Value & v, const PosIdx pos);
 
-    template <typename Callable>
-    inline void forceValue(Value & v, Callable getPos);
+    void tryFixupBlackHolePos(Value & v, PosIdx pos);
 
     /**
      * Force a value, then recursively force list elements and

src/libexpr/get-drvs.cc

@@ -199,7 +199,7 @@ StringSet DrvInfo::queryMetaNames()
 
 bool DrvInfo::checkMeta(Value & v)
 {
-    state->forceValue(v, [&]() { return v.determinePos(noPos); });
+    state->forceValue(v, v.determinePos(noPos));
     if (v.type() == nList) {
         for (auto elem : v.listItems())
             if (!checkMeta(*elem)) return false;
@@ -305,7 +305,7 @@ static bool getDerivation(EvalState & state, Value & v,
     bool ignoreAssertionFailures)
 {
     try {
-        state.forceValue(v, [&]() { return v.determinePos(noPos); });
+        state.forceValue(v, v.determinePos(noPos));
         if (!state.isDerivation(v)) return true;
 
         /* Remove spurious duplicates (e.g., a set like `rec { x =

src/libexpr/lexer.l

@@ -1,4 +1,5 @@
 %option reentrant bison-bridge bison-locations
+%option align
 %option noyywrap
 %option never-interactive
 %option stack
@@ -35,9 +36,6 @@ static inline PosIdx makeCurPos(const YYLTYPE & loc, ParseData * data)
 
 #define CUR_POS makeCurPos(*yylloc, data)
 
-// backup to recover from yyless(0)
-thread_local YYLTYPE prev_yylloc;
-
 static void initLoc(YYLTYPE * loc)
 {
     loc->first_line = loc->last_line = 1;
@@ -46,7 +44,7 @@ static void initLoc(YYLTYPE * loc)
 
 static void adjustLoc(YYLTYPE * loc, const char * s, size_t len)
 {
-    prev_yylloc = *loc;
+    loc->stash();
 
     loc->first_line = loc->last_line;
     loc->first_column = loc->last_column;
@@ -230,7 +228,7 @@ or          { return OR_KW; }
 {HPATH_START}\$\{ {
   PUSH_STATE(PATH_START);
   yyless(0);
-  *yylloc = prev_yylloc;
+  yylloc->unstash();
 }
 
 <PATH_START>{PATH_SEG} {
@@ -286,7 +284,7 @@ or          { return OR_KW; }
      context (it may be ')', ';', or something of that sort) */
   POP_STATE();
   yyless(0);
-  *yylloc = prev_yylloc;
+  yylloc->unstash();
   return PATH_END;
 }
 

src/libexpr/nixexpr.cc

@@ -9,6 +9,8 @@
 
 namespace nix {
 
+ExprBlackHole eBlackHole;
+
 struct PosAdapter : AbstractPos
 {
     Pos::Origin origin;

src/libexpr/nixexpr.hh

@@ -22,6 +22,13 @@ MakeError(UndefinedVarError, Error);
 MakeError(MissingArgumentError, EvalError);
 MakeError(RestrictedPathError, Error);
 
+class InfiniteRecursionError : public EvalError
+{
+    friend class EvalState;
+public:
+    using EvalError::EvalError;
+};
+
 /**
  * Position objects.
  */
@@ -450,6 +457,16 @@ struct ExprPos : Expr
     COMMON_METHODS
 };
 
+/* only used to mark thunks as black holes. */
+struct ExprBlackHole : Expr
+{
+    void show(const SymbolTable & symbols, std::ostream & str) const override {}
+    void eval(EvalState & state, Env & env, Value & v) override;
+    void bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env) override {}
+};
+
+extern ExprBlackHole eBlackHole;
+
 
 /* Static environments are used to map variable names onto (level,
    displacement) pairs used to obtain the value of the variable at

src/libexpr/parser.y

@@ -27,6 +27,31 @@
 
 namespace nix {
 
+#define YYLTYPE ::nix::ParserLocation
+    struct ParserLocation
+    {
+        int first_line, first_column;
+        int last_line, last_column;
+
+        // backup to recover from yyless(0)
+        int stashed_first_line, stashed_first_column;
+        int stashed_last_line, stashed_last_column;
+
+        void stash() {
+          stashed_first_line = first_line;
+          stashed_first_column = first_column;
+          stashed_last_line = last_line;
+          stashed_last_column = last_column;
+        }
+
+        void unstash() {
+          first_line = stashed_first_line;
+          first_column = stashed_first_column;
+          last_line = stashed_last_line;
+          last_column = stashed_last_column;
+        }
+    };
+
     struct ParseData
     {
         EvalState & state;

src/libexpr/value.hh

@@ -31,7 +31,6 @@ typedef enum {
     tThunk,
     tApp,
     tLambda,
-    tBlackhole,
     tPrimOp,
     tPrimOpApp,
     tExternal,
@@ -61,6 +60,7 @@ class Bindings;
 struct Env;
 struct Expr;
 struct ExprLambda;
+struct ExprBlackHole;
 struct PrimOp;
 class Symbol;
 class PosIdx;
@@ -151,7 +151,7 @@ public:
     // type() == nThunk
     inline bool isThunk() const { return internalType == tThunk; };
     inline bool isApp() const { return internalType == tApp; };
-    inline bool isBlackhole() const { return internalType == tBlackhole; };
+    inline bool isBlackhole() const;
 
     // type() == nFunction
     inline bool isLambda() const { return internalType == tLambda; };
@@ -236,7 +236,7 @@ public:
             case tLambda: case tPrimOp: case tPrimOpApp: return nFunction;
             case tExternal: return nExternal;
             case tFloat: return nFloat;
-            case tThunk: case tApp: case tBlackhole: return nThunk;
+            case tThunk: case tApp: return nThunk;
         }
         if (invalidIsThunk)
             return nThunk;
@@ -343,11 +343,7 @@ public:
         lambda.fun = f;
     }
 
-    inline void mkBlackhole()
-    {
-        internalType = tBlackhole;
-        // Value will be overridden anyways
-    }
+    inline void mkBlackhole();
 
     void mkPrimOp(PrimOp * p);
 
@@ -443,6 +439,20 @@ public:
 };
 
 
+extern ExprBlackHole eBlackHole;
+
+bool Value::isBlackhole() const
+{
+    return internalType == tThunk && thunk.expr == (Expr*) &eBlackHole;
+}
+
+void Value::mkBlackhole()
+{
+    internalType = tThunk;
+    thunk.expr = (Expr*) &eBlackHole;
+}
+
+
 #if HAVE_BOEHMGC
 typedef std::vector<Value *, traceable_allocator<Value *>> ValueVector;
 typedef std::map<Symbol, Value *, std::less<Symbol>, traceable_allocator<std::pair<const Symbol, Value *>>> ValueMap;