From 5d6bb8c350f367f83b4d3c5bb7595ef4ef62571a Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 28 Sep 2025 00:02:20 +0200 Subject: [PATCH] libexpr: remove tPrimOpApp using the same nodes as tApp is possible, and thanks to multi-arg app nodes it can even be a bit faster than the linked lists used to date. Change-Id: Idccb7c0b54c808e62da85d1c42ee09e6e92c4f7b --- lix/libexpr/eval-inline.hh | 6 +- lix/libexpr/eval.cc | 64 ++++++++---------- lix/libexpr/print.cc | 2 +- lix/libexpr/value.cc | 19 +----- lix/libexpr/value.hh | 107 ++++++++++++++---------------- tests/unit/libexpr/value/print.cc | 7 +- 6 files changed, 92 insertions(+), 113 deletions(-) diff --git a/lix/libexpr/eval-inline.hh b/lix/libexpr/eval-inline.hh index 005de77b8..a5ae1f761 100644 --- a/lix/libexpr/eval-inline.hh +++ b/lix/libexpr/eval-inline.hh @@ -107,7 +107,11 @@ void EvalState::forceValue(Value & v, const PosIdx pos) throw; } } else if (v.isApp()) { - callFunction(*v.app().left(), v.app().args(), v, pos); + auto & app = v.app(); + auto target = app.target(); + if (!target->isPrimOp() || target->primOp()->arity <= app.totalArgs()) { + callFunction(*v.app().left(), v.app().args(), v, pos); + } } } diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index 703e8742b..139b42c21 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -72,15 +72,6 @@ std::string printValue(EvalState & state, Value & v) return out.str(); } -const Value * getPrimOp(const Value &v) { - const Value * primOp = &v; - while (primOp->isPrimOpApp()) { - primOp = primOp->primOpApp().left; - } - assert(primOp->isPrimOp()); - return primOp; -} - std::string_view showType(ValueType type, bool withArticle) { #define WA(a, w) withArticle ? a " " w : w @@ -110,13 +101,18 @@ std::string showType(const Value & v) case tString: return v.string().context ? "a string with context" : "a string"; case tPrimOp: return fmt("the built-in function '%s'", std::string(v.primOp()->name)); - 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 v.isBlackhole() ? "a black hole" : "a thunk"; - case tApp: return "a function application"; - default: - return std::string(showType(v.type())); + case tApp: + if (v.isPrimOpApp()) { + return fmt( + "the partially applied built-in function '%s'", v.app().target()->primOp()->name + ); + } else { + return "a function application"; + } + default: + return std::string(showType(v.type())); } #pragma GCC diagnostic pop } @@ -581,10 +577,14 @@ Value * EvalBuiltins::addPrimOp(PrimOp && primOp) vPrimOp->mkPrimOp(new PrimOp(primOp)); Value v; v.mkApp(vPrimOp, vPrimOp); - return addConstant(primOp.name, v, { - .type = nThunk, // FIXME - .doc = primOp.doc, - }); + return addConstant( + primOp.name, + v, + { + .type = nFunction, + .doc = primOp.doc, + } + ); } auto envName = symbols.create(primOp.name); @@ -1571,14 +1571,10 @@ void EvalState::callFunction(Value & fun, std::span args, Value & vRes, Value vCur(fun); - auto makeAppChain = [&]() - { - vRes = vCur; - for (auto arg : args) { - auto fun2 = ctx.mem.allocValue(); - *fun2 = vRes; - vRes.mkPrimOpApp(fun2, arg); - } + auto makeAppChain = [&]() { + auto fun2 = ctx.mem.allocValue(); + *fun2 = vCur; + vRes = {NewValueAs::app, ctx.mem, *fun2, args}; }; const Attr * functor; @@ -1648,13 +1644,8 @@ void EvalState::callFunction(Value & fun, std::span args, Value & vRes, else if (vCur.isPrimOpApp()) { /* Figure out the number of arguments still needed. */ - size_t argsDone = 0; - Value * primOp = &vCur; - while (primOp->isPrimOpApp()) { - argsDone++; - primOp = primOp->primOpApp().left; - } - assert(primOp->isPrimOp()); + size_t argsDone = vCur.app().totalArgs(); + Value * primOp = vCur.app().target(); auto arity = primOp->primOp()->arity; auto argsLeft = arity - argsDone; @@ -1669,8 +1660,11 @@ void EvalState::callFunction(Value & fun, std::span args, Value & vRes, // max arity as of writing is 3. even 4 seems excessive though. SmallVector vArgs(arity); auto n = argsDone; - for (Value * arg = &vCur; arg->isPrimOpApp(); arg = arg->primOpApp().left) - vArgs[--n] = arg->primOpApp().right; + for (Value * arg = &vCur; arg->isApp(); arg = arg->app().left()) { + auto curArgs = arg->app().args(); + memcpy(&vArgs[n] - curArgs.size(), curArgs.data(), curArgs.size_bytes()); + n -= curArgs.size(); + } for (size_t i = 0; i < argsLeft; ++i) vArgs[argsDone + i] = args[i]; diff --git a/lix/libexpr/print.cc b/lix/libexpr/print.cc index e117f8641..6f59b7894 100644 --- a/lix/libexpr/print.cc +++ b/lix/libexpr/print.cc @@ -427,7 +427,7 @@ private: output << "primop"; } else if (v.isPrimOpApp()) { output << "partially applied "; - auto primOp = v.primOpAppPrimOp(); + auto primOp = v.app().target()->primOp(); if (primOp) output << *primOp; else diff --git a/lix/libexpr/value.cc b/lix/libexpr/value.cc index e23c32d9d..91c3e3ba0 100644 --- a/lix/libexpr/value.cc +++ b/lix/libexpr/value.cc @@ -38,26 +38,11 @@ void Value::print(EvalState & state, std::ostream & str, PrintOptions options) bool Value::isTrivial() const { - return - internalType != tApp - && internalType != tPrimOpApp + return internalType != tApp && (internalType != tThunk || (thunk().expr->try_cast() && static_cast(thunk().expr)->dynamicAttrs.empty()) - || thunk().expr->try_cast() - || thunk().expr->try_cast()); -} - -PrimOp * Value::primOpAppPrimOp() const -{ - Value * left = primOpApp().left; - while (left && !left->isPrimOp()) { - left = left->primOpApp().left; - } - - if (!left) - return nullptr; - return left->primOp(); + || thunk().expr->try_cast() || thunk().expr->try_cast()); } void Value::mkPrimOp(PrimOp * p) diff --git a/lix/libexpr/value.hh b/lix/libexpr/value.hh index 54fa9de64..8146b13d3 100644 --- a/lix/libexpr/value.hh +++ b/lix/libexpr/value.hh @@ -33,7 +33,6 @@ typedef enum { tApp, tLambda, tPrimOp, - tPrimOpApp, tExternal, tFloat } InternalType; @@ -190,9 +189,6 @@ struct NewValueAs struct primop_t { }; constexpr static primop_t primop{}; - struct primOpApp_t { }; - constexpr static primOpApp_t primOpApp{}; - struct lambda_t { }; constexpr static lambda_t lambda{}; @@ -231,7 +227,6 @@ public: USING_VALUETYPE(primop_t); USING_VALUETYPE(app_t); USING_VALUETYPE(null_t); - USING_VALUETYPE(primOpApp_t); USING_VALUETYPE(lambda_t); USING_VALUETYPE(external_t); USING_VALUETYPE(blackhole_t); @@ -432,14 +427,7 @@ public: Value(primop_t, PrimOp & primop); /// Constructs a nix language value of type "lambda", which represents a - /// partially applied primop. - Value(primOpApp_t, Value & lhs, Value & rhs) - : internalType(tPrimOpApp) - , _primOpApp({ .left = &lhs, .right = &rhs }) - { } - - /// Constructs a nix language value of type "lambda", which represents a - /// lazy partial application of another lambda. + /// lazy and/or partial application of a function. Value(app_t, Value & lhs, Value & rhs) : internalType(tApp) , _app{._left = reinterpret_cast(&lhs), ._right = &rhs} @@ -447,7 +435,7 @@ public: } /// Constructs a nix language value of type "lambda", which represents a - /// lazy partial application of another lambda. + /// lazy and/or partial application of a function. Value(app_t, EvalMemory & mem, Value & lhs, std::span args); /// Constructs a nix language value of type "external", which is only used @@ -520,7 +508,10 @@ public: // type() == nFunction inline bool isLambda() const { return internalType == tLambda; }; inline bool isPrimOp() const { return internalType == tPrimOp; }; - inline bool isPrimOpApp() const { return internalType == tPrimOpApp; }; + inline bool isPrimOpApp() const + { + return internalType == tApp && app().target()->isPrimOp(); + } struct List { @@ -580,6 +571,37 @@ public: } }; + struct App + { + uintptr_t _left; + union + { + Value * _right; + AppN * _appn; + }; + + Value * left() const + { + return reinterpret_cast(_left & ~uintptr_t(1)); + } + + Value * target() const + { + return left()->isApp() ? left()->app().target() : left(); + } + + std::span args() + { + return _left & 1 ? _appn->argsSpan() : std::span{&_right, 1}; + } + + size_t totalArgs() const + { + return (_left & 1 ? _appn->nargs : 1) + + (left()->isApp() ? left()->app().totalArgs() : 0); + } + }; + union { /// Dummy field, which takes up as much space as the largest union variants @@ -606,24 +628,7 @@ public: Env * env; Expr * expr; } _thunk; - struct { - uintptr_t _left; - union - { - Value * _right; - AppN * _appn; - }; - - Value * left() const - { - return reinterpret_cast(_left & ~uintptr_t(1)); - } - - std::span args() - { - return _left & 1 ? _appn->argsSpan() : std::span{&_right, 1}; - } - } _app; + App _app; struct { Env * env; @@ -633,9 +638,6 @@ public: PrimOp * _primOp; uintptr_t _primop_pad; }; - struct { - Value * left, * right; - } _primOpApp; struct { ExternalValueBase * _external; uintptr_t _external_pad; @@ -664,10 +666,15 @@ public: case tAttrs: return nAttrs; case tList: return nList; - case tLambda: case tPrimOp: case tPrimOpApp: return nFunction; + case tLambda: + case tPrimOp: + return nFunction; case tExternal: return nExternal; case tFloat: return nFloat; - case tThunk: case tApp: return nThunk; + case tThunk: + return nThunk; + case tApp: + return app().target()->isPrimOp() ? nFunction : nThunk; } if (invalidIsThunk) return nThunk; @@ -768,18 +775,6 @@ public: void mkPrimOp(PrimOp * p); - inline void mkPrimOpApp(Value * l, Value * r) - { - internalType = tPrimOpApp; - _primOpApp.left = l; - _primOpApp.right = r; - } - - /** - * For a `tPrimOpApp` value, get the original `PrimOp` value. - */ - PrimOp * primOpAppPrimOp() const; - inline void mkExternal(ExternalValueBase * e) { clearValue(); @@ -881,7 +876,12 @@ public: return _thunk; } - auto & app() + App & app() + { + return _app; + } + + const App & app() const { return _app; } @@ -896,11 +896,6 @@ public: return _primOp; } - const auto & primOpApp() const - { - return _primOpApp; - } - ExternalValueBase * external() const { return _external; diff --git a/tests/unit/libexpr/value/print.cc b/tests/unit/libexpr/value/print.cc index 381d3a97d..4965f473d 100644 --- a/tests/unit/libexpr/value/print.cc +++ b/tests/unit/libexpr/value/print.cc @@ -102,7 +102,8 @@ TEST_F(ValuePrintingTests, vThunk) TEST_F(ValuePrintingTests, vApp) { Value vApp; - vApp.mkApp(&vApp, &vApp); + Value vFn{NewValueAs::null}; + vApp.mkApp(&vFn, &vFn); test(vApp, "«thunk»"); } @@ -149,7 +150,7 @@ TEST_F(ValuePrintingTests, vPrimOpApp) vPrimOp.mkPrimOp(&primOp); Value vPrimOpApp; - vPrimOpApp.mkPrimOpApp(&vPrimOp, nullptr); + vPrimOpApp.mkApp(&vPrimOp, &vPrimOp); test(vPrimOpApp, "«partially applied primop puppy»"); } @@ -603,7 +604,7 @@ TEST_F(ValuePrintingTests, ansiColorsPrimOpApp) vPrimOp.mkPrimOp(&primOp); Value v; - v.mkPrimOpApp(&vPrimOp, nullptr); + v.mkApp(&vPrimOp, &vPrimOp); test(v, ANSI_BLUE "«partially applied primop puppy»" ANSI_NORMAL,