From 0218e4e6c386e4c432520506568420c3cc384e47 Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 10 Dec 2023 04:15:51 +0100 Subject: [PATCH 01/10] memset less in addToStoreFromDump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resizing a std::string clears the newly added bytes, which is not necessary here and comes with a ~1.4% slowdown on our test nixos config. 〉 nix eval --raw --impure --expr 'with import {}; system' before: Time (mean ± σ): 4.486 s ± 0.003 s [User: 3.978 s, System: 0.507 s] Range (min … max): 4.482 s … 4.492 s 10 runs after: Time (mean ± σ): 4.429 s ± 0.002 s [User: 3.929 s, System: 0.500 s] Range (min … max): 4.427 s … 4.433 s 10 runs --- src/libstore/local-store.cc | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 7e82bae28..d903bb061 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -18,6 +18,8 @@ #include #include +#include +#include #include #include #include @@ -1130,7 +1132,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 @@ -1139,13 +1145,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; @@ -1171,7 +1182,8 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name else writeFile(tempPath, bothSource); - dump.clear(); + dumpBuffer.reset(); + dump = {}; } auto [hash, size] = hashSink->finish(); From 78353deb028fcc700776db9d92dcae45d68fb85f Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 10 Dec 2023 08:24:45 +0100 Subject: [PATCH 02/10] encode black holes as tApp values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checking for isBlackhole in the forceValue hot path is rather more expensive than necessary, and with a little bit of trickery we can move such handling into the isApp case. small performance benefit, but under some circumstances we've seen 2% improvement as well. 〉 nix eval --raw --impure --expr 'with import {}; system' before: Time (mean ± σ): 4.429 s ± 0.002 s [User: 3.929 s, System: 0.500 s] Range (min … max): 4.427 s … 4.433 s 10 runs after: Time (mean ± σ): 4.396 s ± 0.002 s [User: 3.894 s, System: 0.501 s] Range (min … max): 4.393 s … 4.399 s 10 runs --- src/libexpr/eval-inline.hh | 13 +++++++---- src/libexpr/eval.cc | 44 +++++++++++++++++++++----------------- src/libexpr/eval.hh | 8 +++++++ src/libexpr/nixexpr.hh | 7 ++++++ src/libexpr/primops.cc | 23 ++++++++++++++++++++ src/libexpr/primops.hh | 6 ++++++ src/libexpr/value.hh | 24 ++++++++++++++------- 7 files changed, 93 insertions(+), 32 deletions(-) diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index c37b1d62b..9d08f1938 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -104,11 +104,16 @@ void EvalState::forceValue(Value & v, Callable getPos) } } else if (v.isApp()) { - PosIdx pos = getPos(); - callFunction(*v.app.left, *v.app.right, v, pos); + try { + callFunction(*v.app.left, *v.app.right, v, noPos); + } catch (InfiniteRecursionError & e) { + // only one black hole can *throw* in any given eval stack so we need not + // check whether the position is set already. + if (v.isBlackhole()) + e.err.errPos = positions[getPos()]; + throw; + } } - 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 9e494148e..71c151f96 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -162,7 +162,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 << ""; @@ -179,15 +189,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,8 +257,7 @@ std::string showType(const Value & v) 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 tApp: return "a function application"; - case tBlackhole: return "a black hole"; + case tApp: return v.isBlackhole() ? "a black hole" : "a function application"; default: return std::string(showType(v.type())); } @@ -1621,15 +1621,17 @@ 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]++; + // This will count black holes, but that's ok, because unrecoverable errors are rare. + 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); + if (!fn->hideInDiagnostics) + addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); throw; } @@ -1666,18 +1668,20 @@ 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]++; + // This will count black holes, but that's ok, because unrecoverable errors are rare. + 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); + if (!fn->hideInDiagnostics) + addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); throw; } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index f3f6d35b9..e5e401ab6 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -77,6 +77,14 @@ struct PrimOp */ std::optional experimentalFeature; + /** + * Whether to hide this primop in diagnostics. + * + * Used to hide the fact that black holes are primop applications from + * stack traces. + */ + bool hideInDiagnostics; + /** * Validity check to be performed by functions that introduce primops, * such as RegisterPrimOp() and Value::mkPrimOp(). diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 020286815..cf6fd1a8d 100644 --- a/src/libexpr/nixexpr.hh +++ b/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. */ diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 89d5492da..d46eccd34 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -4263,6 +4263,29 @@ static RegisterPrimOp primop_splitVersion({ }); +static void prim_blackHoleFn(EvalState & state, const PosIdx pos, Value * * args, Value & v) +{ + state.error("infinite recursion encountered") + .debugThrow(); +} + +static PrimOp primop_blackHole = { + .name = "«blackHole»", + .args = {}, + .fun = prim_blackHoleFn, + .hideInDiagnostics = true, +}; + +static Value makeBlackHole() +{ + Value v; + v.mkPrimOp(&primop_blackHole); + return v; +} + +Value prim_blackHole = makeBlackHole(); + + /************************************************************* * Primop registration *************************************************************/ diff --git a/src/libexpr/primops.hh b/src/libexpr/primops.hh index 45486608f..244eada86 100644 --- a/src/libexpr/primops.hh +++ b/src/libexpr/primops.hh @@ -51,4 +51,10 @@ void prim_importNative(EvalState & state, const PosIdx pos, Value * * args, Valu */ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v); +/** + * Placeholder value for black holes, used to represent black holes as + * applications of this value to the evaluated thunks. + */ +extern Value prim_blackHole; + } diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 30b3d4934..52cd0f901 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -32,7 +32,6 @@ typedef enum { tThunk, tApp, tLambda, - tBlackhole, tPrimOp, tPrimOpApp, tExternal, @@ -151,7 +150,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 +247,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 +355,7 @@ public: lambda.fun = f; } - inline void mkBlackhole() - { - internalType = tBlackhole; - // Value will be overridden anyways - } + inline void mkBlackhole(); void mkPrimOp(PrimOp * p); @@ -447,6 +442,19 @@ public: }; +extern Value prim_blackHole; + +inline bool Value::isBlackhole() const +{ + return internalType == tApp && app.left == &prim_blackHole; +} + +inline void Value::mkBlackhole() +{ + mkApp(&prim_blackHole, &prim_blackHole); +} + + #if HAVE_BOEHMGC typedef std::vector> ValueVector; typedef std::map, traceable_allocator>> ValueMap; From 74c134914c747b1df6385cab5d2298f66a87b61f Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 10 Dec 2023 09:25:20 +0100 Subject: [PATCH 03/10] compare string values with strcmp string_view()ification calls strlen() first, which we don't need here. --- src/libexpr/eval.cc | 2 +- src/libexpr/primops.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 71c151f96..8e89ddcf1 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2436,7 +2436,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 diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index d46eccd34..b7e903667 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -586,7 +586,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 @@ -2401,7 +2401,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({ From cc4038d54177c944340607c7d141680e66ff92a7 Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 10 Dec 2023 09:49:38 +0100 Subject: [PATCH 04/10] use std::tie() for macro-generated operators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit as written the comparisons generate copies, even though it looks as though they shouldn't. before: Time (mean ± σ): 4.396 s ± 0.002 s [User: 3.894 s, System: 0.501 s] Range (min … max): 4.393 s … 4.399 s 10 runs after: Time (mean ± σ): 4.260 s ± 0.003 s [User: 3.754 s, System: 0.505 s] Range (min … max): 4.257 s … 4.266 s 10 runs --- src/libcmd/built-path.cc | 4 ++-- src/libstore/derived-path.cc | 8 ++------ src/libutil/comparator.hh | 4 ++-- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/libcmd/built-path.cc b/src/libcmd/built-path.cc index 8e2efc7c3..c5eb93c5d 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/libstore/derived-path.cc b/src/libstore/derived-path.cc index 3105dbc93..a7b404321 100644 --- a/src/libstore/derived-path.cc +++ b/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(*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) \ @@ -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 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...) \ From 2e0321912a9efa352160eb1e57e6b7b88e517d0d Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 10 Dec 2023 12:59:51 +0100 Subject: [PATCH 05/10] use aligned flex tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ~2% speedup on parsing without eval, less (but still significant) on system eval. having flex generate faster parsers leads to very strange misparses. maybe re2c is worth investigating. before: Time (mean ± σ): 4.260 s ± 0.003 s [User: 3.754 s, System: 0.505 s] Range (min … max): 4.257 s … 4.266 s 10 runs after: Time (mean ± σ): 4.231 s ± 0.004 s [User: 3.725 s, System: 0.504 s] Range (min … max): 4.226 s … 4.240 s 10 runs --- src/libexpr/lexer.l | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index a3a8608d9..9a35dd594 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 From b78e77b34c14b0f127b22e252309527e84967dcc Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 10 Dec 2023 13:00:18 +0100 Subject: [PATCH 06/10] use custom location type in the parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ~1% parser speedup from not using TLS indirections, less on system eval. this could have also gone in flex yyextra data, but that's significantly slower for some reason (albeit still faster than thread locals). before: Time (mean ± σ): 4.231 s ± 0.004 s [User: 3.725 s, System: 0.504 s] Range (min … max): 4.226 s … 4.240 s 10 runs after: Time (mean ± σ): 4.224 s ± 0.005 s [User: 3.711 s, System: 0.512 s] Range (min … max): 4.218 s … 4.234 s 10 runs --- src/libexpr/lexer.l | 9 +++------ src/libexpr/parser.y | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index 9a35dd594..df2cbd06f 100644 --- a/src/libexpr/lexer.l +++ b/src/libexpr/lexer.l @@ -36,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; @@ -47,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; @@ -231,7 +228,7 @@ or { return OR_KW; } {HPATH_START}\$\{ { PUSH_STATE(PATH_START); yyless(0); - *yylloc = prev_yylloc; + yylloc->unstash(); } {PATH_SEG} { @@ -287,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/parser.y b/src/libexpr/parser.y index 16ad8af2e..b331776f0 100644 --- a/src/libexpr/parser.y +++ b/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; From f9aee2f2c41652b3b76d16a874fdded4e6d28d92 Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 10 Dec 2023 10:34:55 +0100 Subject: [PATCH 07/10] don't malloc/memset posix accessor buffer it's relatively small and fits on the stack nicely, and we don't need it initialized either. --- src/libutil/posix-source-accessor.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc index 15ff76e59..5f26fa67b 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -25,7 +25,7 @@ void PosixSourceAccessor::readFile( off_t left = st.st_size; - std::vector buf(64 * 1024); + std::array buf; while (left) { checkInterrupt(); ssize_t rd = read(fd.get(), buf.data(), (size_t) std::min(left, (off_t) buf.size())); From 69ed4aee612e247f2d6ebbb44aba743c4282e00e Mon Sep 17 00:00:00 2001 From: pennae Date: Mon, 11 Dec 2023 15:48:24 +0100 Subject: [PATCH 08/10] remove lazy-pos forceValue almost all uses of this are interactive, except for deepSeq. deepSeq is going to be expensive and rare enough to not care much about, and Value::determinePos should usually be cheap enough to not be too much of a burden in any case. --- src/libcmd/installable-flake.cc | 2 +- src/libcmd/repl.cc | 4 ++-- src/libexpr/eval-inline.hh | 10 +--------- src/libexpr/eval.cc | 2 +- src/libexpr/eval.hh | 3 --- src/libexpr/get-drvs.cc | 4 ++-- src/nix-build/nix-build.cc | 2 +- src/nix-env/user-env.cc | 2 +- src/nix-instantiate/nix-instantiate.cc | 2 +- 9 files changed, 10 insertions(+), 21 deletions(-) diff --git a/src/libcmd/installable-flake.cc b/src/libcmd/installable-flake.cc index 2f428cb7e..ddec7537b 100644 --- a/src/libcmd/installable-flake.cc +++ b/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; } diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 0986296ad..97d709ff4 100644 --- a/src/libcmd/repl.cc +++ b/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()) { diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 9d08f1938..8a9ebb77a 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -81,15 +81,7 @@ 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; @@ -110,7 +102,7 @@ void EvalState::forceValue(Value & v, Callable getPos) // only one black hole can *throw* in any given eval stack so we need not // check whether the position is set already. if (v.isBlackhole()) - e.err.errPos = positions[getPos()]; + e.err.errPos = positions[pos]; throw; } } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 8e89ddcf1..4dc5af97a 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2044,7 +2044,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 e5e401ab6..4c7ea1d98 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -473,9 +473,6 @@ public: */ inline void forceValue(Value & v, const PosIdx pos); - template - inline void forceValue(Value & v, Callable getPos); - /** * Force a value, then recursively force list elements and * attributes. diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index d4e946d81..a6441871c 100644 --- a/src/libexpr/get-drvs.cc +++ b/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 = diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 01da028d8..4465e2f90 100644 --- a/src/nix-build/nix-build.cc +++ b/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, diff --git a/src/nix-env/user-env.cc b/src/nix-env/user-env.cc index 34f6bd005..fe5b89b3f 100644 --- a/src/nix-env/user-env.cc +++ b/src/nix-env/user-env.cc @@ -128,7 +128,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 86b9be17d..ab590b3a6 100644 --- a/src/nix-instantiate/nix-instantiate.cc +++ b/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) { From f9db4de0f3758e0f730a5d98348e7cc40082104a Mon Sep 17 00:00:00 2001 From: pennae Date: Mon, 11 Dec 2023 15:54:16 +0100 Subject: [PATCH 09/10] force-inline forceValue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit forceValue is extremely hot. interestingly adding likeliness annotations to the branches does not seem to make a difference. before: Time (mean ± σ): 4.224 s ± 0.005 s [User: 3.711 s, System: 0.512 s] Range (min … max): 4.218 s … 4.234 s 10 runs after: Time (mean ± σ): 4.140 s ± 0.009 s [User: 3.647 s, System: 0.492 s] Range (min … max): 4.130 s … 4.152 s 10 runs --- src/libexpr/eval-inline.hh | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 8a9ebb77a..d48871628 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -81,6 +81,7 @@ Env & EvalState::allocEnv(size_t size) } +[[gnu::always_inline]] void EvalState::forceValue(Value & v, const PosIdx pos) { if (v.isThunk()) { From 2b0e95e7aabd075f95cbfb1607330b2284b01918 Mon Sep 17 00:00:00 2001 From: pennae Date: Mon, 11 Dec 2023 16:23:08 +0100 Subject: [PATCH 10/10] use singleton expr to generate black hole errors this also reduces forceValue code size and removes the need for hideInDiagnostics. coopting thunk forcing like this has the additional benefit of clarifying how these errors can happen in the first place. --- src/libexpr/eval-inline.hh | 14 +++----------- src/libexpr/eval.cc | 35 +++++++++++++++++++++++++++-------- src/libexpr/eval.hh | 10 ++-------- src/libexpr/nixexpr.cc | 2 ++ src/libexpr/nixexpr.hh | 10 ++++++++++ src/libexpr/primops.cc | 23 ----------------------- src/libexpr/primops.hh | 6 ------ src/libexpr/value.hh | 12 +++++++----- 8 files changed, 51 insertions(+), 61 deletions(-) diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index d48871628..52aa75b5f 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -93,20 +93,12 @@ void EvalState::forceValue(Value & v, const PosIdx pos) expr->eval(*this, *env, v); } catch (...) { v.mkThunk(env, expr); + tryFixupBlackHolePos(v, pos); throw; } } - else if (v.isApp()) { - try { - callFunction(*v.app.left, *v.app.right, v, noPos); - } catch (InfiniteRecursionError & e) { - // only one black hole can *throw* in any given eval stack so we need not - // check whether the position is set already. - if (v.isBlackhole()) - e.err.errPos = positions[pos]; - throw; - } - } + else if (v.isApp()) + callFunction(*v.app.left, *v.app.right, v, pos); } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 4dc5af97a..0c35b3713 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -256,8 +256,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 tApp: return v.isBlackhole() ? "a black hole" : "a function application"; + case tThunk: return v.isBlackhole() ? "a black hole" : "a thunk"; + case tApp: return "a function application"; default: return std::string(showType(v.type())); } @@ -1624,14 +1624,12 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & auto * fn = vCur.primOp; nrPrimOpCalls++; - // This will count black holes, but that's ok, because unrecoverable errors are rare. if (countCalls) primOpCalls[fn->name]++; try { fn->fun(*this, vCur.determinePos(noPos), args, vCur); } catch (Error & e) { - if (!fn->hideInDiagnostics) - addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); + addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); throw; } @@ -1670,7 +1668,6 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & auto fn = primOp->primOp; nrPrimOpCalls++; - // This will count black holes, but that's ok, because unrecoverable errors are rare. if (countCalls) primOpCalls[fn->name]++; try { @@ -1680,8 +1677,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & // so the debugger allows to inspect the wrong parameters passed to the builtin. fn->fun(*this, vCur.determinePos(noPos), vArgs, vCur); } catch (Error & e) { - if (!fn->hideInDiagnostics) - addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); + addErrorTrace(e, pos, "while calling the '%1%' builtin", fn->name); throw; } @@ -2035,6 +2031,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; diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 4c7ea1d98..56bc5e48f 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -77,14 +77,6 @@ struct PrimOp */ std::optional experimentalFeature; - /** - * Whether to hide this primop in diagnostics. - * - * Used to hide the fact that black holes are primop applications from - * stack traces. - */ - bool hideInDiagnostics; - /** * Validity check to be performed by functions that introduce primops, * such as RegisterPrimOp() and Value::mkPrimOp(). @@ -473,6 +465,8 @@ public: */ inline void forceValue(Value & v, const PosIdx pos); + void tryFixupBlackHolePos(Value & v, PosIdx pos); + /** * Force a value, then recursively force list elements and * attributes. diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 22be8e68c..84860b30f 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 cf6fd1a8d..1e57fec7a 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -462,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 & 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/primops.cc b/src/libexpr/primops.cc index b7e903667..2a71747a0 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -4263,29 +4263,6 @@ static RegisterPrimOp primop_splitVersion({ }); -static void prim_blackHoleFn(EvalState & state, const PosIdx pos, Value * * args, Value & v) -{ - state.error("infinite recursion encountered") - .debugThrow(); -} - -static PrimOp primop_blackHole = { - .name = "«blackHole»", - .args = {}, - .fun = prim_blackHoleFn, - .hideInDiagnostics = true, -}; - -static Value makeBlackHole() -{ - Value v; - v.mkPrimOp(&primop_blackHole); - return v; -} - -Value prim_blackHole = makeBlackHole(); - - /************************************************************* * Primop registration *************************************************************/ diff --git a/src/libexpr/primops.hh b/src/libexpr/primops.hh index 244eada86..45486608f 100644 --- a/src/libexpr/primops.hh +++ b/src/libexpr/primops.hh @@ -51,10 +51,4 @@ void prim_importNative(EvalState & state, const PosIdx pos, Value * * args, Valu */ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v); -/** - * Placeholder value for black holes, used to represent black holes as - * applications of this value to the evaluated thunks. - */ -extern Value prim_blackHole; - } diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 52cd0f901..d9860e921 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -61,6 +61,7 @@ class Bindings; struct Env; struct Expr; struct ExprLambda; +struct ExprBlackHole; struct PrimOp; class Symbol; class PosIdx; @@ -442,16 +443,17 @@ public: }; -extern Value prim_blackHole; +extern ExprBlackHole eBlackHole; -inline bool Value::isBlackhole() const +bool Value::isBlackhole() const { - return internalType == tApp && app.left == &prim_blackHole; + return internalType == tThunk && thunk.expr == (Expr*) &eBlackHole; } -inline void Value::mkBlackhole() +void Value::mkBlackhole() { - mkApp(&prim_blackHole, &prim_blackHole); + internalType = tThunk; + thunk.expr = (Expr*) &eBlackHole; }