From 1b5f4eb5eb38431ad785d5d15879748654aaf163 Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 28 Sep 2025 00:02:20 +0200 Subject: [PATCH] libexpr: move primops to auxiliary storage same as for null: we have few of them, they're statically allocated, and they're not the largest contributor to the value population. not storing them in Value itself frees up resources we *will* use later. Change-Id: I521f9f243f48f56a78f7bffdf1dc1f0bc40a5c2d --- lix/libexpr/eval.cc | 67 ++++++++++++++-------------- lix/libexpr/eval.hh | 49 +------------------- lix/libexpr/primops.cc | 46 +++++++++---------- lix/libexpr/primops.hh | 2 +- lix/libexpr/value.cc | 9 +--- lix/libexpr/value.hh | 74 ++++++++++++++++++++++++++----- tests/unit/libexpr/value/print.cc | 16 ++----- 7 files changed, 129 insertions(+), 134 deletions(-) diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index f129713c8..56e7a4aec 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -110,36 +110,38 @@ std::string_view showType(ValueType type, bool withArticle) std::string showType(const Value & v) { // Allow selecting a subset of enum values - #pragma GCC diagnostic push - #pragma GCC diagnostic ignored "-Wswitch-enum" +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wswitch-enum" switch (v.internalType) { - 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 tAuxiliary: + case tString: + return v.string().context ? "a string with context" : "a string"; + case tAuxiliary: #pragma GCC diagnostic push #pragma GCC diagnostic error "-Wswitch-enum" - switch (v.auxiliary()->type) { - case Value::Acb::tExternal: - return v.external()->showType(); - case Value::Acb::tFloat: - case Value::Acb::tNull: - return std::string(showType(v.type())); - } -#pragma GCC diagnostic pop - case tThunk: return v.isBlackhole() ? "a black hole" : "a thunk"; - 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: + switch (v.auxiliary()->type) { + case Value::Acb::tExternal: + return v.external()->showType(); + case Value::Acb::tFloat: + case Value::Acb::tNull: return std::string(showType(v.type())); + case Value::Acb::tPrimOp: + return fmt("the built-in function '%s'", v.primOp()->name); + } +#pragma GCC diagnostic pop + case tThunk: + return v.isBlackhole() ? "a black hole" : "a thunk"; + 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 +#pragma GCC diagnostic pop } @@ -588,29 +590,28 @@ void EvalBuiltins::addConstant(const std::string & name, Value * v, Constant inf } } - -std::ostream & operator<<(std::ostream & output, PrimOp & primOp) +std::ostream & operator<<(std::ostream & output, const PrimOp & primOp) { output << "primop " << primOp.name; return output; } -Value * EvalBuiltins::addPrimOp(PrimOp && primOp) +Value * EvalBuiltins::addPrimOp(PrimOpDetails && primOp) { /* Hack to make constants lazy: turn them into a application of the primop to a dummy value. */ if (primOp.arity == 0) { primOp.arity = 1; auto vPrimOp = mem.allocValue(); - vPrimOp->mkPrimOp(new PrimOp(primOp)); + vPrimOp->mkPrimOp(new PrimOp(std::move(primOp))); Value v; v.mkApp(vPrimOp, vPrimOp); return addConstant( - primOp.name, + vPrimOp->primOp()->name, v, { .type = nFunction, - .doc = primOp.doc, + .doc = vPrimOp->primOp()->doc, } ); } @@ -620,10 +621,10 @@ Value * EvalBuiltins::addPrimOp(PrimOp && primOp) primOp.name = primOp.name.substr(2); Value * v = mem.allocValue(); - v->mkPrimOp(new PrimOp(primOp)); + v->mkPrimOp(new PrimOp(std::move(primOp))); staticEnv->vars.insert_or_assign(auto(envName), baseEnvDispl); env.values[baseEnvDispl++] = v; - env.values[0]->attrs()->push_back(Attr(symbols.create(primOp.name), v)); + env.values[0]->attrs()->push_back(Attr(symbols.create(v->primOp()->name), v)); return v; } diff --git a/lix/libexpr/eval.hh b/lix/libexpr/eval.hh index 25e1c133e..505ef0386 100644 --- a/lix/libexpr/eval.hh +++ b/lix/libexpr/eval.hh @@ -36,52 +36,7 @@ namespace eval_cache { class EvalCache; } -/** - * Function that implements a primop. - */ -using PrimOpImpl = void(EvalState & state, Value ** args, Value & v); - -/** - * Info about a primitive operation, and its implementation - */ -struct PrimOp -{ - /** - * Name of the primop. `__` prefix is treated specially. - */ - std::string name; - - /** - * Names of the parameters of a primop, for primops that take a - * fixed number of arguments to be substituted for these parameters. - */ - std::vector args; - - /** - * Aritiy of the primop. - * - * If `args` is not empty, this field will be computed from that - * field instead, so it doesn't need to be manually set. - */ - size_t arity = 0; - - /** - * Optional free-form documentation about the primop. - */ - const char * doc = nullptr; - - /** - * Implementation of the primop. - */ - std::function fun; - - /** - * Optional experimental for this to be gated on. - */ - std::optional experimentalFeature; -}; - -std::ostream & operator<<(std::ostream & output, PrimOp & primOp); +std::ostream & operator<<(std::ostream & output, const PrimOp & primOp); /** * Info about a constant @@ -314,7 +269,7 @@ private: void addConstant(const std::string & name, Value * v, Constant info); - Value * addPrimOp(PrimOp && primOp); + Value * addPrimOp(PrimOpDetails && primOp); Value prepareNixPath(const SearchPath & searchPath); diff --git a/lix/libexpr/primops.cc b/lix/libexpr/primops.cc index eb85ada76..0a29b5961 100644 --- a/lix/libexpr/primops.cc +++ b/lix/libexpr/primops.cc @@ -252,12 +252,10 @@ static void import(EvalState & state, Value & vPath, Value * vScope, Value & v) } } -static RegisterPrimOp primop_scopedImport(PrimOp { - .name = "scopedImport", - .arity = 2, - .fun = [](EvalState & state, Value * * args, Value & v) - { - import(state, *args[1], args[0], v); +static RegisterPrimOp primop_scopedImport(PrimOp{ + {.name = "scopedImport", + .arity = 2, + .fun = [](EvalState & state, Value ** args, Value & v) { import(state, *args[1], args[0], v); } } }); @@ -624,11 +622,11 @@ static void prim_addErrorContext(EvalState & state, Value * * args, Value & v) } } -static RegisterPrimOp primop_addErrorContext(PrimOp { +static RegisterPrimOp primop_addErrorContext(PrimOp{{ .name = "__addErrorContext", .arity = 2, .fun = prim_addErrorContext, -}); +}}); static void prim_ceil(EvalState & state, Value * * args, Value & v) { @@ -1135,11 +1133,11 @@ drvName, Bindings * attrs, Value & v) v.mkAttrs(result); } -static RegisterPrimOp primop_derivationStrict(PrimOp { +static RegisterPrimOp primop_derivationStrict(PrimOp{{ .name = "derivationStrict", .arity = 1, .fun = prim_derivationStrict, -}); +}}); /* Return a placeholder string for the specified output that will be substituted by the corresponding output path at build time. For @@ -1728,14 +1726,15 @@ static void prim_unsafeGetAttrPos(EvalState & state, Value * * args, Value & v) // as with black holes this cost is too high to justify another thunk type to check // for in the very hot path that is forceValue. static struct LazyPosAcessors { - PrimOp primop_lineOfPos{.arity = 1, .fun = [](EvalState & state, Value ** args, Value & v) { - v.mkInt(state.ctx.positions[PosIdx(args[0]->integer().value)].line); - }}; - PrimOp primop_columnOfPos{.arity = 1, .fun = [](EvalState & state, Value ** args, Value & v) { - v.mkInt( - state.ctx.positions[PosIdx(args[0]->integer().value)].column - ); - }}; + PrimOp primop_lineOfPos{{.arity = 1, .fun = [](EvalState & state, Value ** args, Value & v) { + v.mkInt(state.ctx.positions[PosIdx(args[0]->integer().value)].line + ); + }}}; + PrimOp primop_columnOfPos{{.arity = 1, .fun = [](EvalState & state, Value ** args, Value & v) { + v.mkInt( + state.ctx.positions[PosIdx(args[0]->integer().value)].column + ); + }}}; Value lineOfPos, columnOfPos; @@ -2827,11 +2826,10 @@ static void prim_splitVersion(EvalState & state, Value * * args, Value & v) RegisterPrimOp::PrimOps * RegisterPrimOp::primOps; - -RegisterPrimOp::RegisterPrimOp(PrimOp && primOp) +RegisterPrimOp::RegisterPrimOp(PrimOpDetails && primOp) { if (!primOps) primOps = new PrimOps; - primOps->push_back(std::move(primOp)); + primOps->emplace_back(std::move(primOp)); } @@ -2879,19 +2877,19 @@ void EvalBuiltins::createBaseEnv(const SearchPath & searchPath, const Path & sto addPrimOp(std::move(primOpAdjusted)); } - static PrimOp prim_initializeDerivation{ + static PrimOp prim_initializeDerivation{{ .arity = 1, .fun = [](EvalState & state, Value ** args, Value & v) { char code[] = - #include "primops/derivation.nix.gen.hh" +#include "primops/derivation.nix.gen.hh" ; auto & expr = *state.ctx.parse( code, sizeof(code), Pos::Hidden{}, {CanonPath::root}, state.ctx.builtins.staticEnv ); state.eval(expr, v); }, - }; + }}; static Value initializeDerivation{NewValueAs::primop, prim_initializeDerivation}; /* Add a wrapper around the derivation primop that computes the diff --git a/lix/libexpr/primops.hh b/lix/libexpr/primops.hh index 9b840f2fc..556ded156 100644 --- a/lix/libexpr/primops.hh +++ b/lix/libexpr/primops.hh @@ -33,7 +33,7 @@ struct RegisterPrimOp * will get called during EvalState initialization, so there * may be primops not yet added and builtins is not yet sorted. */ - RegisterPrimOp(PrimOp && primOp); + RegisterPrimOp(PrimOpDetails && primOp); }; /* These primops are disabled without enableNativeCode, but plugins diff --git a/lix/libexpr/value.cc b/lix/libexpr/value.cc index 9c95a3c5d..5a7ed0485 100644 --- a/lix/libexpr/value.cc +++ b/lix/libexpr/value.cc @@ -25,10 +25,7 @@ static void copyContextToValue(Value::String & s, const NixStringContext & conte } } -Value::Value(primop_t, PrimOp & primop) - : internalType(tPrimOp) - , _primOp(&primop) - , _primop_pad(0) +Value::Value(primop_t, PrimOp & primop) : internalType(tAuxiliary), _auxiliary(&primop), _aux_pad(0) { } @@ -49,9 +46,7 @@ bool Value::isTrivial() const void Value::mkPrimOp(PrimOp * p) { - clearValue(); - internalType = tPrimOp; - _primOp = p; + *this = {NewValueAs::primop, *p}; } void Value::mkString(std::string_view s) diff --git a/lix/libexpr/value.hh b/lix/libexpr/value.hh index 50a9b7b11..2e66b0a50 100644 --- a/lix/libexpr/value.hh +++ b/lix/libexpr/value.hh @@ -21,6 +21,53 @@ namespace nix { class BindingsBuilder; class EvalMemory; +class EvalState; +struct Value; + +/** + * Function that implements a primop. + */ +using PrimOpImpl = void(EvalState & state, Value ** args, Value & v); + +/** + * Info about a primitive operation, and its implementation + */ +struct PrimOpDetails +{ + /** + * Name of the primop. `__` prefix is treated specially. + */ + std::string name; + + /** + * Names of the parameters of a primop, for primops that take a + * fixed number of arguments to be substituted for these parameters. + */ + std::vector args; + + /** + * Aritiy of the primop. + * + * If `args` is not empty, this field will be computed from that + * field instead, so it doesn't need to be manually set. + */ + size_t arity = 0; + + /** + * Optional free-form documentation about the primop. + */ + const char * doc = nullptr; + + /** + * Implementation of the primop. + */ + std::function fun; + + /** + * Optional experimental for this to be gated on. + */ + std::optional experimentalFeature; +}; typedef enum { tInt = 1, @@ -31,7 +78,6 @@ typedef enum { tThunk, tApp, tLambda, - tPrimOp, tAuxiliary, } InternalType; @@ -79,7 +125,6 @@ struct Env; struct Expr; struct ExprLambda; struct ExprBlackHole; -struct PrimOp; class PosIdx; struct Pos; class StorePath; @@ -237,6 +282,7 @@ public: #undef USING_VALUETYPE struct List; + struct PrimOp; /// Default constructor which is still used in the codebase but should not /// be used in new code. Zero initializes its members. @@ -537,7 +583,10 @@ public: // type() == nFunction inline bool isLambda() const { return internalType == tLambda; }; - inline bool isPrimOp() const { return internalType == tPrimOp; }; + inline bool isPrimOp() const + { + return internalType == tAuxiliary && _auxiliary->type == Acb::tPrimOp; + } inline bool isPrimOpApp() const { return internalType == tApp && app().target()->isPrimOp(); @@ -640,6 +689,7 @@ public: tExternal, tFloat, tNull, + tPrimOp, } type; }; struct External : Acb @@ -652,6 +702,10 @@ public: }; struct Null : Acb {}; + struct PrimOp : Acb, PrimOpDetails + { + explicit PrimOp(PrimOpDetails && p) : Acb{tPrimOp}, PrimOpDetails(std::move(p)) {} + }; union { @@ -688,10 +742,6 @@ public: Env * env; ExprLambda * fun; } _lambda; - struct { - PrimOp * _primOp; - uintptr_t _primop_pad; - }; struct { const Acb * _auxiliary; uintptr_t _aux_pad; @@ -716,7 +766,6 @@ public: case tList: return nList; case tLambda: - case tPrimOp: return nFunction; case tAuxiliary: switch (_auxiliary->type) { @@ -726,6 +775,8 @@ public: return nFloat; case Acb::tNull: return nNull; + case Acb::tPrimOp: + return nFunction; } case tThunk: return nThunk; @@ -944,9 +995,10 @@ public: return _lambda; } - PrimOp * primOp() const + const PrimOp * primOp() const { - return _primOp; + assert(internalType == tAuxiliary && _auxiliary->type == Acb::tPrimOp); + return static_cast(_auxiliary); } const ExternalValueBase * external() const @@ -969,6 +1021,8 @@ public: using ValueVector = GcVector; +using PrimOp = Value::PrimOp; + /** * A value allocated in traceable memory. */ diff --git a/tests/unit/libexpr/value/print.cc b/tests/unit/libexpr/value/print.cc index 4965f473d..634f1ecc9 100644 --- a/tests/unit/libexpr/value/print.cc +++ b/tests/unit/libexpr/value/print.cc @@ -133,9 +133,7 @@ TEST_F(ValuePrintingTests, vLambda) TEST_F(ValuePrintingTests, vPrimOp) { Value vPrimOp; - PrimOp primOp{ - .name = "puppy" - }; + PrimOp primOp{{.name = "puppy"}}; vPrimOp.mkPrimOp(&primOp); test(vPrimOp, "«primop puppy»"); @@ -143,9 +141,7 @@ TEST_F(ValuePrintingTests, vPrimOp) TEST_F(ValuePrintingTests, vPrimOpApp) { - PrimOp primOp{ - .name = "puppy" - }; + PrimOp primOp{{.name = "puppy"}}; Value vPrimOp; vPrimOp.mkPrimOp(&primOp); @@ -582,9 +578,7 @@ TEST_F(ValuePrintingTests, ansiColorsLambda) TEST_F(ValuePrintingTests, ansiColorsPrimOp) { - PrimOp primOp{ - .name = "puppy" - }; + PrimOp primOp{{.name = "puppy"}}; Value v; v.mkPrimOp(&primOp); @@ -597,9 +591,7 @@ TEST_F(ValuePrintingTests, ansiColorsPrimOp) TEST_F(ValuePrintingTests, ansiColorsPrimOpApp) { - PrimOp primOp{ - .name = "puppy" - }; + PrimOp primOp{{.name = "puppy"}}; Value vPrimOp; vPrimOp.mkPrimOp(&primOp);