Merge pull request #9582 from pennae/misc-opts

a packet of small optimizations

src/libcmd/built-path.cc

@@ -12,9 +12,9 @@ namespace nix {
     bool MY_TYPE ::operator COMPARATOR (const MY_TYPE & other) const \
     { \
         const MY_TYPE* me = this; \
-        auto fields1 = std::make_tuple<const CHILD_TYPE &, const FIELD_TYPE &>(*me->drvPath, me->FIELD); \
+        auto fields1 = std::tie(*me->drvPath, me->FIELD); \
         me = &other; \
-        auto fields2 = std::make_tuple<const CHILD_TYPE &, const FIELD_TYPE &>(*me->drvPath, me->FIELD); \
+        auto fields2 = std::tie(*me->drvPath, me->FIELD); \
         return fields1 COMPARATOR fields2; \
     }
 #define CMP(CHILD_TYPE, MY_TYPE, FIELD) \

src/libcmd/installable-flake.cc

@@ -52,7 +52,7 @@ Value * InstallableFlake::getFlakeOutputs(EvalState & state, const flake::Locked
     auto aOutputs = vFlake->attrs->get(state.symbols.create("outputs"));
     assert(aOutputs);
 
-    state.forceValue(*aOutputs->value, [&]() { return aOutputs->value->determinePos(noPos); });
+    state.forceValue(*aOutputs->value, aOutputs->value->determinePos(noPos));
 
     return aOutputs->value;
 }

src/libcmd/repl.cc

@@ -888,7 +888,7 @@ void NixRepl::evalString(std::string s, Value & v)
 {
     Expr * e = parseString(s);
     e->eval(*state, *env, v);
-    state->forceValue(v, [&]() { return v.determinePos(noPos); });
+    state->forceValue(v, v.determinePos(noPos));
 }
 
 
@@ -907,7 +907,7 @@ std::ostream & NixRepl::printValue(std::ostream & str, Value & v, unsigned int m
     str.flush();
     checkInterrupt();
 
-    state->forceValue(v, [&]() { return v.determinePos(noPos); });
+    state->forceValue(v, v.determinePos(noPos));
 
     switch (v.type()) {
 

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()) {
+    else if (v.isApp())
-        PosIdx pos = getPos();
         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

@@ -163,7 +163,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>";
@@ -180,15 +190,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();
@@ -256,9 +257,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()));
     }
@@ -1646,15 +1646,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;
                 }
 
@@ -1691,18 +1691,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;
                 }
 
@@ -2056,6 +2056,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;
@@ -2065,7 +2088,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)
@@ -2457,7 +2480,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v
             return v1.boolean == v2.boolean;
 
         case nString:
-            return v1.string_view().compare(v2.string_view()) == 0;
+            return strcmp(v1.c_str(), v2.c_str()) == 0;
 
         case nPath:
             return

src/libexpr/eval.hh

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

src/libexpr/get-drvs.cc

@@ -198,7 +198,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;
@@ -304,7 +304,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

@@ -21,6 +21,13 @@ MakeError(TypeError, EvalError);
 MakeError(UndefinedVarError, Error);
 MakeError(MissingArgumentError, EvalError);
 
+class InfiniteRecursionError : public EvalError
+{
+    friend class EvalState;
+public:
+    using EvalError::EvalError;
+};
+
 /**
  * Position objects.
  */
@@ -455,6 +462,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

@@ -28,6 +28,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/primops.cc

@@ -584,7 +584,7 @@ struct CompareValues
                 case nFloat:
                     return v1->fpoint < v2->fpoint;
                 case nString:
-                    return v1->string_view().compare(v2->string_view()) < 0;
+                    return strcmp(v1->c_str(), v2->c_str()) < 0;
                 case nPath:
                     // Note: we don't take the accessor into account
                     // since it's not obvious how to compare them in a
@@ -2405,7 +2405,7 @@ static void prim_attrNames(EvalState & state, const PosIdx pos, Value * * args,
         (v.listElems()[n++] = state.allocValue())->mkString(state.symbols[i.name]);
 
     std::sort(v.listElems(), v.listElems() + n,
-              [](Value * v1, Value * v2) { return v1->string_view().compare(v2->string_view()) < 0; });
+              [](Value * v1, Value * v2) { return strcmp(v1->c_str(), v2->c_str()) < 0; });
 }
 
 static RegisterPrimOp primop_attrNames({

src/libexpr/value.hh

@@ -32,7 +32,6 @@ typedef enum {
     tThunk,
     tApp,
     tLambda,
-    tBlackhole,
     tPrimOp,
     tPrimOpApp,
     tExternal,
@@ -62,6 +61,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; };
@@ -248,7 +248,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;
@@ -356,11 +356,7 @@ public:
         lambda.fun = f;
     }
 
-    inline void mkBlackhole()
+    inline void mkBlackhole();
-    {
-        internalType = tBlackhole;
-        // Value will be overridden anyways
-    }
 
     void mkPrimOp(PrimOp * p);
 
@@ -447,6 +443,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;

src/libstore/derived-path.cc

@@ -12,9 +12,9 @@ namespace nix {
     bool MY_TYPE ::operator COMPARATOR (const MY_TYPE & other) const \
     { \
         const MY_TYPE* me = this; \
-        auto fields1 = std::make_tuple<const CHILD_TYPE &, const FIELD_TYPE &>(*me->drvPath, me->FIELD); \
+        auto fields1 = std::tie(*me->drvPath, me->FIELD); \
         me = &other; \
-        auto fields2 = std::make_tuple<const CHILD_TYPE &, const FIELD_TYPE &>(*me->drvPath, me->FIELD); \
+        auto fields2 = std::tie(*me->drvPath, me->FIELD); \
         return fields1 COMPARATOR fields2; \
     }
 #define CMP(CHILD_TYPE, MY_TYPE, FIELD) \
@@ -22,13 +22,9 @@ namespace nix {
     CMP_ONE(CHILD_TYPE, MY_TYPE, FIELD, !=) \
     CMP_ONE(CHILD_TYPE, MY_TYPE, FIELD, <)
 
-#define FIELD_TYPE std::string
 CMP(SingleDerivedPath, SingleDerivedPathBuilt, output)
-#undef FIELD_TYPE
 
-#define FIELD_TYPE OutputsSpec
 CMP(SingleDerivedPath, DerivedPathBuilt, outputs)
-#undef FIELD_TYPE
 
 #undef CMP
 #undef CMP_ONE

src/libstore/local-store.cc

@@ -19,6 +19,8 @@
 #include <algorithm>
 #include <cstring>
 
+#include <memory>
+#include <new>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/select.h>
@@ -1147,7 +1149,11 @@ StorePath LocalStore::addToStoreFromDump(
        path. */
     bool inMemory = false;
 
-    std::string dump;
+    struct Free {
+        void operator()(void* v) { free(v); }
+    };
+    std::unique_ptr<char, Free> dumpBuffer(nullptr);
+    std::string_view dump;
 
     /* Fill out buffer, and decide whether we are working strictly in
        memory based on whether we break out because the buffer is full
@@ -1156,13 +1162,18 @@ StorePath LocalStore::addToStoreFromDump(
         auto oldSize = dump.size();
         constexpr size_t chunkSize = 65536;
         auto want = std::min(chunkSize, settings.narBufferSize - oldSize);
-        dump.resize(oldSize + want);
+        if (auto tmp = realloc(dumpBuffer.get(), oldSize + want)) {
+            dumpBuffer.release();
+            dumpBuffer.reset((char*) tmp);
+        } else {
+            throw std::bad_alloc();
+        }
         auto got = 0;
         Finally cleanup([&]() {
-            dump.resize(oldSize + got);
+            dump = {dumpBuffer.get(), dump.size() + got};
         });
         try {
-            got = source.read(dump.data() + oldSize, want);
+            got = source.read(dumpBuffer.get() + oldSize, want);
         } catch (EndOfFile &) {
             inMemory = true;
             break;
@@ -1185,7 +1196,8 @@ StorePath LocalStore::addToStoreFromDump(
 
         restorePath(tempPath, bothSource, method.getFileIngestionMethod());
 
-        dump.clear();
+        dumpBuffer.reset();
+        dump = {};
     }
 
     auto [hash, size] = hashSink->finish();

src/libutil/comparator.hh

@@ -13,9 +13,9 @@
 #define GENERATE_ONE_CMP(PRE, QUAL, COMPARATOR, MY_TYPE, ...) \
     PRE bool QUAL operator COMPARATOR(const MY_TYPE & other) const { \
       __VA_OPT__(const MY_TYPE * me = this;) \
-      auto fields1 = std::make_tuple( __VA_ARGS__ ); \
+      auto fields1 = std::tie( __VA_ARGS__ ); \
       __VA_OPT__(me = &other;) \
-      auto fields2 = std::make_tuple( __VA_ARGS__ ); \
+      auto fields2 = std::tie( __VA_ARGS__ ); \
       return fields1 COMPARATOR fields2; \
     }
 #define GENERATE_EQUAL(prefix, qualification, my_type, args...) \

src/libutil/posix-source-accessor.cc

@@ -25,7 +25,7 @@ void PosixSourceAccessor::readFile(
 
     off_t left = st.st_size;
 
-    std::vector<unsigned char> buf(64 * 1024);
+    std::array<unsigned char, 64 * 1024> buf;
     while (left) {
         checkInterrupt();
         ssize_t rd = read(fd.get(), buf.data(), (size_t) std::min(left, (off_t) buf.size()));

src/nix-build/nix-build.cc

@@ -350,7 +350,7 @@ static void main_nix_build(int argc, char * * argv)
                 takesNixShellAttr(vRoot) ? *autoArgsWithInNixShell : *autoArgs,
                 vRoot
             ).first);
-            state->forceValue(v, [&]() { return v.determinePos(noPos); });
+            state->forceValue(v, v.determinePos(noPos));
             getDerivations(
                 *state,
                 v,

src/nix-env/user-env.cc

@@ -133,7 +133,7 @@ bool createUserEnv(EvalState & state, DrvInfos & elems,
 
     /* Evaluate it. */
     debug("evaluating user environment builder");
-    state.forceValue(topLevel, [&]() { return topLevel.determinePos(noPos); });
+    state.forceValue(topLevel, topLevel.determinePos(noPos));
     NixStringContext context;
     Attr & aDrvPath(*topLevel.attrs->find(state.sDrvPath));
     auto topLevelDrv = state.coerceToStorePath(aDrvPath.pos, *aDrvPath.value, context, "");

src/nix-instantiate/nix-instantiate.cc

@@ -40,7 +40,7 @@ void processExpr(EvalState & state, const Strings & attrPaths,
 
     for (auto & i : attrPaths) {
         Value & v(*findAlongAttrPath(state, i, autoArgs, vRoot).first);
-        state.forceValue(v, [&]() { return v.determinePos(noPos); });
+        state.forceValue(v, v.determinePos(noPos));
 
         NixStringContext context;
         if (evalOnly) {