wip

Author: ConnorBaker

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: ${

src/libexpr/eval-inline.hh

@@ -65,26 +65,13 @@ 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()
 {
     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));

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);
-}
+            }
         }
     };
 

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;
 }
 

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);