From f742092d6a9131d81daf6dd836ce415aee31ba75 Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Sat, 16 Nov 2024 07:41:11 +0000 Subject: [PATCH] wip --- src/libexpr-tests/nix_api_expr.cc | 2 -- src/libexpr/eval-inline.hh | 13 ------------- src/libexpr/eval.cc | 4 ++-- src/libexpr/get-drvs.cc | 10 +++++++--- src/libexpr/primops.cc | 24 +++++++++++++++++------- 5 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/libexpr-tests/nix_api_expr.cc b/src/libexpr-tests/nix_api_expr.cc index 7a879cdda..b37ac44b3 100644 --- a/src/libexpr-tests/nix_api_expr.cc +++ b/src/libexpr-tests/nix_api_expr.cc @@ -118,7 +118,6 @@ TEST_F(nix_api_expr_test, nix_expr_realise_context_bad_value) TEST_F(nix_api_expr_test, nix_expr_realise_context_bad_build) { - // TODO(@connorbaker): Fails because "allow-import-from-derivation" is disabled on host. auto expr = R"( derivation { name = "letsbuild"; system = builtins.currentSystem; @@ -137,7 +136,6 @@ TEST_F(nix_api_expr_test, nix_expr_realise_context_bad_build) TEST_F(nix_api_expr_test, nix_expr_realise_context) { // TODO (ca-derivations): add a content-addressed derivation output, which produces a placeholder - // TODO(@connorbaker): Fails because "allow-import-from-derivation" is disabled on host. auto expr = R"( '' a derivation output: ${ diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 4539f7ce4..8ef0acfc2 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -65,16 +65,6 @@ inline Value * EvalState::allocValue() } -// TODO(@connorbaker): -// Is it possible using the batched alloc resulted in a performance regression? -// $ hyperfine --warmup 2 --min-runs 20 --export-markdown system-eval-bench.md --parameter-list nix-version nix-after-batch-alloc './{nix-version}/bin/nix eval --no-eval-cache --read-only github:ConnorBaker/nixos-configs/39f01b05c37494627242162863b4725e38600b50#nixosConfigurations.nixos-desktop.config.system.build.toplevel' && hyperfine --warmup 2 --min-runs 20 --export-markdown list-concat-eval-bench.md --parameter-list nix-version nix-after-batch-alloc './{nix-version}/bin/nix eval --no-eval-cache --read-only --json --file ./nixpkgs-91bc7dadf7bf0bf3fb9203e611bd856f40fc2b66/pkgs/top-level/release-attrpaths-superset.nix names' -// Benchmark 1: ./nix-after-batch-alloc/bin/nix eval --no-eval-cache --read-only github:ConnorBaker/nixos-configs/39f01b05c37494627242162863b4725e38600b50#nixosConfigurations.nixos-desktop.config.system.build.toplevel -// Time (mean ± σ): 3.357 s ± 0.030 s [User: 3.003 s, System: 0.339 s] -// Range (min … max): 3.327 s … 3.442 s 20 runs -// -// Benchmark 1: ./nix-after-batch-alloc/bin/nix eval --no-eval-cache --read-only --json --file ./nixpkgs-91bc7dadf7bf0bf3fb9203e611bd856f40fc2b66/pkgs/top-level/release-attrpaths-superset.nix names -// Time (mean ± σ): 19.000 s ± 0.050 s [User: 17.066 s, System: 1.888 s] -// Range (min … max): 18.932 s … 19.145 s 20 runs [[nodiscard]] [[using gnu: hot, always_inline, returns_nonnull, malloc]] inline ValueList * EvalState::allocList() @@ -82,9 +72,6 @@ inline ValueList * EvalState::allocList() void * p = nullptr; // See the comment in allocValue for an explanation of this block. - // TODO(@connorbaker): Beginning cargo-culting. - // 1. Do we need to assign to an intermediate variable, like `allocEnv` does? - // 2. Do we need to use a C-style cast? if constexpr (HAVE_BOEHMGC) { if (*listAllocCache == nullptr) [[unlikely]] { *listAllocCache = GC_malloc_many(sizeof(ValueList)); diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 9c696bc1a..c54a94253 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -2120,7 +2120,7 @@ void EvalState::forceValueDeep(Value & v) addErrorTrace(e, i.pos, "while evaluating the attribute '%1%'", symbols[i.name]); throw; } -} + } } else if (v.isList()) { @@ -2129,7 +2129,7 @@ void EvalState::forceValueDeep(Value & v) // Increases the heap size by a fair amount, potentially because of the lambda capture. for (auto * const v2 : v.list()) { recurse(*v2); -} + } } }; diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 4d35fab61..7ffbdf247 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -215,13 +215,17 @@ bool PackageInfo::checkMeta(Value & v) return checkMeta(*elem); }); } - else if (v.type() == nAttrs) { - if (v.attrs()->get(state->sOutPath)) return false; + + if (v.type() == nAttrs) { + if (v.attrs()->get(state->sOutPath) != nullptr) { + return false; + } return ranges::all_of(*v.attrs(), [&](const auto & i) { return checkMeta(*i.value); }); } - else return v.type() == nInt || v.type() == nBool || v.type() == nString || + + return v.type() == nInt || v.type() == nBool || v.type() == nString || v.type() == nFloat; } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 8f423ac33..8ebf03286 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -4407,18 +4407,28 @@ static void prim_concatStringsSep(EvalState & state, const PosIdx pos, Value * * { NixStringContext context; - auto sep = state.forceString(*args[0], context, pos, "while evaluating the first argument (the separator string) passed to builtins.concatStringsSep"); + const auto sep = state.forceString(*args[0], context, pos, "while evaluating the first argument (the separator string) passed to builtins.concatStringsSep"); state.forceList(*args[1], pos, "while evaluating the second argument (the list of strings to concat) passed to builtins.concatStringsSep"); + const auto list = args[1]->list(); + const auto numElements = list.size(); std::string res; - res.reserve((args[1]->listSize() + 32) * sep.size()); - bool first = true; - // TODO(@connorbaker): Resume rewrite and update here. See if ranges has something for this. + if (numElements == 0) { + v.mkString(res, context); + return; + } - for (auto elem : args[1]->list()) { - if (first) first = false; else res += sep; - res += *state.coerceToString(pos, *elem, context, "while evaluating one element of the list of strings to concat passed to builtins.concatStringsSep"); + // Manually handle the first element for the singleton case. + auto * const head = list.front(); + res = *state.coerceToString(pos, *head, context, "while evaluating one element of the list of strings to concat passed to builtins.concatStringsSep"); + + if (numElements > 1) { + res.reserve((numElements + 32) * sep.size()); + immer::for_each(list.drop(1), [&](auto * const elem) { + res += sep; + res += *state.coerceToString(pos, *elem, context, "while evaluating one element of the list of strings to concat passed to builtins.concatStringsSep"); + }); } v.mkString(res, context);