From d71e74838aade579f9e5e2771ba26b7077398e93 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 20 Mar 2024 22:56:42 +0100 Subject: [PATCH 1/4] readDir: Allocate type strings only once --- src/libexpr/eval.cc | 4 ++++ src/libexpr/eval.hh | 9 +++++++++ src/libexpr/primops.cc | 21 +++++++++------------ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index fb4cfdccf..a6e8a4a8b 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -437,6 +437,10 @@ EvalState::EvalState( vEmptyList.mkList(buildList(0)); vNull.mkNull(); + vStringRegular.mkString("regular"); + vStringDirectory.mkString("directory"); + vStringSymlink.mkString("symlink"); + vStringUnknown.mkString("unknown"); /* Initialise the Nix expression search path. */ if (!evalSettings.pureEval) { diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 7db911fce..a405888c1 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -191,6 +191,15 @@ public: */ Value vNull; + /** `"regular"` */ + Value vStringRegular; + /** `"directory"` */ + Value vStringDirectory; + /** `"symlink"` */ + Value vStringSymlink; + /** `"unknown"` */ + Value vStringUnknown; + /** * The accessor for the root filesystem. */ diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 61a11b226..2022f6dcf 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1775,20 +1775,20 @@ static RegisterPrimOp primop_hashFile({ .fun = prim_hashFile, }); -static std::string_view fileTypeToString(InputAccessor::Type type) +static Value * fileTypeToString(EvalState & state, InputAccessor::Type type) { return - type == InputAccessor::Type::tRegular ? "regular" : - type == InputAccessor::Type::tDirectory ? "directory" : - type == InputAccessor::Type::tSymlink ? "symlink" : - "unknown"; + type == InputAccessor::Type::tRegular ? &state.vStringRegular : + type == InputAccessor::Type::tDirectory ? &state.vStringDirectory : + type == InputAccessor::Type::tSymlink ? &state.vStringSymlink : + &state.vStringUnknown; } static void prim_readFileType(EvalState & state, const PosIdx pos, Value * * args, Value & v) { auto path = realisePath(state, pos, *args[0], std::nullopt); /* Retrieve the directory entry type and stringize it. */ - v.mkString(fileTypeToString(path.lstat().type)); + v = *fileTypeToString(state, path.lstat().type); } static RegisterPrimOp primop_readFileType({ @@ -1819,8 +1819,8 @@ static void prim_readDir(EvalState & state, const PosIdx pos, Value * * args, Va Value * readFileType = nullptr; for (auto & [name, type] : entries) { - auto & attr = attrs.alloc(name); if (!type) { + auto & attr = attrs.alloc(name); // Some filesystems or operating systems may not be able to return // detailed node info quickly in this case we produce a thunk to // query the file type lazily. @@ -1832,7 +1832,7 @@ static void prim_readDir(EvalState & state, const PosIdx pos, Value * * args, Va } else { // This branch of the conditional is much more likely. // Here we just stringize the directory entry type. - attr.mkString(fileTypeToString(*type)); + attrs.insert(state.symbols.create(name), fileTypeToString(state, *type)); } } @@ -2193,11 +2193,8 @@ bool EvalState::callPathFilter( Value arg1; arg1.mkString(pathArg); - Value arg2; // assert that type is not "unknown" - arg2.mkString(fileTypeToString(st.type)); - - Value * args []{&arg1, &arg2}; + Value * args []{&arg1, fileTypeToString(*this, st.type)}; Value res; callFunction(*filterFun, 2, args, res, pos); From a865049c4f39cb7773f97a67cfa12f5b650a86ee Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 20 Mar 2024 23:06:23 +0100 Subject: [PATCH 2/4] tryEval: Allocate true and false once --- src/libexpr/eval.cc | 2 ++ src/libexpr/eval.hh | 18 +++++++++++++++++- src/libexpr/primops.cc | 7 ++++--- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index a6e8a4a8b..a62cee299 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -437,6 +437,8 @@ EvalState::EvalState( vEmptyList.mkList(buildList(0)); vNull.mkNull(); + vTrue.mkBool(true); + vFalse.mkBool(false); vStringRegular.mkString("regular"); vStringDirectory.mkString("directory"); vStringSymlink.mkString("symlink"); diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index a405888c1..eac83fe34 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -187,10 +187,26 @@ public: Value vEmptyList; /** - * Null constant. + * `null` constant. + * + * This is _not_ a singleton. Pointer equality is _not_ sufficient. */ Value vNull; + /** + * `true` constant. + * + * This is _not_ a singleton. Pointer equality is _not_ sufficient. + */ + Value vTrue; + + /** + * `true` constant. + * + * This is _not_ a singleton. Pointer equality is _not_ sufficient. + */ + Value vFalse; + /** `"regular"` */ Value vStringRegular; /** `"directory"` */ diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 2022f6dcf..0f2aaa83f 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -896,10 +896,11 @@ static void prim_tryEval(EvalState & state, const PosIdx pos, Value * * args, Va try { state.forceValue(*args[0], pos); attrs.insert(state.sValue, args[0]); - attrs.alloc("success").mkBool(true); + attrs.insert(state.symbols.create("success"), &state.vTrue); } catch (AssertionError & e) { - attrs.alloc(state.sValue).mkBool(false); - attrs.alloc("success").mkBool(false); + // `value = false;` is unfortunate but removing it is a breaking change. + attrs.insert(state.sValue, &state.vFalse); + attrs.insert(state.symbols.create("success"), &state.vFalse); } // restore the debugRepl pointer if we saved it earlier. From 8c6e0df45f9091fc27143f24d4fe782c661393e2 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 20 Mar 2024 23:07:00 +0100 Subject: [PATCH 3/4] value.hh: Fix warning about {struct/class} Value --- src/libexpr/value.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 885621cf5..335801b34 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -159,7 +159,7 @@ public: iterator begin() { return &elems[0]; } iterator end() { return &elems[size]; } - friend class Value; + friend struct Value; }; From 1fcdd1640ec2e63f14487c1af60514fb62ffef19 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 20 Mar 2024 23:11:54 +0100 Subject: [PATCH 4/4] functionArgs: Allocate bools only once --- src/libexpr/eval.cc | 3 +++ src/libexpr/eval.hh | 5 +++++ src/libexpr/primops.cc | 3 +-- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index a62cee299..5e2f71649 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -937,6 +937,9 @@ ListBuilder::ListBuilder(EvalState & state, size_t size) state.nrListElems += size; } +Value * EvalState::getBool(bool b) { + return b ? &vTrue : &vFalse; +} unsigned long nrThunks = 0; diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index eac83fe34..f15d19653 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -650,6 +650,11 @@ public: return ListBuilder(*this, size); } + /** + * Return a boolean `Value *` without allocating. + */ + Value *getBool(bool b); + void mkThunk_(Value & v, Expr * expr); void mkPos(Value & v, PosIdx pos); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 0f2aaa83f..d0fcfd194 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -2845,8 +2845,7 @@ static void prim_functionArgs(EvalState & state, const PosIdx pos, Value * * arg auto attrs = state.buildBindings(args[0]->lambda.fun->formals->formals.size()); for (auto & i : args[0]->lambda.fun->formals->formals) - // !!! should optimise booleans (allocate only once) - attrs.alloc(i.name, i.pos).mkBool(i.def); + attrs.insert(i.name, state.getBool(i.def), i.pos); v.mkAttrs(attrs); }