From dd180911d8ecf737e6b2ceb89d6797965fcc3b78 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Mon, 4 Mar 2024 07:32:31 +0100 Subject: [PATCH] Merge pull request #9582 from pennae/misc-opts a packet of small optimizations (cherry picked from commit ee439734e924eb337a869ff2e48aff8b989198bc) Change-Id: I125d870710750a32a0dece48f39a3e9132b0d023 --- src/libcmd/built-path.cc | 4 +- src/libcmd/installable-flake.cc | 2 +- src/libcmd/repl.cc | 4 +- src/libexpr/eval-inline.hh | 14 +----- src/libexpr/eval.cc | 65 +++++++++++++++++--------- src/libexpr/eval.hh | 3 +- src/libexpr/get-drvs.cc | 4 +- src/libexpr/lexer.l | 10 ++-- src/libexpr/nixexpr.cc | 2 + src/libexpr/nixexpr.hh | 17 +++++++ src/libexpr/parser.y | 25 ++++++++++ src/libexpr/value.hh | 26 +++++++---- src/libstore/derived-path.cc | 8 +--- src/libstore/local-store.cc | 22 +++++++-- src/libutil/comparator.hh | 4 +- src/nix-build/nix-build.cc | 2 +- src/nix-env/user-env.cc | 2 +- src/nix-instantiate/nix-instantiate.cc | 2 +- 18 files changed, 144 insertions(+), 72 deletions(-) diff --git a/src/libcmd/built-path.cc b/src/libcmd/built-path.cc index 9a2dce806..6ff99070d 100644 --- a/src/libcmd/built-path.cc +++ b/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(*me->drvPath, me->FIELD); \ + auto fields1 = std::tie(*me->drvPath, me->FIELD); \ me = &other; \ - auto fields2 = std::make_tuple(*me->drvPath, me->FIELD); \ + auto fields2 = std::tie(*me->drvPath, me->FIELD); \ return fields1 COMPARATOR fields2; \ } #define CMP(CHILD_TYPE, MY_TYPE, FIELD) \ diff --git a/src/libcmd/installable-flake.cc b/src/libcmd/installable-flake.cc index 4074da06d..bb75b8ff0 100644 --- a/src/libcmd/installable-flake.cc +++ b/src/libcmd/installable-flake.cc @@ -47,7 +47,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; } diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 9bd773f68..e6f0faa06 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -885,7 +885,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)); } @@ -904,7 +904,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()) { diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index c37b1d62b..52aa75b5f 100644 --- a/src/libexpr/eval-inline.hh +++ b/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 -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(); } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 1817ebf55..98fc01368 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -158,7 +158,17 @@ void Value::print(const SymbolTable &symbols, std::ostream &str, break; case tThunk: case tApp: - str << ""; + if (!isBlackhole()) { + str << ""; + } 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 << ""; @@ -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(); +} + +// 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 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) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 4425efbca..20ea7b704 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -467,8 +467,7 @@ public: */ inline void forceValue(Value & v, const PosIdx pos); - template - inline void forceValue(Value & v, Callable getPos); + void tryFixupBlackHolePos(Value & v, PosIdx pos); /** * Force a value, then recursively force list elements and diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 506a63677..e4ee4409c 100644 --- a/src/libexpr/get-drvs.cc +++ b/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 = diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index a3a8608d9..df2cbd06f 100644 --- a/src/libexpr/lexer.l +++ b/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_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; } diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 4566a1388..76ce62f6c 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -9,6 +9,8 @@ namespace nix { +ExprBlackHole eBlackHole; + struct PosAdapter : AbstractPos { Pos::Origin origin; diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 120de7d97..cb952f5bb 100644 --- a/src/libexpr/nixexpr.hh +++ b/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 & 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 diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index dd2b20e25..c54c1d11a 100644 --- a/src/libexpr/parser.y +++ b/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; diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index ec7d82a64..224ffac9c 100644 --- a/src/libexpr/value.hh +++ b/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> ValueVector; typedef std::map, traceable_allocator>> ValueMap; diff --git a/src/libstore/derived-path.cc b/src/libstore/derived-path.cc index 47d784deb..214caab54 100644 --- a/src/libstore/derived-path.cc +++ b/src/libstore/derived-path.cc @@ -11,9 +11,9 @@ namespace nix { bool MY_TYPE ::operator COMPARATOR (const MY_TYPE & other) const \ { \ const MY_TYPE* me = this; \ - auto fields1 = std::make_tuple(*me->drvPath, me->FIELD); \ + auto fields1 = std::tie(*me->drvPath, me->FIELD); \ me = &other; \ - auto fields2 = std::make_tuple(*me->drvPath, me->FIELD); \ + auto fields2 = std::tie(*me->drvPath, me->FIELD); \ return fields1 COMPARATOR fields2; \ } #define CMP(CHILD_TYPE, MY_TYPE, FIELD) \ @@ -21,13 +21,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 diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 17b4ecc73..554e13e3d 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -15,6 +15,8 @@ #include #include +#include +#include #include #include #include @@ -1286,7 +1288,11 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name path. */ bool inMemory = false; - std::string dump; + struct Free { + void operator()(void* v) { free(v); } + }; + std::unique_ptr 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 @@ -1295,13 +1301,18 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name 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; @@ -1327,7 +1338,8 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name else writeFile(tempPath, bothSource); - dump.clear(); + dumpBuffer.reset(); + dump = {}; } auto [hash, size] = hashSink->finish(); diff --git a/src/libutil/comparator.hh b/src/libutil/comparator.hh index a4d20a675..cbc2bb4fd 100644 --- a/src/libutil/comparator.hh +++ b/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...) \ diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 54e8bd8d1..cf07e60a1 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -346,7 +346,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, diff --git a/src/nix-env/user-env.cc b/src/nix-env/user-env.cc index d12d70f33..d2b917095 100644 --- a/src/nix-env/user-env.cc +++ b/src/nix-env/user-env.cc @@ -129,7 +129,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, ""); diff --git a/src/nix-instantiate/nix-instantiate.cc b/src/nix-instantiate/nix-instantiate.cc index d40196497..e1686bdb5 100644 --- a/src/nix-instantiate/nix-instantiate.cc +++ b/src/nix-instantiate/nix-instantiate.cc @@ -41,7 +41,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) {