diff --git a/flake.nix b/flake.nix index 704de0f59ee..7136c4b2908 100644 --- a/flake.nix +++ b/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})) ]; diff --git a/package.nix b/package.nix index 50526769be8..5cd5574ce4c 100644 --- a/package.nix +++ b/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 ); diff --git a/src/libexpr/eval-gc.cc b/src/libexpr/eval-gc.cc index 07ce05a2c73..45a8cf67cc1 100644 --- a/src/libexpr/eval-gc.cc +++ b/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. */ diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index e0e2b095c6a..e69764f9f4b 100644 --- a/src/libexpr/eval-inline.hh +++ b/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 -[[gnu::always_inline]] -ValueList * EvalState::allocListFromInitializerList(std::initializer_list 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 +[[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) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index a4dd1f132ab..874fd1269a7 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -23,17 +23,23 @@ #include "value.hh" #include -#include -#include #include -#include -#include -#include #include #include +#include +#include +#include +#include +#include +#include +#include -#include #include +#include +#include +#include +#include +#include #ifndef _WIN32 // TODO use portable implementation # include @@ -274,6 +280,7 @@ EvalState::EvalState( , regexCache(makeRegexCache()) #if HAVE_BOEHMGC , valueAllocCache(std::allocate_shared(traceable_allocator(), nullptr)) + , listAllocCache(std::allocate_shared(traceable_allocator(), nullptr)) , env1AllocCache(std::allocate_shared(traceable_allocator(), nullptr)) , baseEnvP(std::allocate_shared(traceable_allocator(), &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>::type::ownee::operator=(edit) + // [HeapPolicy = immer::heap_policy]: 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; + })); } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 5941ac41fe5..8a6267a5c95 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -355,6 +355,11 @@ private: */ std::shared_ptr valueAllocCache; + /** + * Allocation cache for GC'd ValueList objects. + */ + std::shared_ptr listAllocCache; + /** * Allocation cache for size-1 Env objects. */ @@ -699,8 +704,7 @@ public: * Allocation primitives. */ inline ValueList * allocList(); - template - inline ValueList * allocListFromInitializerList(std::initializer_list values); + template inline ValueList * allocListFromRange(Range range); inline Value * allocValue(); inline Env & allocEnv(size_t size); diff --git a/src/libexpr/meson.build b/src/libexpr/meson.build index 098113c4fed..06b4e4d30a8 100644 --- a/src/libexpr/meson.build +++ b/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 diff --git a/src/libexpr/package.nix b/src/libexpr/package.nix index 68eee148b98..3645c01ff09 100644 --- a/src/libexpr/package.nix +++ b/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 = diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 4c14a628204..c92576499f0 100644 --- a/src/libexpr/value.hh +++ b/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 ValueList; -typedef immer::flex_vector_transient ValueListTransient; +typedef ValueList::transient_type ValueListTransient; typedef enum { tUninitialized = 0,