From 78353deb028fcc700776db9d92dcae45d68fb85f Mon Sep 17 00:00:00 2001 From: pennae Date: Sun, 10 Dec 2023 08:24:45 +0100 Subject: [PATCH] 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;