wip alloc scheme and use of ranges

flake.nix

@@ -98,31 +98,41 @@
               useLLVM = true;
             };
             overlays = [
-              # Update immer to the latest
+              # Update immer and range-v3 to the latest
               # TODO(@connorbaker): Upstream this.
               (final: prev: {
                 immer =
-                let
-                  inherit (final.lib.strings) cmakeBool;
-                in
-                prev.immer.overrideAttrs (finalAttrs: _: {
+                  let
+                    inherit (final.lib.strings) cmakeBool;
+                  in
+                  prev.immer.overrideAttrs (finalAttrs: _: {
+                    strictDeps = true;
+                    version = "0.8.1-unstable-2024-09-18";
+                    src = final.fetchFromGitHub {
+                      owner = "arximboldi";
+                      repo = "immer";
+                      rev = "df6ef46d97e1fe81f397015b9aeb32505cef653b";
+                      hash = "sha256-fV6Rqbg/vtUH2DdgLYULl0zLM3WUSG1qYLZtqAhaWQw=";
+                    };
+                    doCheck = false;
+                    # TODO(@connorbaker): Add support for doCheck = true;
+                    cmakeFlags = [
+                      (cmakeBool "immer_BUILD_DOCS" false)
+                      (cmakeBool "immer_BUILD_EXAMPLES" finalAttrs.doCheck)
+                      (cmakeBool "immer_BUILD_EXTRAS" false)
+                      (cmakeBool "immer_BUILD_TESTS" finalAttrs.doCheck)
+                    ];
+                  });
+                range-v3 = prev.range-v3.overrideAttrs {
                   strictDeps = true;
-                  version = "0.8.1-unstable-2024-09-18";
+                  version = "0.12.0-unstable-2024-10-01";
                   src = final.fetchFromGitHub {
-                    owner = "arximboldi";
-                    repo = "immer";
-                    rev = "df6ef46d97e1fe81f397015b9aeb32505cef653b";
-                    hash = "sha256-fV6Rqbg/vtUH2DdgLYULl0zLM3WUSG1qYLZtqAhaWQw=";
+                    owner = "ericniebler";
+                    repo = "range-v3";
+                    rev = "7e6f34b1e820fb8321346888ef0558a0ec842b8e";
+                    hash = "sha256-FbLZ8CHnLdTs0FRdLPTOBiw2lcuIuSoFSb8E4MD8DWQ=";
                   };
-                  doCheck = false;
-                  # TODO(@connorbaker): Add support for doCheck = true;
-                  cmakeFlags = [
-                    (cmakeBool "immer_BUILD_DOCS" false)
-                    (cmakeBool "immer_BUILD_EXAMPLES" finalAttrs.doCheck)
-                    (cmakeBool "immer_BUILD_EXTRAS" false)
-                    (cmakeBool "immer_BUILD_TESTS" finalAttrs.doCheck)
-                  ];
-                });
+                };
               })
               (overlayFor (p: p.${stdenv}))
             ];

package.nix

@@ -31,6 +31,7 @@
 , openssh
 , openssl
 , pkg-config
+, range-v3
 , rapidcheck
 , sqlite
 , toml11
@@ -246,6 +247,7 @@ in {
     boost
     immer
     nlohmann_json
+    range-v3
   ] ++ lib.optional enableGC boehmgc
   );
 

src/libexpr/eval-gc.cc

@@ -35,10 +35,7 @@ static void * oomHandler(size_t requested)
 static inline void initGCReal()
 {
     /* Initialise the Boehm garbage collector. */
-
-    /* Don't look for interior pointers. This reduces the odds of
-       misdetection a bit. */
-    GC_set_all_interior_pointers(0);
+    GC_set_all_interior_pointers(1);
 
     /* We don't have any roots in data segments, so don't scan from
        there. */

src/libexpr/eval-inline.hh

@@ -25,23 +25,6 @@ inline void * allocBytes(size_t n)
     return p;
 }
 
-/* Allocate a new array of attributes for an attribute set with a specific
-   capacity. The space is implicitly reserved after the Bindings
-   structure. */
-[[gnu::always_inline]]
-ValueList * EvalState::allocList()
-{
-    return new (allocBytes(sizeof(ValueList))) ValueList();
-}
-
-template <typename T>
-[[gnu::always_inline]]
-ValueList * EvalState::allocListFromInitializerList(std::initializer_list<T> values)
-{
-    return new (allocBytes(sizeof(ValueList))) ValueList(values);
-}
-
-
 [[gnu::always_inline]]
 Value * EvalState::allocValue()
 {
@@ -68,6 +51,63 @@ Value * EvalState::allocValue()
     return (Value *) p;
 }
 
+// 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
+[[gnu::always_inline]]
+ValueList * EvalState::allocList()
+{
+    // 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 HAVE_BOEHMGC
+    if (!*listAllocCache) {
+        *listAllocCache = GC_malloc_many(sizeof(ValueList));
+        if (!*listAllocCache) throw std::bad_alloc();
+    }
+
+    void * p = *listAllocCache;
+    *listAllocCache = GC_NEXT(p);
+    GC_NEXT(p) = nullptr;
+#else
+    void * p = allocBytes(sizeof(ValueList));
+#endif
+
+    return ::new (p) ValueList;
+}
+
+template <typename Range>
+[[gnu::always_inline]]
+ValueList * EvalState::allocListFromRange(Range range)
+{
+    // 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 HAVE_BOEHMGC
+    if (!*listAllocCache) {
+        *listAllocCache = GC_malloc_many(sizeof(ValueList));
+        if (!*listAllocCache) throw std::bad_alloc();
+    }
+
+    void * p = *listAllocCache;
+    *listAllocCache = GC_NEXT(p);
+    GC_NEXT(p) = nullptr;
+#else
+    void * p = allocBytes(sizeof(ValueList));
+#endif
+
+    return ::new (p) ValueList(range.begin(), range.end());
+}
+
 
 [[gnu::always_inline]]
 Env & EvalState::allocEnv(size_t size)

src/libexpr/eval.cc

@@ -23,17 +23,23 @@
 #include "value.hh"
 
 #include <algorithm>
-#include <iostream>
-#include <sstream>
 #include <cstring>
-#include <optional>
-#include <unistd.h>
-#include <sys/time.h>
 #include <fstream>
 #include <functional>
+#include <iostream>
+#include <memory>
+#include <optional>
+#include <sstream>
+#include <sys/time.h>
+#include <unistd.h>
+#include <utility>
 
-#include <nlohmann/json.hpp>
 #include <boost/container/small_vector.hpp>
+#include <immer/algorithm.hpp>
+#include <nlohmann/json.hpp>
+#include <range/v3/algorithm/fold_left.hpp>
+#include <range/v3/algorithm/transform.hpp>
+#include <range/v3/view/transform.hpp>
 
 #ifndef _WIN32 // TODO use portable implementation
 #  include <sys/resource.h>
@@ -274,6 +280,7 @@ EvalState::EvalState(
     , regexCache(makeRegexCache())
 #if HAVE_BOEHMGC
     , valueAllocCache(std::allocate_shared<void *>(traceable_allocator<void *>(), nullptr))
+    , listAllocCache(std::allocate_shared<void *>(traceable_allocator<void *>(), nullptr))
     , env1AllocCache(std::allocate_shared<void *>(traceable_allocator<void *>(), nullptr))
     , baseEnvP(std::allocate_shared<Env *>(traceable_allocator<Env *>(), &allocEnv(BASE_ENV_SIZE)))
     , baseEnv(**baseEnvP)
@@ -1295,14 +1302,22 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
 
 void ExprList::eval(EvalState & state, Env & env, Value & v)
 {
-    // TODO(@connorbaker): Tried to switch to using transient here, but started getting this error:
-    // Value::type: invalid internal type: -183796896
+    // TODO(@connorbaker): Tried to switch to using transient here, but started getting this error when calling transience.push_back:
+    // nix: /nix/store/gkghbi5d6849mwsbcdhnqljz2xnjvnis-immer-0.8.1-unstable-2024-09-18/include/immer/transience/gc_transience_policy.hpp:95:
+    // ownee &immer::gc_transience_policy::apply<immer::heap_policy<immer::gc_heap>>::type::ownee::operator=(edit)
+    // [HeapPolicy = immer::heap_policy<immer::gc_heap>]: Assertion `e != noone' failed.
     // That's an indication the value is being used after it's been freed. Not sure why that's happening.
     // NOTE: Running with GC_DONT_GC=1 doesn't seem to trigger the error, so it's likely a GC issue.
-    auto list = state.allocList();
-    for (auto & i : elems)
-        *list = list->push_back(i->maybeThunk(state, env));
-    v.mkList(list);
+    v.mkList(ranges::fold_left(elems, state.allocList(), [&](const auto & list, const auto & elem) {
+        *list = list->push_back(elem->maybeThunk(state, env));
+        return list;
+    }));
+    // TODO(@connorbaker): This doesn't work either, which leads me to suspect the implementation of the mutable data
+    // structure isn't correctly interfacing with the GC. Is this because it is intended to store values, rather than
+    // references to values like I'm using it for currently?
+    // v.mkList(state.allocListFromRange(elems | ranges::views::transform([&](const auto & elem) {
+    //     return elem->maybeThunk(state, env);
+    // })));
 }
 
 
@@ -1899,9 +1914,12 @@ void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v)
 
 void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v)
 {
+    state.nrListConcats++;
+
     auto v1 = state.allocValue(); 
     e1->eval(state, env, *v1);
     state.forceList(*v1, pos, "in the left operand of the list concatenation operator");
+
     auto v2 = state.allocValue();
     e2->eval(state, env, *v2);
     state.forceList(*v2, pos, "in the right operand of the list concatenation operator");
@@ -1918,12 +1936,11 @@ void EvalState::concatLists(Value & v, const ValueList lists, const PosIdx pos,
 
     // TODO(@connorbaker): We should be able to get a pointer to the list from the first argument and then concatenate from there.
     // However, because Value is mutable, I can't think of an easy way to do that.
-    auto newList = allocList();
-    for (auto list : lists) {
+    v.mkList(immer::accumulate(lists, allocList(), [&](const auto & concatenated, const auto & list) {
         forceList(*list, pos, errorCtx);
-        *newList = *newList + list->list();
-    }
-    v.mkList(newList);
+        *concatenated = *concatenated + list->list();
+        return concatenated;
+    }));
 }
 
 

src/libexpr/eval.hh

@@ -355,6 +355,11 @@ private:
      */
     std::shared_ptr<void *> valueAllocCache;
 
+    /**
+     * Allocation cache for GC'd ValueList objects.
+     */
+    std::shared_ptr<void *> listAllocCache;
+
     /**
      * Allocation cache for size-1 Env objects.
      */
@@ -699,8 +704,7 @@ public:
      * Allocation primitives.
      */
     inline ValueList * allocList();
-    template <typename T>
-    inline ValueList * allocListFromInitializerList(std::initializer_list<T> values);
+    template <typename Range> inline ValueList * allocListFromRange(Range range);
     inline Value * allocValue();
     inline Env & allocEnv(size_t size);
 

src/libexpr/meson.build

@@ -46,6 +46,14 @@ immer = dependency(
 )
 deps_other += immer
 
+range_v3 = dependency(
+  'range-v3',
+  version : '>=0.12.0',
+  method : 'cmake',
+  include_type: 'system',
+)
+deps_other += range_v3
+
 nlohmann_json = dependency('nlohmann_json', version : '>= 3.9')
 deps_public += nlohmann_json
 

src/libexpr/package.nix

@@ -13,6 +13,7 @@
 , boost
 , boehmgc
 , nlohmann_json
+, range-v3
 , toml11
 
 # Configuration Options
@@ -75,6 +76,7 @@ mkMesonLibrary (finalAttrs: {
     boost
     immer
     nlohmann_json
+    range-v3
   ] ++ lib.optional enableGC boehmgc;
 
   preConfigure =

src/libexpr/value.hh

@@ -35,7 +35,7 @@ class BindingsBuilder;
 // alias the vector type so we are not concerned about memory policies
 // in the places where we actually use it
 typedef immer::flex_vector<nix::Value *, gc_policy, 5U, 5U> ValueList;
-typedef immer::flex_vector_transient<nix::Value *, gc_policy, 5U, 5U> ValueListTransient;
+typedef ValueList::transient_type ValueListTransient;
 
 typedef enum {
     tUninitialized = 0,