From 0a24286a44c0620849f296dc0b41727e95419830 Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Tue, 29 Oct 2024 21:00:58 +0000 Subject: [PATCH 1/9] wip --- .gitignore | 2 + flake.nix | 26 ++ packaging/dev-shell.nix | 5 +- src/libexpr-c/nix_api_expr_internal.h | 5 - src/libexpr-c/nix_api_value.cc | 49 +--- src/libexpr-c/nix_api_value.h | 42 --- src/libexpr-tests/nix_api_expr.cc | 2 + src/libexpr-tests/nix_api_value.cc | 29 -- src/libexpr-tests/primops.cc | 94 +++---- src/libexpr-tests/value/print.cc | 49 ++-- src/libexpr/attr-path.cc | 2 +- src/libexpr/eval-cache.cc | 2 +- src/libexpr/eval-inline.hh | 15 +- src/libexpr/eval.cc | 69 ++--- src/libexpr/eval.hh | 9 +- src/libexpr/get-drvs.cc | 8 +- src/libexpr/json-to-value.cc | 8 +- src/libexpr/meson.build | 9 + src/libexpr/package.nix | 2 + src/libexpr/primops.cc | 390 ++++++++++++++------------ src/libexpr/primops/context.cc | 13 +- src/libexpr/primops/fromTOML.cc | 11 +- src/libexpr/print-ambiguous.cc | 16 +- src/libexpr/print-options.hh | 1 + src/libexpr/print.cc | 10 +- src/libexpr/value-to-json.cc | 2 +- src/libexpr/value-to-xml.cc | 2 +- src/libexpr/value.hh | 95 ++----- src/libflake/flake/flake.cc | 2 +- src/nix-env/nix-env.cc | 2 +- src/nix-env/user-env.cc | 20 +- src/nix/prefetch.cc | 4 +- 32 files changed, 469 insertions(+), 526 deletions(-) diff --git a/.gitignore b/.gitignore index de1183977b3..68de0dc778f 100644 --- a/.gitignore +++ b/.gitignore @@ -155,6 +155,8 @@ result-* # IDE .vscode/ .idea/ +.direnv/ +.envrc .pre-commit-config.yaml diff --git a/flake.nix b/flake.nix index 06025e3b79c..04b16878d41 100644 --- a/flake.nix +++ b/flake.nix @@ -98,6 +98,32 @@ useLLVM = true; }; overlays = [ + # Update immer to the latest + # TODO(@connorbaker): Upstream this. + (final: prev: { + immer = + 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) + ]; + }); + }) (overlayFor (p: p.${stdenv})) ]; }; diff --git a/packaging/dev-shell.nix b/packaging/dev-shell.nix index 30ac518d5f7..b64dc0858fb 100644 --- a/packaging/dev-shell.nix +++ b/packaging/dev-shell.nix @@ -112,7 +112,10 @@ in { # TODO: Remove the darwin check once # https://github.com/NixOS/nixpkgs/pull/291814 is available ++ lib.optional (stdenv.cc.isClang && !stdenv.buildPlatform.isDarwin) pkgs.buildPackages.bear - ++ lib.optional (stdenv.cc.isClang && stdenv.hostPlatform == stdenv.buildPlatform) (lib.hiPrio pkgs.buildPackages.clang-tools); + ++ lib.optional (stdenv.cc.isClang && stdenv.hostPlatform == stdenv.buildPlatform) [ + (lib.hiPrio pkgs.buildPackages.clang-tools-18) + pkgs.buildPackages.llvmPackages_18.lldb + ]; buildInputs = attrs.buildInputs or [] ++ pkgs.nixComponents.nix-util.buildInputs diff --git a/src/libexpr-c/nix_api_expr_internal.h b/src/libexpr-c/nix_api_expr_internal.h index 12f24b6eb07..1bb5042b86f 100644 --- a/src/libexpr-c/nix_api_expr_internal.h +++ b/src/libexpr-c/nix_api_expr_internal.h @@ -19,11 +19,6 @@ struct BindingsBuilder nix::BindingsBuilder builder; }; -struct ListBuilder -{ - nix::ListBuilder builder; -}; - struct nix_value { nix::Value value; diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index bae078d312f..5514d9c0b61 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -324,7 +324,7 @@ nix_value * nix_get_list_byidx(nix_c_context * context, const nix_value * value, try { auto & v = check_value_in(value); assert(v.type() == nix::nList); - auto * p = v.listElems()[ix]; + auto * p = v.list()[ix]; nix_gc_incref(nullptr, p); if (p != nullptr) state->state.forceValue(*p, nix::noPos); @@ -490,53 +490,6 @@ nix_err nix_init_external(nix_c_context * context, nix_value * value, ExternalVa NIXC_CATCH_ERRS } -ListBuilder * nix_make_list_builder(nix_c_context * context, EvalState * state, size_t capacity) -{ - if (context) - context->last_err_code = NIX_OK; - try { - auto builder = state->state.buildList(capacity); - return new -#if HAVE_BOEHMGC - (NoGC) -#endif - ListBuilder{std::move(builder)}; - } - NIXC_CATCH_ERRS_NULL -} - -nix_err -nix_list_builder_insert(nix_c_context * context, ListBuilder * list_builder, unsigned int index, nix_value * value) -{ - if (context) - context->last_err_code = NIX_OK; - try { - auto & e = check_value_not_null(value); - list_builder->builder[index] = &e; - } - NIXC_CATCH_ERRS -} - -void nix_list_builder_free(ListBuilder * list_builder) -{ -#if HAVE_BOEHMGC - GC_FREE(list_builder); -#else - delete list_builder; -#endif -} - -nix_err nix_make_list(nix_c_context * context, ListBuilder * list_builder, nix_value * value) -{ - if (context) - context->last_err_code = NIX_OK; - try { - auto & v = check_value_out(value); - v.mkList(list_builder->builder); - } - NIXC_CATCH_ERRS -} - nix_err nix_init_primop(nix_c_context * context, nix_value * value, PrimOp * p) { if (context) diff --git a/src/libexpr-c/nix_api_value.h b/src/libexpr-c/nix_api_value.h index 8a0813ebe1f..f8337028d78 100644 --- a/src/libexpr-c/nix_api_value.h +++ b/src/libexpr-c/nix_api_value.h @@ -50,15 +50,6 @@ typedef struct EvalState EvalState; */ typedef struct BindingsBuilder BindingsBuilder; -/** @brief Stores an under-construction list - * @ingroup value_manip - * - * Do not reuse. - * @see nix_make_list_builder, nix_list_builder_free, nix_make_list - * @see nix_list_builder_insert - */ -typedef struct ListBuilder ListBuilder; - /** @brief PrimOp function * @ingroup primops * @@ -393,39 +384,6 @@ nix_err nix_init_apply(nix_c_context * context, nix_value * value, nix_value * f */ nix_err nix_init_external(nix_c_context * context, nix_value * value, ExternalValue * val); -/** @brief Create a list from a list builder - * @param[out] context Optional, stores error information - * @param[in] list_builder list builder to use. Make sure to unref this afterwards. - * @param[out] value Nix value to modify - * @return error code, NIX_OK on success. - */ -nix_err nix_make_list(nix_c_context * context, ListBuilder * list_builder, nix_value * value); - -/** @brief Create a list builder - * @param[out] context Optional, stores error information - * @param[in] state nix evaluator state - * @param[in] capacity how many bindings you'll add. Don't exceed. - * @return owned reference to a list builder. Make sure to unref when you're done. - */ -ListBuilder * nix_make_list_builder(nix_c_context * context, EvalState * state, size_t capacity); - -/** @brief Insert bindings into a builder - * @param[out] context Optional, stores error information - * @param[in] list_builder ListBuilder to insert into - * @param[in] index index to manipulate - * @param[in] value value to insert - * @return error code, NIX_OK on success. - */ -nix_err -nix_list_builder_insert(nix_c_context * context, ListBuilder * list_builder, unsigned int index, nix_value * value); - -/** @brief Free a list builder - * - * Does not fail. - * @param[in] list_builder The builder to free. - */ -void nix_list_builder_free(ListBuilder * list_builder); - /** @brief Create an attribute set from a bindings builder * @param[out] context Optional, stores error information * @param[out] value Nix value to modify diff --git a/src/libexpr-tests/nix_api_expr.cc b/src/libexpr-tests/nix_api_expr.cc index b37ac44b317..7a879cdda6c 100644 --- a/src/libexpr-tests/nix_api_expr.cc +++ b/src/libexpr-tests/nix_api_expr.cc @@ -118,6 +118,7 @@ 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; @@ -136,6 +137,7 @@ 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-tests/nix_api_value.cc b/src/libexpr-tests/nix_api_value.cc index 7fc8b4f641f..a88389bff6b 100644 --- a/src/libexpr-tests/nix_api_value.cc +++ b/src/libexpr-tests/nix_api_value.cc @@ -143,35 +143,6 @@ TEST_F(nix_api_expr_test, nix_build_and_init_list_invalid) assert_ctx_err(); } -TEST_F(nix_api_expr_test, nix_build_and_init_list) -{ - int size = 10; - ListBuilder * builder = nix_make_list_builder(ctx, state, size); - - nix_value * intValue = nix_alloc_value(ctx, state); - nix_value * intValue2 = nix_alloc_value(ctx, state); - - // `init` and `insert` can be called in any order - nix_init_int(ctx, intValue, 42); - nix_list_builder_insert(ctx, builder, 0, intValue); - nix_list_builder_insert(ctx, builder, 1, intValue2); - nix_init_int(ctx, intValue2, 43); - - nix_make_list(ctx, builder, value); - nix_list_builder_free(builder); - - ASSERT_EQ(42, nix_get_int(ctx, nix_get_list_byidx(ctx, value, state, 0))); - ASSERT_EQ(43, nix_get_int(ctx, nix_get_list_byidx(ctx, value, state, 1))); - ASSERT_EQ(nullptr, nix_get_list_byidx(ctx, value, state, 2)); - ASSERT_EQ(10, nix_get_list_size(ctx, value)); - - ASSERT_STREQ("a list", nix_get_typename(ctx, value)); - ASSERT_EQ(NIX_TYPE_LIST, nix_get_type(ctx, value)); - - // Clean up - nix_gc_decref(ctx, intValue); -} - TEST_F(nix_api_expr_test, nix_build_and_init_attr_invalid) { ASSERT_EQ(nullptr, nix_get_attr_byname(ctx, nullptr, state, 0)); diff --git a/src/libexpr-tests/primops.cc b/src/libexpr-tests/primops.cc index 5b589823798..477f18bf8de 100644 --- a/src/libexpr-tests/primops.cc +++ b/src/libexpr-tests/primops.cc @@ -135,8 +135,8 @@ namespace nix { TEST_F(PrimOpTest, attrValues) { auto v = eval("builtins.attrValues { x = \"foo\"; a = 1; }"); ASSERT_THAT(v, IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[0], IsIntEq(1)); - ASSERT_THAT(*v.listElems()[1], IsStringEq("foo")); + ASSERT_THAT(*v.list()[0], IsIntEq(1)); + ASSERT_THAT(*v.list()[1], IsStringEq("foo")); } TEST_F(PrimOpTest, getAttr) { @@ -235,8 +235,8 @@ namespace nix { TEST_F(PrimOpTest, catAttrs) { auto v = eval("builtins.catAttrs \"a\" [{a = 1;} {b = 0;} {a = 2;}]"); ASSERT_THAT(v, IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[0], IsIntEq(1)); - ASSERT_THAT(*v.listElems()[1], IsIntEq(2)); + ASSERT_THAT(*v.list()[0], IsIntEq(1)); + ASSERT_THAT(*v.list()[1], IsIntEq(2)); } TEST_F(PrimOpTest, functionArgs) { @@ -304,7 +304,7 @@ namespace nix { TEST_F(PrimOpTest, tail) { auto v = eval("builtins.tail [ 3 2 1 0 ]"); ASSERT_THAT(v, IsListOfSize(3)); - for (const auto [n, elem] : enumerate(v.listItems())) + for (const auto [n, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsIntEq(2 - static_cast(n))); } @@ -315,17 +315,17 @@ namespace nix { TEST_F(PrimOpTest, map) { auto v = eval("map (x: \"foo\" + x) [ \"bar\" \"bla\" \"abc\" ]"); ASSERT_THAT(v, IsListOfSize(3)); - auto elem = v.listElems()[0]; + auto elem = v.list()[0]; ASSERT_THAT(*elem, IsThunk()); state.forceValue(*elem, noPos); ASSERT_THAT(*elem, IsStringEq("foobar")); - elem = v.listElems()[1]; + elem = v.list()[1]; ASSERT_THAT(*elem, IsThunk()); state.forceValue(*elem, noPos); ASSERT_THAT(*elem, IsStringEq("foobla")); - elem = v.listElems()[2]; + elem = v.list()[2]; ASSERT_THAT(*elem, IsThunk()); state.forceValue(*elem, noPos); ASSERT_THAT(*elem, IsStringEq("fooabc")); @@ -334,7 +334,7 @@ namespace nix { TEST_F(PrimOpTest, filter) { auto v = eval("builtins.filter (x: x == 2) [ 3 2 3 2 3 2 ]"); ASSERT_THAT(v, IsListOfSize(3)); - for (const auto elem : v.listItems()) + for (const auto elem : v.list()) ASSERT_THAT(*elem, IsIntEq(2)); } @@ -351,7 +351,7 @@ namespace nix { TEST_F(PrimOpTest, concatLists) { auto v = eval("builtins.concatLists [[1 2] [3 4]]"); ASSERT_THAT(v, IsListOfSize(4)); - for (const auto [i, elem] : enumerate(v.listItems())) + for (const auto [i, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsIntEq(static_cast(i)+1)); } @@ -389,7 +389,7 @@ namespace nix { auto v = eval("builtins.genList (x: x + 1) 3"); ASSERT_EQ(v.type(), nList); ASSERT_EQ(v.listSize(), 3); - for (const auto [i, elem] : enumerate(v.listItems())) { + for (const auto [i, elem] : enumerate(v.list())) { ASSERT_THAT(*elem, IsThunk()); state.forceValue(*elem, noPos); ASSERT_THAT(*elem, IsIntEq(static_cast(i)+1)); @@ -402,7 +402,7 @@ namespace nix { ASSERT_EQ(v.listSize(), 6); const std::vector numbers = { 42, 77, 147, 249, 483, 526 }; - for (const auto [n, elem] : enumerate(v.listItems())) + for (const auto [n, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsIntEq(numbers[n])); } @@ -413,17 +413,17 @@ namespace nix { auto right = v.attrs()->get(createSymbol("right")); ASSERT_NE(right, nullptr); ASSERT_THAT(*right->value, IsListOfSize(2)); - ASSERT_THAT(*right->value->listElems()[0], IsIntEq(23)); - ASSERT_THAT(*right->value->listElems()[1], IsIntEq(42)); + ASSERT_THAT(*right->value->list()[0], IsIntEq(23)); + ASSERT_THAT(*right->value->list()[1], IsIntEq(42)); auto wrong = v.attrs()->get(createSymbol("wrong")); ASSERT_NE(wrong, nullptr); ASSERT_EQ(wrong->value->type(), nList); ASSERT_EQ(wrong->value->listSize(), 3); ASSERT_THAT(*wrong->value, IsListOfSize(3)); - ASSERT_THAT(*wrong->value->listElems()[0], IsIntEq(1)); - ASSERT_THAT(*wrong->value->listElems()[1], IsIntEq(9)); - ASSERT_THAT(*wrong->value->listElems()[2], IsIntEq(3)); + ASSERT_THAT(*wrong->value->list()[0], IsIntEq(1)); + ASSERT_THAT(*wrong->value->list()[1], IsIntEq(9)); + ASSERT_THAT(*wrong->value->list()[2], IsIntEq(3)); } TEST_F(PrimOpTest, concatMap) { @@ -432,7 +432,7 @@ namespace nix { ASSERT_EQ(v.listSize(), 6); const std::vector numbers = { 1, 2, 0, 3, 4, 0 }; - for (const auto [n, elem] : enumerate(v.listItems())) + for (const auto [n, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsIntEq(numbers[n])); } @@ -656,7 +656,7 @@ namespace nix { ASSERT_THAT(v, IsListOfSize(4)); const std::vector strings = { "1", "2", "3", "git" }; - for (const auto [n, p] : enumerate(v.listItems())) + for (const auto [n, p] : enumerate(v.list())) ASSERT_THAT(*p, IsStringEq(strings[n])); } @@ -746,12 +746,12 @@ namespace nix { auto v = eval("builtins.split \"(a)b\" \"abc\""); ASSERT_THAT(v, IsListOfSize(3)); - ASSERT_THAT(*v.listElems()[0], IsStringEq("")); + ASSERT_THAT(*v.list()[0], IsStringEq("")); - ASSERT_THAT(*v.listElems()[1], IsListOfSize(1)); - ASSERT_THAT(*v.listElems()[1]->listElems()[0], IsStringEq("a")); + ASSERT_THAT(*v.list()[1], IsListOfSize(1)); + ASSERT_THAT(*v.list()[1]->list()[0], IsStringEq("a")); - ASSERT_THAT(*v.listElems()[2], IsStringEq("c")); + ASSERT_THAT(*v.list()[2], IsStringEq("c")); } TEST_F(PrimOpTest, split2) { @@ -759,17 +759,17 @@ namespace nix { auto v = eval("builtins.split \"([ac])\" \"abc\""); ASSERT_THAT(v, IsListOfSize(5)); - ASSERT_THAT(*v.listElems()[0], IsStringEq("")); + ASSERT_THAT(*v.list()[0], IsStringEq("")); - ASSERT_THAT(*v.listElems()[1], IsListOfSize(1)); - ASSERT_THAT(*v.listElems()[1]->listElems()[0], IsStringEq("a")); + ASSERT_THAT(*v.list()[1], IsListOfSize(1)); + ASSERT_THAT(*v.list()[1]->list()[0], IsStringEq("a")); - ASSERT_THAT(*v.listElems()[2], IsStringEq("b")); + ASSERT_THAT(*v.list()[2], IsStringEq("b")); - ASSERT_THAT(*v.listElems()[3], IsListOfSize(1)); - ASSERT_THAT(*v.listElems()[3]->listElems()[0], IsStringEq("c")); + ASSERT_THAT(*v.list()[3], IsListOfSize(1)); + ASSERT_THAT(*v.list()[3]->list()[0], IsStringEq("c")); - ASSERT_THAT(*v.listElems()[4], IsStringEq("")); + ASSERT_THAT(*v.list()[4], IsStringEq("")); } TEST_F(PrimOpTest, split3) { @@ -777,36 +777,36 @@ namespace nix { ASSERT_THAT(v, IsListOfSize(5)); // First list element - ASSERT_THAT(*v.listElems()[0], IsStringEq("")); + ASSERT_THAT(*v.list()[0], IsStringEq("")); // 2nd list element is a list [ "" null ] - ASSERT_THAT(*v.listElems()[1], IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[1]->listElems()[0], IsStringEq("a")); - ASSERT_THAT(*v.listElems()[1]->listElems()[1], IsNull()); + ASSERT_THAT(*v.list()[1], IsListOfSize(2)); + ASSERT_THAT(*v.list()[1]->list()[0], IsStringEq("a")); + ASSERT_THAT(*v.list()[1]->list()[1], IsNull()); // 3rd element - ASSERT_THAT(*v.listElems()[2], IsStringEq("b")); + ASSERT_THAT(*v.list()[2], IsStringEq("b")); // 4th element is a list: [ null "c" ] - ASSERT_THAT(*v.listElems()[3], IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[3]->listElems()[0], IsNull()); - ASSERT_THAT(*v.listElems()[3]->listElems()[1], IsStringEq("c")); + ASSERT_THAT(*v.list()[3], IsListOfSize(2)); + ASSERT_THAT(*v.list()[3]->list()[0], IsNull()); + ASSERT_THAT(*v.list()[3]->list()[1], IsStringEq("c")); // 5th element is the empty string - ASSERT_THAT(*v.listElems()[4], IsStringEq("")); + ASSERT_THAT(*v.list()[4], IsStringEq("")); } TEST_F(PrimOpTest, split4) { auto v = eval("builtins.split \"([[:upper:]]+)\" \" FOO \""); ASSERT_THAT(v, IsListOfSize(3)); - auto first = v.listElems()[0]; - auto second = v.listElems()[1]; - auto third = v.listElems()[2]; + auto first = v.list()[0]; + auto second = v.list()[1]; + auto third = v.list()[2]; ASSERT_THAT(*first, IsStringEq(" ")); ASSERT_THAT(*second, IsListOfSize(1)); - ASSERT_THAT(*second->listElems()[0], IsStringEq("FOO")); + ASSERT_THAT(*second->list()[0], IsStringEq("FOO")); ASSERT_THAT(*third, IsStringEq(" ")); } @@ -824,14 +824,14 @@ namespace nix { TEST_F(PrimOpTest, match3) { auto v = eval("builtins.match \"a(b)(c)\" \"abc\""); ASSERT_THAT(v, IsListOfSize(2)); - ASSERT_THAT(*v.listElems()[0], IsStringEq("b")); - ASSERT_THAT(*v.listElems()[1], IsStringEq("c")); + ASSERT_THAT(*v.list()[0], IsStringEq("b")); + ASSERT_THAT(*v.list()[1], IsStringEq("c")); } TEST_F(PrimOpTest, match4) { auto v = eval("builtins.match \"[[:space:]]+([[:upper:]]+)[[:space:]]+\" \" FOO \""); ASSERT_THAT(v, IsListOfSize(1)); - ASSERT_THAT(*v.listElems()[0], IsStringEq("FOO")); + ASSERT_THAT(*v.list()[0], IsStringEq("FOO")); } TEST_F(PrimOpTest, match5) { @@ -848,7 +848,7 @@ namespace nix { // ensure that the list is sorted const std::vector expected { "a", "x", "y", "z" }; - for (const auto [n, elem] : enumerate(v.listItems())) + for (const auto [n, elem] : enumerate(v.list())) ASSERT_THAT(*elem, IsStringEq(expected[n])); } diff --git a/src/libexpr-tests/value/print.cc b/src/libexpr-tests/value/print.cc index 43b54503546..93c0881fd7c 100644 --- a/src/libexpr-tests/value/print.cc +++ b/src/libexpr-tests/value/print.cc @@ -79,11 +79,12 @@ TEST_F(ValuePrintingTests, tList) Value vTwo; vTwo.mkInt(2); - auto list = state.buildList(3); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; + // TODO(@connorbaker): Test fails because previously, using ListBuilder and over-allocating would set entries to nullptr. + // That's no longer the case with persistent vectors. + auto list = Value::List({ &vOne, &vTwo, nullptr }); + Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ 1 2 «nullptr» ]"); } @@ -249,12 +250,9 @@ TEST_F(ValuePrintingTests, depthList) Value vNested; vNested.mkAttrs(builder2.finish()); - auto list = state.buildList(3); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; - list.elems[2] = &vNested; + auto list = Value::List({ &vOne, &vTwo, &vNested }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ 1 2 { ... } ]", PrintOptions { .maxDepth = 1 }); test(vList, "[ 1 2 { nested = { ... }; one = 1; two = 2; } ]", PrintOptions { .maxDepth = 2 }); @@ -539,11 +537,11 @@ TEST_F(ValuePrintingTests, ansiColorsList) Value vTwo; vTwo.mkInt(2); - auto list = state.buildList(3); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; + // TODO(@connorbaker): Test fails because previously, using ListBuilder and over-allocating would set entries to nullptr. + // That's no longer the case with persistent vectors. + auto list = Value::List({ &vOne, &vTwo, nullptr }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_CYAN "2" ANSI_NORMAL " " ANSI_MAGENTA "«nullptr»" ANSI_NORMAL " ]", @@ -670,11 +668,9 @@ TEST_F(ValuePrintingTests, ansiColorsListRepeated) Value vEmpty; vEmpty.mkAttrs(emptyBuilder.finish()); - auto list = state.buildList(2); - list.elems[0] = &vEmpty; - list.elems[1] = &vEmpty; + auto list = Value::List({ &vEmpty, &vEmpty }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ { } " ANSI_MAGENTA "«repeated»" ANSI_NORMAL " ]", @@ -690,11 +686,9 @@ TEST_F(ValuePrintingTests, listRepeated) Value vEmpty; vEmpty.mkAttrs(emptyBuilder.finish()); - auto list = state.buildList(2); - list.elems[0] = &vEmpty; - list.elems[1] = &vEmpty; + auto list = Value::List({ &vEmpty, &vEmpty }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ { } «repeated» ]", PrintOptions { }); test(vList, @@ -751,11 +745,9 @@ TEST_F(ValuePrintingTests, ansiColorsListElided) vTwo.mkInt(2); { - auto list = state.buildList(2); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; + auto list = Value::List({ &vOne, &vTwo }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_FAINT "«1 item elided»" ANSI_NORMAL " ]", @@ -769,12 +761,9 @@ TEST_F(ValuePrintingTests, ansiColorsListElided) vThree.mkInt(3); { - auto list = state.buildList(3); - list.elems[0] = &vOne; - list.elems[1] = &vTwo; - list.elems[2] = &vThree; + auto list = Value::List({ &vOne, &vTwo, &vThree }); Value vList; - vList.mkList(list); + vList.mkList(&list); test(vList, "[ " ANSI_CYAN "1" ANSI_NORMAL " " ANSI_FAINT "«2 items elided»" ANSI_NORMAL " ]", diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc index 2f67260c532..8953eb4dc4a 100644 --- a/src/libexpr/attr-path.cc +++ b/src/libexpr/attr-path.cc @@ -95,7 +95,7 @@ std::pair findAlongAttrPath(EvalState & state, const std::strin if (*attrIndex >= v->listSize()) throw AttrPathNotFound("list index %1% in selection path '%2%' is out of range", *attrIndex, attrPath); - v = v->listElems()[*attrIndex]; + v = v->list()[*attrIndex]; pos = noPos; } diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index ea3319f9939..61183011e3c 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -719,7 +719,7 @@ std::vector AttrCursor::getListOfStrings() std::vector res; - for (auto & elem : v.listItems()) + for (auto & elem : v.list()) res.push_back(std::string(root->state.forceStringNoCtx(*elem, noPos, "while evaluating an attribute for caching"))); if (root->db) diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index d5ce238b2ed..bddad661e77 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -5,6 +5,7 @@ #include "eval.hh" #include "eval-error.hh" #include "eval-settings.hh" +#include namespace nix { @@ -24,6 +25,15 @@ 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]] +Value::List * EvalState::allocList() +{ + return new (allocBytes(sizeof(Value::List))) Value::List(); +} + [[gnu::always_inline]] Value * EvalState::allocValue() @@ -98,8 +108,11 @@ void EvalState::forceValue(Value & v, const PosIdx pos) throw; } } - else if (v.isApp()) + else if (v.isApp()) { callFunction(*v.payload.app.left, *v.payload.app.right, v, pos); + } + // TODO(@connorbaker): Somewhere, somehow, an uninitialized value is being forced. + assert(v.isValid()); } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index e21f70553a7..0fa3f339fbb 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -290,7 +290,7 @@ EvalState::EvalState( static_assert(sizeof(Env) <= 16, "environment must be <= 16 bytes"); - vEmptyList.mkList(buildList(0)); + vEmptyList.mkList(allocList()); vNull.mkNull(); vTrue.mkBool(true); vFalse.mkBool(false); @@ -874,13 +874,6 @@ inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval) } } -ListBuilder::ListBuilder(EvalState & state, size_t size) - : size(size) - , elems(size <= 2 ? inlineElems : (Value * *) allocBytes(size * sizeof(Value *))) -{ - state.nrListElems += size; -} - Value * EvalState::getBool(bool b) { return b ? &vTrue : &vFalse; } @@ -1307,9 +1300,15 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) void ExprList::eval(EvalState & state, Env & env, Value & v) { - auto list = state.buildList(elems.size()); - for (const auto & [n, v2] : enumerate(list)) - v2 = elems[n]->maybeThunk(state, env); + auto list = state.allocList(); + // TODO(@connorbaker): Did removing the use of transients here resolve an error? + // auto transientList = list->transient(); + // for (auto & i : elems) + // transientList.push_back(i->maybeThunk(state, env)); + + // *list = transientList.persistent(); + for (auto & i : elems) + *list = list->push_back(i->maybeThunk(state, env)); v.mkList(list); } @@ -1907,40 +1906,26 @@ void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v) void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) { - Value v1; e1->eval(state, env, v1); - Value v2; e2->eval(state, env, v2); - Value * lists[2] = { &v1, &v2 }; - state.concatLists(v, 2, lists, pos, "while evaluating one of the elements to concatenate"); + auto v1 = state.allocValue(); + e1->eval(state, env, *v1); + auto v2 = state.allocValue(); + e2->eval(state, env, *v2); + state.concatLists(v, Value::List({v1, v2}), pos, "while evaluating one of the elements to concatenate"); } -void EvalState::concatLists(Value & v, size_t nrLists, Value * const * lists, const PosIdx pos, std::string_view errorCtx) +void EvalState::concatLists(Value & v, const Value::List lists, const PosIdx pos, std::string_view errorCtx) { nrListConcats++; - Value * nonEmpty = 0; - size_t len = 0; - for (size_t n = 0; n < nrLists; ++n) { - forceList(*lists[n], pos, errorCtx); - auto l = lists[n]->listSize(); - len += l; - if (l) nonEmpty = lists[n]; + // 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) { + forceList(*list, pos, errorCtx); + *newList = *newList + list->list(); } - - if (nonEmpty && len == nonEmpty->listSize()) { - v = *nonEmpty; - return; - } - - auto list = buildList(len); - auto out = list.elems; - for (size_t n = 0, pos = 0; n < nrLists; ++n) { - auto l = lists[n]->listSize(); - if (l) - memcpy(out + pos, lists[n]->listElems(), l * sizeof(Value *)); - pos += l; - } - v.mkList(list); + v.mkList(newList); } @@ -2099,7 +2084,7 @@ void EvalState::forceValueDeep(Value & v) } else if (v.isList()) { - for (auto v2 : v.listItems()) + for (auto v2 : v.list()) recurse(*v2); } }; @@ -2326,7 +2311,7 @@ BackedStringView EvalState::coerceToString( if (v.isList()) { std::string result; - for (auto [n, v2] : enumerate(v.listItems())) { + for (auto [n, v2] : enumerate(v.list())) { try { result += *coerceToString(pos, *v2, context, "while evaluating one element of the list", @@ -2580,7 +2565,7 @@ void EvalState::assertEqValues(Value & v1, Value & v2, const PosIdx pos, std::st } for (size_t n = 0; n < v1.listSize(); ++n) { try { - assertEqValues(*v1.listElems()[n], *v2.listElems()[n], pos, errorCtx); + assertEqValues(*v1.list()[n], *v2.list()[n], pos, errorCtx); } catch (Error & e) { e.addTrace(positions[pos], "while comparing list element %d", n); throw; @@ -2732,7 +2717,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v case nList: if (v1.listSize() != v2.listSize()) return false; for (size_t n = 0; n < v1.listSize(); ++n) - if (!eqValues(*v1.listElems()[n], *v2.listElems()[n], pos, errorCtx)) return false; + if (!eqValues(*v1.list()[n], *v2.list()[n], pos, errorCtx)) return false; return true; case nAttrs: { diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index a1882dded49..6c8058f62b4 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -708,6 +708,7 @@ public: /** * Allocation primitives. */ + inline Value::List * allocList(); inline Value * allocValue(); inline Env & allocEnv(size_t size); @@ -718,11 +719,6 @@ public: return BindingsBuilder(*this, allocBindings(capacity)); } - ListBuilder buildList(size_t size) - { - return ListBuilder(*this, size); - } - /** * Return a boolean `Value *` without allocating. */ @@ -774,7 +770,7 @@ public: const SingleDerivedPath & p, Value & v); - void concatLists(Value & v, size_t nrLists, Value * const * lists, const PosIdx pos, std::string_view errorCtx); + void concatLists(Value & v, const Value::List lists, const PosIdx pos, std::string_view errorCtx); /** * Print statistics, if enabled. @@ -872,7 +868,6 @@ private: friend void prim_split(EvalState & state, const PosIdx pos, Value * * args, Value & v); friend struct Value; - friend class ListBuilder; }; struct DebugTraceStacker { diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 1ac13fcd2b1..cfb3bdae7f7 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -117,7 +117,7 @@ PackageInfo::Outputs PackageInfo::queryOutputs(bool withPaths, bool onlyOutputsT state->forceList(*i->value, i->pos, "while evaluating the 'outputs' attribute of a derivation"); /* For each output... */ - for (auto elem : i->value->listItems()) { + for (auto elem : i->value->list()) { std::string output(state->forceStringNoCtx(*elem, i->pos, "while evaluating the name of an output of a derivation")); if (withPaths) { @@ -159,7 +159,7 @@ PackageInfo::Outputs PackageInfo::queryOutputs(bool withPaths, bool onlyOutputsT /* ^ this shows during `nix-env -i` right under the bad derivation */ if (!outTI->isList()) throw errMsg; Outputs result; - for (auto elem : outTI->listItems()) { + for (auto elem : outTI->list()) { if (elem->type() != nString) throw errMsg; auto out = outputs.find(elem->c_str()); if (out == outputs.end()) throw errMsg; @@ -206,7 +206,7 @@ bool PackageInfo::checkMeta(Value & v) { state->forceValue(v, v.determinePos(noPos)); if (v.type() == nList) { - for (auto elem : v.listItems()) + for (auto elem : v.list()) if (!checkMeta(*elem)) return false; return true; } @@ -400,7 +400,7 @@ static void getDerivations(EvalState & state, Value & vIn, } else if (v.type() == nList) { - for (auto [n, elem] : enumerate(v.listItems())) { + for (auto [n, elem] : enumerate(v.list())) { std::string pathPrefix2 = addToPath(pathPrefix, fmt("%d", n)); if (getDerivation(state, *elem, pathPrefix2, drvs, done, ignoreAssertionFailures)) getDerivations(state, *elem, pathPrefix2, autoArgs, drvs, done, ignoreAssertionFailures); diff --git a/src/libexpr/json-to-value.cc b/src/libexpr/json-to-value.cc index 9ac56541ab9..24430c046fb 100644 --- a/src/libexpr/json-to-value.cc +++ b/src/libexpr/json-to-value.cc @@ -58,9 +58,11 @@ class JSONSax : nlohmann::json_sax { ValueVector values; std::unique_ptr resolve(EvalState & state) override { - auto list = state.buildList(values.size()); - for (const auto & [n, v2] : enumerate(list)) - v2 = values[n]; + auto list = state.allocList(); + auto transientList = list->transient(); + for (auto & i : values) + transientList.push_back(i); + *list = transientList.persistent(); parent->value(state).mkList(list); return std::move(parent); } diff --git a/src/libexpr/meson.build b/src/libexpr/meson.build index 4d8a38b435c..098113c4fed 100644 --- a/src/libexpr/meson.build +++ b/src/libexpr/meson.build @@ -38,6 +38,14 @@ boost = dependency( # put in `deps_other`. deps_other += boost +immer = dependency( + 'Immer', + version : '>=0.8.0', + method : 'cmake', + include_type: 'system', +) +deps_other += immer + nlohmann_json = dependency('nlohmann_json', version : '>= 3.9') deps_public += nlohmann_json @@ -53,6 +61,7 @@ if bdw_gc.found() configdata.set(define_name, define_value) endforeach configdata.set('GC_THREADS', 1) + configdata.set('IMMER_HAS_LIBGC', 1) endif configdata.set('HAVE_BOEHMGC', bdw_gc.found().to_int()) diff --git a/src/libexpr/package.nix b/src/libexpr/package.nix index d97e7f3a82f..a4a9a1f6ef2 100644 --- a/src/libexpr/package.nix +++ b/src/libexpr/package.nix @@ -4,6 +4,7 @@ , bison , flex +, immer , cmake # for resolving toml11 dep , nix-util @@ -76,6 +77,7 @@ mkMesonLibrary (finalAttrs: { # Hack for sake of the dev shell passthru.externalPropagatedBuildInputs = [ boost + immer nlohmann_json ] ++ lib.optional enableGC boehmgc; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 45d9f86ac05..f34ce9f85b9 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -26,6 +26,8 @@ #include #include #include +#include +#include #ifndef _WIN32 # include @@ -35,6 +37,14 @@ namespace nix { +// TODO(@connorbaker): Be careful when using containers from the standard library, as they need to be configured to use +// the Boehm GC's allocator! Look for and replace usages of vector, pair, map, etc. with the GC versions. + +// TODO(@connorbaker): Look for uses of values after they've been added to a Value::List -- immer uses `std::move` to +// avoid copying, but not sure what the implications are. + +// TODO(@connorbaker): Rewrite functions involving lists to make use of persistence -- currently they're largely written +// in a way that duplicates data. /************************************************************* * Miscellaneous @@ -197,10 +207,12 @@ void derivationToValue(EvalState & state, const PosIdx pos, const SourcePath & p }); attrs.alloc(state.sName).mkString(drv.env["name"]); - auto list = state.buildList(drv.outputs.size()); - for (const auto & [i, o] : enumerate(drv.outputs)) { + auto list = state.allocList(); + for (const auto & o : drv.outputs) { mkOutputString(state, attrs, storePath, o); - (list[i] = state.allocValue())->mkString(o.first); + auto outputString = state.allocValue(); + outputString->mkString(o.first); + *list = list->push_back(outputString); } attrs.alloc(state.sOutputs).mkList(list); @@ -402,7 +414,7 @@ void prim_importNative(EvalState & state, const PosIdx pos, Value * * args, Valu void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v) { state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.exec"); - auto elems = args[0]->listElems(); + auto elems = args[0]->list(); auto count = args[0]->listSize(); if (count == 0) state.error("at least one argument to 'exec' required").atPos(pos).debugThrow(); @@ -647,8 +659,8 @@ struct CompareValues return false; } else if (i == v1->listSize()) { return true; - } else if (!state.eqValues(*v1->listElems()[i], *v2->listElems()[i], pos, errorCtx)) { - return (*this)(v1->listElems()[i], v2->listElems()[i], "while comparing two list elements"); + } else if (!state.eqValues(*v1->list()[i], *v2->list()[i], pos, errorCtx)) { + return (*this)(v1->list()[i], v2->list()[i], "while comparing two list elements"); } } default: @@ -664,6 +676,7 @@ struct CompareValues }; +// TODO(@connorbaker): Get rid of ValueList. typedef std::list> ValueList; @@ -690,7 +703,7 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a state.forceList(*startSet->value, noPos, "while evaluating the 'startSet' attribute passed as argument to builtins.genericClosure"); ValueList workSet; - for (auto elem : startSet->value->listItems()) + for (auto elem : startSet->value->list()) workSet.push_back(elem); if (startSet->value->listSize() == 0) { @@ -728,16 +741,18 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a state.forceList(newElements, noPos, "while evaluating the return value of the `operator` passed to builtins.genericClosure"); /* Add the values returned by the operator to the work set. */ - for (auto elem : newElements.listItems()) { + for (auto elem : newElements.list()) { state.forceValue(*elem, noPos); // "while evaluating one one of the elements returned by the `operator` passed to builtins.genericClosure"); workSet.push_back(elem); } } /* Create the result list. */ - auto list = state.buildList(res.size()); - for (const auto & [n, i] : enumerate(res)) - list[n] = i; + auto list = state.allocList(); + auto transientList = list->transient(); + for (const auto & v : res) + transientList.push_back(v); + *list = transientList.persistent(); v.mkList(list); } @@ -1324,7 +1339,7 @@ static void derivationStrictInternal( command-line arguments to the builder. */ else if (i->name == state.sArgs) { state.forceList(*i->value, pos, context_below); - for (auto elem : i->value->listItems()) { + for (auto elem : i->value->list()) { auto s = state.coerceToString(pos, *elem, context, "while evaluating an element of the argument list", true).toOwned(); @@ -1356,7 +1371,7 @@ static void derivationStrictInternal( /* Require ‘outputs’ to be a list of strings. */ state.forceList(*i->value, pos, context_below); Strings ss; - for (auto elem : i->value->listItems()) + for (auto elem : i->value->list()) ss.emplace_back(state.forceStringNoCtx(*elem, pos, context_below)); handleOutputs(ss); } @@ -1832,7 +1847,7 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V LookupPath lookupPath; - for (auto v2 : args[0]->listItems()) { + for (auto v2 : args[0]->list()) { state.forceAttrs(*v2, pos, "while evaluating an element of the list passed to builtins.findFile"); std::string prefix; @@ -2675,14 +2690,23 @@ static void prim_attrNames(EvalState & state, const PosIdx pos, Value * * args, { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.attrNames"); - auto list = state.buildList(args[0]->attrs()->size()); + auto vec = std::vector(); + vec.reserve(args[0]->attrs()->size()); - for (const auto & [n, i] : enumerate(*args[0]->attrs())) - (list[n] = state.allocValue())->mkString(state.symbols[i.name]); + for (const auto & attr : *args[0]->attrs()) { + auto v = state.allocValue(); + v->mkString(state.symbols[attr.name]); + vec.push_back(v); + } - std::sort(list.begin(), list.end(), + std::sort(vec.begin(), vec.end(), [](Value * v1, Value * v2) { return strcmp(v1->c_str(), v2->c_str()) < 0; }); - + + auto list = state.allocList(); + auto transientList = list->transient(); + for (auto & v : vec) + transientList.push_back(v); + *list = transientList.persistent(); v.mkList(list); } @@ -2703,21 +2727,29 @@ static void prim_attrValues(EvalState & state, const PosIdx pos, Value * * args, { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.attrValues"); - auto list = state.buildList(args[0]->attrs()->size()); + auto vec = std::vector(); + vec.reserve(args[0]->attrs()->size()); - for (const auto & [n, i] : enumerate(*args[0]->attrs())) - list[n] = (Value *) &i; + for (const auto & attr: *args[0]->attrs()) + // Extremely gross mis-cast so we can sort by name + vec.push_back((Value *) &attr); - std::sort(list.begin(), list.end(), + std::sort(vec.begin(), vec.end(), [&](Value * v1, Value * v2) { std::string_view s1 = state.symbols[((Attr *) v1)->name], s2 = state.symbols[((Attr *) v2)->name]; return s1 < s2; }); - for (auto & v : list) + // Quickly convert the values to the actual values to hide the shame. + for (auto & v : vec) v = ((Attr *) v)->value; + auto list = state.allocList(); + auto transientList = list->transient(); + for (auto & v : vec) + transientList.push_back(v); + *list = transientList.persistent(); v.mkList(list); } @@ -2872,7 +2904,7 @@ static void prim_removeAttrs(EvalState & state, const PosIdx pos, Value * * args // 64: large enough to fit the attributes of a derivation boost::container::small_vector names; names.reserve(args[1]->listSize()); - for (auto elem : args[1]->listItems()) { + for (auto elem : args[1]->list()) { state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs"); names.emplace_back(state.symbols.create(elem->string_view()), nullptr); } @@ -2918,7 +2950,7 @@ static void prim_listToAttrs(EvalState & state, const PosIdx pos, Value * * args std::set seen; - for (auto v2 : args[0]->listItems()) { + for (auto v2 : args[0]->list()) { state.forceAttrs(*v2, pos, "while evaluating an element of the list passed to builtins.listToAttrs"); auto j = getAttr(state, state.sName, v2->attrs(), "in a {name=...; value=...;} pair"); @@ -3052,15 +3084,18 @@ static void prim_catAttrs(EvalState & state, const PosIdx pos, Value * * args, V SmallValueVector res(args[1]->listSize()); size_t found = 0; - for (auto v2 : args[1]->listItems()) { + for (auto v2 : args[1]->list()) { state.forceAttrs(*v2, pos, "while evaluating an element in the list passed as second argument to builtins.catAttrs"); if (auto i = v2->attrs()->get(attrName)) res[found++] = i->value; } - auto list = state.buildList(found); + auto list = state.allocList(); + auto transientList = list->transient(); for (unsigned int n = 0; n < found; ++n) - list[n] = res[n]; + transientList.push_back(res[n]); + + *list = transientList.persistent(); v.mkList(list); } @@ -3154,52 +3189,36 @@ static RegisterPrimOp primop_mapAttrs({ static void prim_zipAttrsWith(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - // we will first count how many values are present for each given key. - // we then allocate a single attrset and pre-populate it with lists of - // appropriate sizes, stash the pointers to the list elements of each, - // and populate the lists. after that we replace the list in the every - // attribute with the merge function application. this way we need not - // use (slightly slower) temporary storage the GC does not know about. - - struct Item - { - size_t size = 0; - size_t pos = 0; - std::optional list; - }; - - std::map, traceable_allocator>> attrsSeen; + // TODO(@connorbaker): Revisit the template parameters and choice of allocator. + std::map, std::less, traceable_allocator>>> attrsSeen; state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.zipAttrsWith"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.zipAttrsWith"); - const auto listItems = args[1]->listItems(); - for (auto & vElem : listItems) { + for (auto & vElem : args[1]->list()) { state.forceAttrs(*vElem, noPos, "while evaluating a value of the list passed as second argument to builtins.zipAttrsWith"); - for (auto & attr : *vElem->attrs()) - attrsSeen.try_emplace(attr.name).first->second.size++; - } - - for (auto & [sym, elem] : attrsSeen) - elem.list.emplace(state.buildList(elem.size)); - - for (auto & vElem : listItems) { for (auto & attr : *vElem->attrs()) { - auto & item = attrsSeen.at(attr.name); - (*item.list)[item.pos++] = attr.value; + if (!attrsSeen.contains(attr.name)) { + auto list = state.allocList(); + auto transientList = list->transient(); + attrsSeen[attr.name] = {list, transientList}; + } + attrsSeen[attr.name].second.push_back(attr.value); } } auto attrs = state.buildBindings(attrsSeen.size()); - for (auto & [sym, elem] : attrsSeen) { + for (auto & [sym, listPair] : attrsSeen) { auto name = state.allocValue(); name->mkString(state.symbols[sym]); auto call1 = state.allocValue(); call1->mkApp(args[0], name); auto call2 = state.allocValue(); auto arg = state.allocValue(); - arg->mkList(*elem.list); + auto valueList = listPair.second.persistent(); + *listPair.first = valueList; + arg->mkList(listPair.first); call2->mkApp(call1, arg); attrs.insert(sym, call2); } @@ -3269,8 +3288,8 @@ static void elemAt(EvalState & state, const PosIdx pos, Value & list, int n, Val "list index %1% is out of bounds", n ).atPos(pos).debugThrow(); - state.forceValue(*list.listElems()[n], pos); - v = *list.listElems()[n]; + state.forceValue(*list.list()[n], pos); + v = *list.list()[n]; } /* Return the n-1'th element of a list. */ @@ -3316,10 +3335,9 @@ static void prim_tail(EvalState & state, const PosIdx pos, Value * * args, Value if (args[0]->listSize() == 0) state.error("'tail' called on an empty list").atPos(pos).debugThrow(); - auto list = state.buildList(args[0]->listSize() - 1); - for (const auto & [n, v] : enumerate(list)) - v = args[0]->listElems()[n + 1]; - v.mkList(list); + auto tail = state.allocList(); + *tail = args[0]->list().drop(1); + v.mkList(tail); } static RegisterPrimOp primop_tail({ @@ -3350,10 +3368,14 @@ static void prim_map(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.map"); - auto list = state.buildList(args[1]->listSize()); - for (const auto & [n, v] : enumerate(list)) - (v = state.allocValue())->mkApp( - args[0], args[1]->listElems()[n]); + auto list = state.allocList(); + auto transientList = list->transient(); + for (const auto & v : args[1]->list()) { + auto newV = state.allocValue(); + newV->mkApp(args[0], v); + transientList.push_back(newV); + } + *list = transientList.persistent(); v.mkList(list); } @@ -3387,26 +3409,18 @@ static void prim_filter(EvalState & state, const PosIdx pos, Value * * args, Val state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filter"); - SmallValueVector vs(args[1]->listSize()); - size_t k = 0; + auto list = state.allocList(); + auto transientList = list->transient(); - bool same = true; - for (unsigned int n = 0; n < args[1]->listSize(); ++n) { + for (auto arg : args[1]->list()) { Value res; - state.callFunction(*args[0], *args[1]->listElems()[n], res, noPos); + state.callFunction(*args[0], *arg, res, noPos); if (state.forceBool(res, pos, "while evaluating the return value of the filtering function passed to builtins.filter")) - vs[k++] = args[1]->listElems()[n]; - else - same = false; + transientList.push_back(arg); } - if (same) - v = *args[1]; - else { - auto list = state.buildList(k); - for (const auto & [n, v] : enumerate(list)) v = vs[n]; - v.mkList(list); - } + *list = transientList.persistent(); + v.mkList(list); } static RegisterPrimOp primop_filter({ @@ -3424,7 +3438,7 @@ static void prim_elem(EvalState & state, const PosIdx pos, Value * * args, Value { bool res = false; state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.elem"); - for (auto elem : args[1]->listItems()) + for (auto elem : args[1]->list()) if (state.eqValues(*args[0], *elem, pos, "while searching for the presence of the given element in the list")) { res = true; break; @@ -3446,7 +3460,7 @@ static RegisterPrimOp primop_elem({ static void prim_concatLists(EvalState & state, const PosIdx pos, Value * * args, Value & v) { state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.concatLists"); - state.concatLists(v, args[0]->listSize(), args[0]->listElems(), pos, "while evaluating a value of the list passed to builtins.concatLists"); + state.concatLists(v, args[0]->list(), pos, "while evaluating a value of the list passed to builtins.concatLists"); } static RegisterPrimOp primop_concatLists({ @@ -3484,7 +3498,7 @@ static void prim_foldlStrict(EvalState & state, const PosIdx pos, Value * * args if (args[2]->listSize()) { Value * vCur = args[1]; - for (auto [n, elem] : enumerate(args[2]->listItems())) { + for (auto [n, elem] : enumerate(args[2]->list())) { Value * vs []{vCur, elem}; vCur = n == args[2]->listSize() - 1 ? &v : state.allocValue(); state.callFunction(*args[0], 2, vs, *vCur, pos); @@ -3526,7 +3540,7 @@ static void anyOrAll(bool any, EvalState & state, const PosIdx pos, Value * * ar : "while evaluating the return value of the function passed to builtins.all"; Value vTmp; - for (auto elem : args[1]->listItems()) { + for (auto elem : args[1]->list()) { state.callFunction(*args[0], *elem, vTmp, pos); bool res = state.forceBool(vTmp, pos, errorCtx); if (res == any) { @@ -3582,12 +3596,16 @@ static void prim_genList(EvalState & state, const PosIdx pos, Value * * args, Va // as evaluating map without accessing any values makes little sense. state.forceFunction(*args[0], noPos, "while evaluating the first argument passed to builtins.genList"); - auto list = state.buildList(len); - for (const auto & [n, v] : enumerate(list)) { + auto list = state.allocList(); + auto transientList = list->transient(); + for (size_t n = 0; n < len; ++n) { auto arg = state.allocValue(); arg->mkInt(n); - (v = state.allocValue())->mkApp(args[0], arg); + auto app = state.allocValue(); + app->mkApp(args[0], arg); + transientList.push_back(app); } + *list = transientList.persistent(); v.mkList(list); } @@ -3622,9 +3640,13 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.sort"); - auto list = state.buildList(len); - for (const auto & [n, v] : enumerate(list)) - state.forceValue(*(v = args[1]->listElems()[n]), pos); + auto vec = std::vector(); + vec.reserve(len); + + for (const auto & v : args[1]->list()) { + state.forceValue(*v, pos); + vec.push_back(v); + } auto comparator = [&](Value * a, Value * b) { /* Optimization: if the comparator is lessThan, bypass @@ -3644,8 +3666,13 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value /* FIXME: std::sort can segfault if the comparator is not a strict weak ordering. What to do? std::stable_sort() seems more resilient, but no guarantees... */ - std::stable_sort(list.begin(), list.end(), comparator); + std::stable_sort(vec.begin(), vec.end(), comparator); + auto list = state.allocList(); + auto transientList = list->transient(); + for (const auto v : vec) + transientList.push_back(v); + *list = transientList.persistent(); v.mkList(list); } @@ -3675,33 +3702,26 @@ static void prim_partition(EvalState & state, const PosIdx pos, Value * * args, state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.partition"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.partition"); - auto len = args[1]->listSize(); - - ValueVector right, wrong; + auto rlist = state.allocList(); + auto rtransientList = rlist->transient(); + auto wlist = state.allocList(); + auto wtransientList = wlist->transient(); - for (unsigned int n = 0; n < len; ++n) { - auto vElem = args[1]->listElems()[n]; - state.forceValue(*vElem, pos); + for (const auto v : args[1]->list()) { + state.forceValue(*v, pos); Value res; - state.callFunction(*args[0], *vElem, res, pos); + state.callFunction(*args[0], *v, res, pos); if (state.forceBool(res, pos, "while evaluating the return value of the partition function passed to builtins.partition")) - right.push_back(vElem); + rtransientList.push_back(v); else - wrong.push_back(vElem); + wtransientList.push_back(v); } + *rlist = rtransientList.persistent(); + *wlist = wtransientList.persistent(); auto attrs = state.buildBindings(2); - auto rsize = right.size(); - auto rlist = state.buildList(rsize); - if (rsize) - memcpy(rlist.elems, right.data(), sizeof(Value *) * rsize); attrs.alloc(state.sRight).mkList(rlist); - - auto wsize = wrong.size(); - auto wlist = state.buildList(wsize); - if (wsize) - memcpy(wlist.elems, wrong.data(), sizeof(Value *) * wsize); attrs.alloc(state.sWrong).mkList(wlist); v.mkAttrs(attrs); @@ -3735,24 +3755,27 @@ static void prim_groupBy(EvalState & state, const PosIdx pos, Value * * args, Va state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.groupBy"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.groupBy"); - ValueVectorMap attrs; + std::map> attrs; - for (auto vElem : args[1]->listItems()) { + for (auto vElem : args[1]->list()) { Value res; state.callFunction(*args[0], *vElem, res, pos); auto name = state.forceStringNoCtx(res, pos, "while evaluating the return value of the grouping function passed to builtins.groupBy"); auto sym = state.symbols.create(name); - auto vector = attrs.try_emplace(sym, ValueVector()).first; - vector->second.push_back(vElem); + + if (!attrs.contains(sym)) { + auto list = state.allocList(); + auto transientList = list->transient(); + attrs[sym] = {list, transientList}; + } + attrs[sym].second.push_back(vElem); } auto attrs2 = state.buildBindings(attrs.size()); - for (auto & i : attrs) { - auto size = i.second.size(); - auto list = state.buildList(size); - memcpy(list.elems, i.second.data(), sizeof(Value *) * size); - attrs2.alloc(i.first).mkList(list); + for (auto & [name, listPair] : attrs) { + *listPair.first = listPair.second.persistent(); + attrs2.alloc(name).mkList(listPair.first); } v.mkAttrs(attrs2.alreadySorted()); @@ -3786,26 +3809,15 @@ static void prim_concatMap(EvalState & state, const PosIdx pos, Value * * args, { state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.concatMap"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.concatMap"); - auto nrLists = args[1]->listSize(); - // List of returned lists before concatenation. References to these Values must NOT be persisted. - SmallTemporaryValueVector lists(nrLists); - size_t len = 0; + auto list = state.allocList(); - for (unsigned int n = 0; n < nrLists; ++n) { - Value * vElem = args[1]->listElems()[n]; - state.callFunction(*args[0], *vElem, lists[n], pos); - state.forceList(lists[n], lists[n].determinePos(args[0]->determinePos(pos)), "while evaluating the return value of the function passed to builtins.concatMap"); - len += lists[n].listSize(); - } - - auto list = state.buildList(len); - auto out = list.elems; - for (unsigned int n = 0, pos = 0; n < nrLists; ++n) { - auto l = lists[n].listSize(); - if (l) - memcpy(out + pos, lists[n].listElems(), l * sizeof(Value *)); - pos += l; + for (auto arg : args[1]->list()) { + // TODO(@connorbaker): Will the list stored in res be available after the loop, or will it be garbage collected? + Value res; + state.callFunction(*args[0], *arg, res, pos); + state.forceList(res, res.determinePos(args[0]->determinePos(pos)), "while evaluating the return value of the function passed to builtins.concatMap"); + *list = *list + res.list(); } v.mkList(list); } @@ -4292,20 +4304,26 @@ void prim_match(EvalState & state, const PosIdx pos, Value * * args, Value & v) NixStringContext context; const auto str = state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.match"); - std::cmatch match; - if (!std::regex_match(str.begin(), str.end(), match, regex)) { + std::cmatch matches; + if (!std::regex_match(str.begin(), str.end(), matches, regex)) { v.mkNull(); return; } + auto listOfMatches = state.allocList(); + auto transientListOfMatches = listOfMatches->transient(); + // the first match is the whole string - auto list = state.buildList(match.size() - 1); - for (const auto & [i, v2] : enumerate(list)) - if (!match[i + 1].matched) - v2 = &state.vNull; - else - v2 = mkString(state, match[i + 1]); - v.mkList(list); + for (auto match : std::span(matches).subspan(1)) { + if (!match.matched) { + transientListOfMatches.push_back(&state.vNull); + } else { + transientListOfMatches.push_back(mkString(state, match)); + } + } + + *listOfMatches = transientListOfMatches.persistent(); + v.mkList(listOfMatches); } catch (std::regex_error & e) { if (e.code() == std::regex_constants::error_space) { @@ -4374,44 +4392,56 @@ void prim_split(EvalState & state, const PosIdx pos, Value * * args, Value & v) // Any matches results are surrounded by non-matching results. const size_t len = std::distance(begin, end); - auto list = state.buildList(2 * len + 1); - size_t idx = 0; + auto listOfMatches = state.allocList(); + auto transientListOfMatches = listOfMatches->transient(); if (len == 0) { - list[0] = args[1]; - v.mkList(list); + transientListOfMatches.push_back(args[1]); + *listOfMatches = transientListOfMatches.persistent(); + v.mkList(listOfMatches); return; } + size_t idx = 0; + for (auto i = begin; i != end; ++i) { assert(idx <= 2 * len + 1 - 3); auto match = *i; // Add a string for non-matched characters. - list[idx++] = mkString(state, match.prefix()); + transientListOfMatches.push_back(mkString(state, match.prefix())); + idx++; // Add a list for matched substrings. - const size_t slen = match.size() - 1; - - // Start at 1, beacause the first match is the whole string. - auto list2 = state.buildList(slen); - for (const auto & [si, v2] : enumerate(list2)) { - if (!match[si + 1].matched) - v2 = &state.vNull; - else - v2 = mkString(state, match[si + 1]); + auto listOfSubMatches = state.allocList(); + auto transientListOfSubMatches = listOfSubMatches->transient(); + + // NOTE: Start at 1, beacause the first match is the whole string. + for (auto subMatch : std::span(match).subspan(1)) { + if (!subMatch.matched) { + transientListOfSubMatches.push_back(&state.vNull); + } else { + transientListOfSubMatches.push_back(mkString(state, subMatch)); + } } - (list[idx++] = state.allocValue())->mkList(list2); + *listOfSubMatches = transientListOfSubMatches.persistent(); + auto scratchValue = state.allocValue(); + scratchValue->mkList(listOfSubMatches); + transientListOfMatches.push_back(scratchValue); + idx++; // Add a string for non-matched suffix characters. - if (idx == 2 * len) - list[idx++] = mkString(state, match.suffix()); + if (idx == 2 * len) { + transientListOfMatches.push_back(mkString(state, match.suffix())); + idx++; + } } assert(idx == 2 * len + 1); - v.mkList(list); + *listOfMatches = transientListOfMatches.persistent(); + v.mkList(listOfMatches); } catch (std::regex_error & e) { if (e.code() == std::regex_constants::error_space) { @@ -4474,7 +4504,7 @@ static void prim_concatStringsSep(EvalState & state, const PosIdx pos, Value * * res.reserve((args[1]->listSize() + 32) * sep.size()); bool first = true; - for (auto elem : args[1]->listItems()) { + 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"); } @@ -4504,11 +4534,11 @@ static void prim_replaceStrings(EvalState & state, const PosIdx pos, Value * * a std::vector from; from.reserve(args[0]->listSize()); - for (auto elem : args[0]->listItems()) + for (auto elem : args[0]->list()) from.emplace_back(state.forceString(*elem, pos, "while evaluating one of the strings to replace passed to builtins.replaceStrings")); std::unordered_map cache; - auto to = args[1]->listItems(); + auto to = args[1]->list(); NixStringContext context; auto s = state.forceString(*args[2], context, pos, "while evaluating the third argument passed to builtins.replaceStrings"); @@ -4633,9 +4663,15 @@ static void prim_splitVersion(EvalState & state, const PosIdx pos, Value * * arg break; components.emplace_back(component); } - auto list = state.buildList(components.size()); - for (const auto & [n, component] : enumerate(components)) - (list[n] = state.allocValue())->mkString(std::move(component)); + + auto list = state.allocList(); + auto transientList = list->transient(); + for (const auto & component : components) { + auto v = state.allocValue(); + v->mkString(std::move(component)); + transientList.push_back(v); + } + *list = transientList.persistent(); v.mkList(list); } @@ -4878,13 +4914,17 @@ void EvalState::createBaseEnv() }); /* Add a value containing the current Nix expression search path. */ - auto list = buildList(lookupPath.elements.size()); - for (const auto & [n, i] : enumerate(lookupPath.elements)) { + auto list = allocList(); + auto transientList = list->transient(); + for (const auto & i : lookupPath.elements) { auto attrs = buildBindings(2); attrs.alloc("path").mkString(i.path.s); attrs.alloc("prefix").mkString(i.prefix.s); - (list[n] = allocValue())->mkAttrs(attrs); + auto v = allocValue(); + v->mkAttrs(attrs); + transientList.push_back(v); } + *list = transientList.persistent(); v.mkList(list); addConstant("__nixPath", v, { .type = nList, diff --git a/src/libexpr/primops/context.cc b/src/libexpr/primops/context.cc index 02683b173aa..8d8271f4420 100644 --- a/src/libexpr/primops/context.cc +++ b/src/libexpr/primops/context.cc @@ -218,9 +218,14 @@ static void prim_getContext(EvalState & state, const PosIdx pos, Value * * args, if (info.second.allOutputs) infoAttrs.alloc(sAllOutputs).mkBool(true); if (!info.second.outputs.empty()) { - auto list = state.buildList(info.second.outputs.size()); - for (const auto & [i, output] : enumerate(info.second.outputs)) - (list[i] = state.allocValue())->mkString(output); + auto list = state.allocList(); + auto transientList = list->transient(); + for (const auto & [i, output] : enumerate(info.second.outputs)) { + auto v = state.allocValue(); + v->mkString(output); + transientList.push_back(v); + } + *list = transientList.persistent(); infoAttrs.alloc(state.sOutputs).mkList(list); } attrs.alloc(state.store->printStorePath(info.first)).mkAttrs(infoAttrs); @@ -310,7 +315,7 @@ static void prim_appendContext(EvalState & state, const PosIdx pos, Value * * ar name ).atPos(i.pos).debugThrow(); } - for (auto elem : attr->value->listItems()) { + for (auto elem : attr->value->list()) { auto outputName = state.forceStringNoCtx(*elem, attr->pos, "while evaluating an output name within a string context"); context.emplace(NixStringContextElem::Built { .drvPath = makeConstantStorePathRef(namePath), diff --git a/src/libexpr/primops/fromTOML.cc b/src/libexpr/primops/fromTOML.cc index 264046711a6..ed8e578d8ca 100644 --- a/src/libexpr/primops/fromTOML.cc +++ b/src/libexpr/primops/fromTOML.cc @@ -38,9 +38,14 @@ static void prim_fromTOML(EvalState & state, const PosIdx pos, Value * * args, V { auto array = toml::get>(t); - auto list = state.buildList(array.size()); - for (const auto & [n, v] : enumerate(list)) - visit(*(v = state.allocValue()), array[n]); + auto list = state.allocList(); + auto transientList = list->transient(); + for (const auto & tomlValue :array) { + auto v = state.allocValue(); + visit(*v, tomlValue); + transientList.push_back(v); + } + *list = transientList.persistent(); v.mkList(list); } break;; diff --git a/src/libexpr/print-ambiguous.cc b/src/libexpr/print-ambiguous.cc index a40c98643e3..df4464a52a2 100644 --- a/src/libexpr/print-ambiguous.cc +++ b/src/libexpr/print-ambiguous.cc @@ -50,11 +50,23 @@ void printAmbiguous( break; } case nList: - if (seen && v.listSize() && !seen->insert(v.listElems()).second) + // TODO(@connorbaker): Accessing payload directly is gross, but we need a single void* + // to insert into the set, and the way immer's flex_vector checks for equality yields + // a tuple of void*. + // TODO(@connorbaker): This currently fails the testcase + // $ nix-instantiate --eval --strict ./tests/functional/lang/eval-okay-print.nix + // [ null [ «repeated» ] ] + // It should yield: + // [ null [ [ «repeated» ] ] ] + // Oddly enough, + // $ nix eval --expr "$(cat ./tests/functional/lang/eval-okay-print.nix)" + // [ null «primop toString» «partially applied primop deepSeq» «lambda @ «string»:1:61» [ [ «repeated» ] ] ] + // So it's likely a bug in the path used by `nix-instantiate`. + if (seen && v.listSize() && !seen->insert(v.payload.list).second) str << "«repeated»"; else { str << "[ "; - for (auto v2 : v.listItems()) { + for (auto v2 : v.list()) { if (v2) printAmbiguous(*v2, symbols, str, seen, depth - 1); else diff --git a/src/libexpr/print-options.hh b/src/libexpr/print-options.hh index 080ba26b86c..6941126c586 100644 --- a/src/libexpr/print-options.hh +++ b/src/libexpr/print-options.hh @@ -4,6 +4,7 @@ * @brief Options for printing Nix values. */ +#include #include namespace nix { diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index d62aaf25f78..d8c0056e20c 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -380,7 +380,7 @@ class Printer /** * @note This may force items. */ - bool shouldPrettyPrintList(std::span list) + bool shouldPrettyPrintList(const Value::List list) { if (!options.shouldPrettyPrint() || list.empty()) { return false; @@ -415,16 +415,16 @@ class Printer if (depth < options.maxDepth) { increaseIndent(); output << "["; - auto listItems = v.listItems(); - auto prettyPrint = shouldPrettyPrintList(listItems); + auto list = v.list(); + auto prettyPrint = shouldPrettyPrintList(list); size_t currentListItemsPrinted = 0; - for (auto elem : listItems) { + for (auto elem : list) { printSpace(prettyPrint); if (totalListItemsPrinted >= options.maxListItems) { - printElided(listItems.size() - currentListItemsPrinted, "item", "items"); + printElided(list.size() - currentListItemsPrinted, "item", "items"); break; } diff --git a/src/libexpr/value-to-json.cc b/src/libexpr/value-to-json.cc index 8044fe3472e..e19977f7115 100644 --- a/src/libexpr/value-to-json.cc +++ b/src/libexpr/value-to-json.cc @@ -72,7 +72,7 @@ json printValueAsJSON(EvalState & state, bool strict, case nList: { out = json::array(); int i = 0; - for (auto elem : v.listItems()) { + for (auto elem : v.list()) { try { out.push_back(printValueAsJSON(state, strict, *elem, pos, context, copyToStore)); } catch (Error & e) { diff --git a/src/libexpr/value-to-xml.cc b/src/libexpr/value-to-xml.cc index 9734ebec498..75ff433e820 100644 --- a/src/libexpr/value-to-xml.cc +++ b/src/libexpr/value-to-xml.cc @@ -114,7 +114,7 @@ static void printValueAsXML(EvalState & state, bool strict, bool location, case nList: { XMLOpenElement _(doc, "list"); - for (auto v2 : v.listItems()) + for (auto v2 : v.list()) printValueAsXML(state, strict, location, *v2, doc, context, drvsSeen, pos); break; } diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index d9816148852..600f46dd78a 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -13,6 +13,21 @@ #include +#include +#include +#include +#include +#include +#include + +// declare a memory policy for using a tracing garbage collector +using gc_policy = immer::memory_policy, + immer::no_refcount_policy, + immer::default_lock_policy, + immer::gc_transience_policy, + false, + false>; + namespace nix { struct Value; @@ -27,9 +42,7 @@ typedef enum { tPath, tNull, tAttrs, - tList1, - tList2, - tListN, + tList, tThunk, tApp, tLambda, @@ -133,36 +146,6 @@ class ExternalValueBase std::ostream & operator << (std::ostream & str, const ExternalValueBase & v); -class ListBuilder -{ - const size_t size; - Value * inlineElems[2] = {nullptr, nullptr}; -public: - Value * * elems; - ListBuilder(EvalState & state, size_t size); - - // NOTE: Can be noexcept because we are just copying integral values and - // raw pointers. - ListBuilder(ListBuilder && x) noexcept - : size(x.size) - , inlineElems{x.inlineElems[0], x.inlineElems[1]} - , elems(size <= 2 ? inlineElems : x.elems) - { } - - Value * & operator [](size_t n) - { - return elems[n]; - } - - typedef Value * * iterator; - - iterator begin() { return &elems[0]; } - iterator end() { return &elems[size]; } - - friend struct Value; -}; - - struct Value { private: @@ -234,6 +217,11 @@ public: ExprLambda * fun; }; + // alias the vector type so we are not concerned about memory policies + // in the places where we actually use it + using List = immer::flex_vector; + using TransientList = immer::flex_vector_transient; + using Payload = union { NixInt integer; @@ -244,11 +232,7 @@ public: Path path; Bindings * attrs; - struct { - size_t size; - Value * const * elems; - } bigList; - Value * smallList[2]; + List * list; ClosureThunk thunk; FunctionApplicationThunk app; Lambda lambda; @@ -277,7 +261,7 @@ public: case tPath: return nPath; case tNull: return nNull; case tAttrs: return nAttrs; - case tList1: case tList2: case tListN: return nList; + case tList: return nList; case tLambda: case tPrimOp: case tPrimOpApp: return nFunction; case tExternal: return nExternal; case tFloat: return nFloat; @@ -286,6 +270,7 @@ public: if (invalidIsThunk) return nThunk; else + printError("Value::type: invalid internal type: %d", internalType); unreachable(); } @@ -356,14 +341,9 @@ public: Value & mkAttrs(BindingsBuilder & bindings); - void mkList(const ListBuilder & builder) + void mkList(List * list) { - if (builder.size == 1) - finishValue(tList1, { .smallList = { builder.inlineElems[0] } }); - else if (builder.size == 2) - finishValue(tList2, { .smallList = { builder.inlineElems[0], builder.inlineElems[1] } }); - else - finishValue(tListN, { .bigList = { .size = builder.size, .elems = builder.elems } }); + finishValue(tList, { .list = list }); } inline void mkThunk(Env * e, Expr * ex) @@ -407,28 +387,12 @@ public: bool isList() const { - return internalType == tList1 || internalType == tList2 || internalType == tListN; - } - - Value * const * listElems() - { - return internalType == tList1 || internalType == tList2 ? payload.smallList : payload.bigList.elems; - } - - std::span listItems() const - { - assert(isList()); - return std::span(listElems(), listSize()); - } - - Value * const * listElems() const - { - return internalType == tList1 || internalType == tList2 ? payload.smallList : payload.bigList.elems; + return internalType == tList; } size_t listSize() const { - return internalType == tList1 ? 1 : internalType == tList2 ? 2 : payload.bigList.size; + return payload.list->size(); } PosIdx determinePos(const PosIdx pos) const; @@ -471,6 +435,9 @@ public: const Bindings * attrs() const { return payload.attrs; } + List list() const + { return *payload.list; } + const PrimOp * primOp() const { return payload.primOp; } diff --git a/src/libflake/flake/flake.cc b/src/libflake/flake/flake.cc index edb76f86154..7a5eff6bbc5 100644 --- a/src/libflake/flake/flake.cc +++ b/src/libflake/flake/flake.cc @@ -287,7 +287,7 @@ static Flake readFlake( Explicit { state.forceBool(*setting.value, setting.pos, "") }); else if (setting.value->type() == nList) { std::vector ss; - for (auto elem : setting.value->listItems()) { + for (auto elem : setting.value->list()) { if (elem->type() != nString) state.error("list element in flake configuration setting '%s' is %s while a string is expected", state.symbols[setting.name], showType(*setting.value)).debugThrow(); diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index ba2baccee85..d81bb19c51a 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -1251,7 +1251,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs) } else if (v->type() == nList) { attrs2["type"] = "strings"; XMLOpenElement m(xml, "meta", attrs2); - for (auto elem : v->listItems()) { + for (auto elem : v->list()) { if (elem->type() != nString) continue; XMLAttrs attrs3; attrs3["value"] = elem->c_str(); diff --git a/src/nix-env/user-env.cc b/src/nix-env/user-env.cc index ee62077c0a7..c8f4720dd35 100644 --- a/src/nix-env/user-env.cc +++ b/src/nix-env/user-env.cc @@ -50,7 +50,8 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, /* Construct the whole top level derivation. */ StorePathSet references; - auto list = state.buildList(elems.size()); + auto list = state.allocList(); + auto listTransient = list->transient(); for (const auto & [n, i] : enumerate(elems)) { /* Create a pseudo-derivation containing the name, system, output paths, and optionally the derivation path, as well @@ -71,9 +72,12 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, attrs.alloc(state.sDrvPath).mkString(state.store->printStorePath(*drvPath)); // Copy each output meant for installation. - auto outputsList = state.buildList(outputs.size()); - for (const auto & [m, j] : enumerate(outputs)) { - (outputsList[m] = state.allocValue())->mkString(j.first); + auto outputsList = state.allocList(); + auto outputsListTransient = outputsList->transient(); + for (const auto & j : outputs) { + auto v = state.allocValue(); + v->mkString(j.first); + outputsListTransient.push_back(v); auto outputAttrs = state.buildBindings(2); outputAttrs.alloc(state.sOutPath).mkString(state.store->printStorePath(*j.second)); attrs.alloc(j.first).mkAttrs(outputAttrs); @@ -85,8 +89,10 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, references.insert(*j.second); } + *outputsList = outputsListTransient.persistent(); attrs.alloc(state.sOutputs).mkList(outputsList); + // Copy the meta attributes. auto meta = state.buildBindings(metaNames.size()); for (auto & j : metaNames) { @@ -96,11 +102,13 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, } attrs.alloc(state.sMeta).mkAttrs(meta); - - (list[n] = state.allocValue())->mkAttrs(attrs); + auto v = state.allocValue(); + v->mkAttrs(attrs); + listTransient.push_back(v); if (drvPath) references.insert(*drvPath); } + *list = listTransient.persistent(); Value manifest; manifest.mkList(list); diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc index db7d9e4efe6..67aee9d4c39 100644 --- a/src/nix/prefetch.cc +++ b/src/nix/prefetch.cc @@ -44,7 +44,7 @@ std::string resolveMirrorUrl(EvalState & state, const std::string & url) if (mirrorList->value->listSize() < 1) throw Error("mirror URL '%s' did not expand to anything", url); - std::string mirror(state.forceString(*mirrorList->value->listElems()[0], noPos, "while evaluating the first available mirror")); + std::string mirror(state.forceString(*mirrorList->value->list()[0], noPos, "while evaluating the first available mirror")); return mirror + (hasSuffix(mirror, "/") ? "" : "/") + s.substr(p + 1); } @@ -222,7 +222,7 @@ static int main_nix_prefetch_url(int argc, char * * argv) state->forceList(*attr->value, noPos, "while evaluating the urls to prefetch"); if (attr->value->listSize() < 1) throw Error("'urls' list is empty"); - url = state->forceString(*attr->value->listElems()[0], noPos, "while evaluating the first url from the urls list"); + url = state->forceString(*attr->value->list()[0], noPos, "while evaluating the first url from the urls list"); /* Extract the hash mode. */ auto attr2 = v.attrs()->get(state->symbols.create("outputHashMode")); From cf3ba2a34898160d04b372734bee0cff388a928e Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Wed, 30 Oct 2024 05:19:45 +0000 Subject: [PATCH 2/9] remove transients for now; all tests pass --- src/libexpr-tests/value/print.cc | 14 +-- src/libexpr/eval-inline.hh | 4 +- src/libexpr/eval.cc | 10 +-- src/libexpr/eval.hh | 4 +- src/libexpr/json-to-value.cc | 4 +- src/libexpr/primops.cc | 149 +++++++++++-------------------- src/libexpr/primops/context.cc | 4 +- src/libexpr/primops/fromTOML.cc | 4 +- src/libexpr/print-ambiguous.cc | 14 +-- src/libexpr/print.cc | 2 +- src/libexpr/value.hh | 16 ++-- src/nix-env/user-env.cc | 8 +- 12 files changed, 77 insertions(+), 156 deletions(-) diff --git a/src/libexpr-tests/value/print.cc b/src/libexpr-tests/value/print.cc index 93c0881fd7c..9a6c95ad361 100644 --- a/src/libexpr-tests/value/print.cc +++ b/src/libexpr-tests/value/print.cc @@ -81,7 +81,7 @@ TEST_F(ValuePrintingTests, tList) // TODO(@connorbaker): Test fails because previously, using ListBuilder and over-allocating would set entries to nullptr. // That's no longer the case with persistent vectors. - auto list = Value::List({ &vOne, &vTwo, nullptr }); + auto list = ValueList({ &vOne, &vTwo, nullptr }); Value vList; vList.mkList(&list); @@ -250,7 +250,7 @@ TEST_F(ValuePrintingTests, depthList) Value vNested; vNested.mkAttrs(builder2.finish()); - auto list = Value::List({ &vOne, &vTwo, &vNested }); + auto list = ValueList({ &vOne, &vTwo, &vNested }); Value vList; vList.mkList(&list); @@ -539,7 +539,7 @@ TEST_F(ValuePrintingTests, ansiColorsList) // TODO(@connorbaker): Test fails because previously, using ListBuilder and over-allocating would set entries to nullptr. // That's no longer the case with persistent vectors. - auto list = Value::List({ &vOne, &vTwo, nullptr }); + auto list = ValueList({ &vOne, &vTwo, nullptr }); Value vList; vList.mkList(&list); @@ -668,7 +668,7 @@ TEST_F(ValuePrintingTests, ansiColorsListRepeated) Value vEmpty; vEmpty.mkAttrs(emptyBuilder.finish()); - auto list = Value::List({ &vEmpty, &vEmpty }); + auto list = ValueList({ &vEmpty, &vEmpty }); Value vList; vList.mkList(&list); @@ -686,7 +686,7 @@ TEST_F(ValuePrintingTests, listRepeated) Value vEmpty; vEmpty.mkAttrs(emptyBuilder.finish()); - auto list = Value::List({ &vEmpty, &vEmpty }); + auto list = ValueList({ &vEmpty, &vEmpty }); Value vList; vList.mkList(&list); @@ -745,7 +745,7 @@ TEST_F(ValuePrintingTests, ansiColorsListElided) vTwo.mkInt(2); { - auto list = Value::List({ &vOne, &vTwo }); + auto list = ValueList({ &vOne, &vTwo }); Value vList; vList.mkList(&list); @@ -761,7 +761,7 @@ TEST_F(ValuePrintingTests, ansiColorsListElided) vThree.mkInt(3); { - auto list = Value::List({ &vOne, &vTwo, &vThree }); + auto list = ValueList({ &vOne, &vTwo, &vThree }); Value vList; vList.mkList(&list); diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index bddad661e77..3bbe0ad8862 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -29,9 +29,9 @@ inline void * allocBytes(size_t n) capacity. The space is implicitly reserved after the Bindings structure. */ [[gnu::always_inline]] -Value::List * EvalState::allocList() +ValueList * EvalState::allocList() { - return new (allocBytes(sizeof(Value::List))) Value::List(); + return new (allocBytes(sizeof(ValueList))) ValueList(); } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 0fa3f339fbb..007c9787fd0 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1301,12 +1301,6 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) void ExprList::eval(EvalState & state, Env & env, Value & v) { auto list = state.allocList(); - // TODO(@connorbaker): Did removing the use of transients here resolve an error? - // auto transientList = list->transient(); - // for (auto & i : elems) - // transientList.push_back(i->maybeThunk(state, env)); - - // *list = transientList.persistent(); for (auto & i : elems) *list = list->push_back(i->maybeThunk(state, env)); v.mkList(list); @@ -1910,11 +1904,11 @@ void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) e1->eval(state, env, *v1); auto v2 = state.allocValue(); e2->eval(state, env, *v2); - state.concatLists(v, Value::List({v1, v2}), pos, "while evaluating one of the elements to concatenate"); + state.concatLists(v, ValueList({v1, v2}), pos, "while evaluating one of the elements to concatenate"); } -void EvalState::concatLists(Value & v, const Value::List lists, const PosIdx pos, std::string_view errorCtx) +void EvalState::concatLists(Value & v, const ValueList lists, const PosIdx pos, std::string_view errorCtx) { nrListConcats++; diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 6c8058f62b4..e49aea98856 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -708,7 +708,7 @@ public: /** * Allocation primitives. */ - inline Value::List * allocList(); + inline ValueList * allocList(); inline Value * allocValue(); inline Env & allocEnv(size_t size); @@ -770,7 +770,7 @@ public: const SingleDerivedPath & p, Value & v); - void concatLists(Value & v, const Value::List lists, const PosIdx pos, std::string_view errorCtx); + void concatLists(Value & v, const ValueList lists, const PosIdx pos, std::string_view errorCtx); /** * Print statistics, if enabled. diff --git a/src/libexpr/json-to-value.cc b/src/libexpr/json-to-value.cc index 24430c046fb..a4b13921d25 100644 --- a/src/libexpr/json-to-value.cc +++ b/src/libexpr/json-to-value.cc @@ -59,10 +59,8 @@ class JSONSax : nlohmann::json_sax { std::unique_ptr resolve(EvalState & state) override { auto list = state.allocList(); - auto transientList = list->transient(); for (auto & i : values) - transientList.push_back(i); - *list = transientList.persistent(); + *list = list->push_back(i); parent->value(state).mkList(list); return std::move(parent); } diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index f34ce9f85b9..05e54ea18a5 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -40,7 +40,7 @@ namespace nix { // TODO(@connorbaker): Be careful when using containers from the standard library, as they need to be configured to use // the Boehm GC's allocator! Look for and replace usages of vector, pair, map, etc. with the GC versions. -// TODO(@connorbaker): Look for uses of values after they've been added to a Value::List -- immer uses `std::move` to +// TODO(@connorbaker): Look for uses of values after they've been added to a ValueList -- immer uses `std::move` to // avoid copying, but not sure what the implications are. // TODO(@connorbaker): Rewrite functions involving lists to make use of persistence -- currently they're largely written @@ -675,11 +675,6 @@ struct CompareValues } }; - -// TODO(@connorbaker): Get rid of ValueList. -typedef std::list> ValueList; - - static Bindings::const_iterator getAttr( EvalState & state, Symbol attrSym, @@ -702,9 +697,9 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a state.forceList(*startSet->value, noPos, "while evaluating the 'startSet' attribute passed as argument to builtins.genericClosure"); - ValueList workSet; + auto workSet = state.allocList(); for (auto elem : startSet->value->list()) - workSet.push_back(elem); + *workSet = workSet->push_back(elem); if (startSet->value->listSize() == 0) { v = *startSet->value; @@ -718,14 +713,14 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a /* Construct the closure by applying the operator to elements of `workSet', adding the result to `workSet', continuing until no new elements are found. */ - ValueList res; + auto res = state.allocList(); // `doneKeys' doesn't need to be a GC root, because its values are // reachable from res. auto cmp = CompareValues(state, noPos, "while comparing the `key` attributes of two genericClosure elements"); - std::set doneKeys(cmp); - while (!workSet.empty()) { - Value * e = *(workSet.begin()); - workSet.pop_front(); + std::set> doneKeys(cmp); + while (!workSet->empty()) { + Value * e = workSet->front(); + *workSet = workSet->drop(1); state.forceAttrs(*e, noPos, "while evaluating one of the elements generated by (or initially passed to) builtins.genericClosure"); @@ -733,7 +728,7 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a state.forceValue(*key->value, noPos); if (!doneKeys.insert(key->value).second) continue; - res.push_back(e); + *res = res->push_back(e); /* Call the `operator' function with `e' as argument. */ Value newElements; @@ -743,17 +738,11 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a /* Add the values returned by the operator to the work set. */ for (auto elem : newElements.list()) { state.forceValue(*elem, noPos); // "while evaluating one one of the elements returned by the `operator` passed to builtins.genericClosure"); - workSet.push_back(elem); + *workSet = workSet->push_back(elem); } } - /* Create the result list. */ - auto list = state.allocList(); - auto transientList = list->transient(); - for (const auto & v : res) - transientList.push_back(v); - *list = transientList.persistent(); - v.mkList(list); + v.mkList(res); } static RegisterPrimOp primop_genericClosure(PrimOp { @@ -2690,7 +2679,7 @@ static void prim_attrNames(EvalState & state, const PosIdx pos, Value * * args, { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.attrNames"); - auto vec = std::vector(); + ValueVector vec; vec.reserve(args[0]->attrs()->size()); for (const auto & attr : *args[0]->attrs()) { @@ -2703,10 +2692,8 @@ static void prim_attrNames(EvalState & state, const PosIdx pos, Value * * args, [](Value * v1, Value * v2) { return strcmp(v1->c_str(), v2->c_str()) < 0; }); auto list = state.allocList(); - auto transientList = list->transient(); for (auto & v : vec) - transientList.push_back(v); - *list = transientList.persistent(); + *list = list->push_back(v); v.mkList(list); } @@ -2727,7 +2714,7 @@ static void prim_attrValues(EvalState & state, const PosIdx pos, Value * * args, { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.attrValues"); - auto vec = std::vector(); + ValueVector vec; vec.reserve(args[0]->attrs()->size()); for (const auto & attr: *args[0]->attrs()) @@ -2746,10 +2733,8 @@ static void prim_attrValues(EvalState & state, const PosIdx pos, Value * * args, v = ((Attr *) v)->value; auto list = state.allocList(); - auto transientList = list->transient(); for (auto & v : vec) - transientList.push_back(v); - *list = transientList.persistent(); + *list = list->push_back(v); v.mkList(list); } @@ -2948,7 +2933,7 @@ static void prim_listToAttrs(EvalState & state, const PosIdx pos, Value * * args auto attrs = state.buildBindings(args[0]->listSize()); - std::set seen; + std::set, traceable_allocator> seen; for (auto v2 : args[0]->list()) { state.forceAttrs(*v2, pos, "while evaluating an element of the list passed to builtins.listToAttrs"); @@ -3091,11 +3076,9 @@ static void prim_catAttrs(EvalState & state, const PosIdx pos, Value * * args, V } auto list = state.allocList(); - auto transientList = list->transient(); for (unsigned int n = 0; n < found; ++n) - transientList.push_back(res[n]); + *list = list->push_back(res[n]); - *list = transientList.persistent(); v.mkList(list); } @@ -3189,8 +3172,7 @@ static RegisterPrimOp primop_mapAttrs({ static void prim_zipAttrsWith(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - // TODO(@connorbaker): Revisit the template parameters and choice of allocator. - std::map, std::less, traceable_allocator>>> attrsSeen; + ValueVectorMap attrsSeen; state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.zipAttrsWith"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.zipAttrsWith"); @@ -3199,26 +3181,24 @@ static void prim_zipAttrsWith(EvalState & state, const PosIdx pos, Value * * arg state.forceAttrs(*vElem, noPos, "while evaluating a value of the list passed as second argument to builtins.zipAttrsWith"); for (auto & attr : *vElem->attrs()) { if (!attrsSeen.contains(attr.name)) { - auto list = state.allocList(); - auto transientList = list->transient(); - attrsSeen[attr.name] = {list, transientList}; + attrsSeen[attr.name] = ValueVector(); } - attrsSeen[attr.name].second.push_back(attr.value); + attrsSeen[attr.name].push_back(attr.value); } } auto attrs = state.buildBindings(attrsSeen.size()); - for (auto & [sym, listPair] : attrsSeen) { + for (auto & [sym, valueVector] : attrsSeen) { auto name = state.allocValue(); name->mkString(state.symbols[sym]); auto call1 = state.allocValue(); call1->mkApp(args[0], name); auto call2 = state.allocValue(); auto arg = state.allocValue(); - auto valueList = listPair.second.persistent(); - *listPair.first = valueList; - arg->mkList(listPair.first); + auto list = state.allocList(); + for (auto v : valueVector) *list = list->push_back(v); + arg->mkList(list); call2->mkApp(call1, arg); attrs.insert(sym, call2); } @@ -3369,13 +3349,11 @@ static void prim_map(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.map"); auto list = state.allocList(); - auto transientList = list->transient(); for (const auto & v : args[1]->list()) { auto newV = state.allocValue(); newV->mkApp(args[0], v); - transientList.push_back(newV); + *list = list->push_back(newV); } - *list = transientList.persistent(); v.mkList(list); } @@ -3410,16 +3388,12 @@ static void prim_filter(EvalState & state, const PosIdx pos, Value * * args, Val state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filter"); auto list = state.allocList(); - auto transientList = list->transient(); - for (auto arg : args[1]->list()) { Value res; state.callFunction(*args[0], *arg, res, noPos); if (state.forceBool(res, pos, "while evaluating the return value of the filtering function passed to builtins.filter")) - transientList.push_back(arg); + *list = list->push_back(arg); } - - *list = transientList.persistent(); v.mkList(list); } @@ -3597,15 +3571,13 @@ static void prim_genList(EvalState & state, const PosIdx pos, Value * * args, Va state.forceFunction(*args[0], noPos, "while evaluating the first argument passed to builtins.genList"); auto list = state.allocList(); - auto transientList = list->transient(); for (size_t n = 0; n < len; ++n) { auto arg = state.allocValue(); arg->mkInt(n); auto app = state.allocValue(); app->mkApp(args[0], arg); - transientList.push_back(app); + *list = list->push_back(app); } - *list = transientList.persistent(); v.mkList(list); } @@ -3640,7 +3612,7 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.sort"); - auto vec = std::vector(); + ValueVector vec; vec.reserve(len); for (const auto & v : args[1]->list()) { @@ -3669,10 +3641,8 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value std::stable_sort(vec.begin(), vec.end(), comparator); auto list = state.allocList(); - auto transientList = list->transient(); for (const auto v : vec) - transientList.push_back(v); - *list = transientList.persistent(); + *list = list->push_back(v); v.mkList(list); } @@ -3703,21 +3673,17 @@ static void prim_partition(EvalState & state, const PosIdx pos, Value * * args, state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.partition"); auto rlist = state.allocList(); - auto rtransientList = rlist->transient(); auto wlist = state.allocList(); - auto wtransientList = wlist->transient(); for (const auto v : args[1]->list()) { state.forceValue(*v, pos); Value res; state.callFunction(*args[0], *v, res, pos); if (state.forceBool(res, pos, "while evaluating the return value of the partition function passed to builtins.partition")) - rtransientList.push_back(v); + *rlist = rlist->push_back(v); else - wtransientList.push_back(v); + *wlist = wlist->push_back(v); } - *rlist = rtransientList.persistent(); - *wlist = wtransientList.persistent(); auto attrs = state.buildBindings(2); @@ -3755,7 +3721,7 @@ static void prim_groupBy(EvalState & state, const PosIdx pos, Value * * args, Va state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.groupBy"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.groupBy"); - std::map> attrs; + ValueVectorMap attrsSeen; for (auto vElem : args[1]->list()) { Value res; @@ -3763,19 +3729,18 @@ static void prim_groupBy(EvalState & state, const PosIdx pos, Value * * args, Va auto name = state.forceStringNoCtx(res, pos, "while evaluating the return value of the grouping function passed to builtins.groupBy"); auto sym = state.symbols.create(name); - if (!attrs.contains(sym)) { - auto list = state.allocList(); - auto transientList = list->transient(); - attrs[sym] = {list, transientList}; - } - attrs[sym].second.push_back(vElem); + if (!attrsSeen.contains(sym)) { + attrsSeen[sym] = ValueVector(); + } + attrsSeen[sym].push_back(vElem); } - auto attrs2 = state.buildBindings(attrs.size()); + auto attrs2 = state.buildBindings(attrsSeen.size()); - for (auto & [name, listPair] : attrs) { - *listPair.first = listPair.second.persistent(); - attrs2.alloc(name).mkList(listPair.first); + for (auto & [name, valueVector] : attrsSeen) { + auto list = state.allocList(); + for (auto v : valueVector) *list = list->push_back(v); + attrs2.alloc(name).mkList(list); } v.mkAttrs(attrs2.alreadySorted()); @@ -4311,18 +4276,15 @@ void prim_match(EvalState & state, const PosIdx pos, Value * * args, Value & v) } auto listOfMatches = state.allocList(); - auto transientListOfMatches = listOfMatches->transient(); // the first match is the whole string for (auto match : std::span(matches).subspan(1)) { if (!match.matched) { - transientListOfMatches.push_back(&state.vNull); + *listOfMatches = listOfMatches->push_back(&state.vNull); } else { - transientListOfMatches.push_back(mkString(state, match)); + *listOfMatches = listOfMatches->push_back(mkString(state, match)); } } - - *listOfMatches = transientListOfMatches.persistent(); v.mkList(listOfMatches); } catch (std::regex_error & e) { @@ -4394,10 +4356,8 @@ void prim_split(EvalState & state, const PosIdx pos, Value * * args, Value & v) const size_t len = std::distance(begin, end); auto listOfMatches = state.allocList(); - auto transientListOfMatches = listOfMatches->transient(); if (len == 0) { - transientListOfMatches.push_back(args[1]); - *listOfMatches = transientListOfMatches.persistent(); + *listOfMatches = listOfMatches->push_back(args[1]); v.mkList(listOfMatches); return; } @@ -4409,38 +4369,35 @@ void prim_split(EvalState & state, const PosIdx pos, Value * * args, Value & v) auto match = *i; // Add a string for non-matched characters. - transientListOfMatches.push_back(mkString(state, match.prefix())); + *listOfMatches = listOfMatches->push_back(mkString(state, match.prefix())); idx++; // Add a list for matched substrings. auto listOfSubMatches = state.allocList(); - auto transientListOfSubMatches = listOfSubMatches->transient(); // NOTE: Start at 1, beacause the first match is the whole string. for (auto subMatch : std::span(match).subspan(1)) { if (!subMatch.matched) { - transientListOfSubMatches.push_back(&state.vNull); + *listOfSubMatches = listOfSubMatches->push_back(&state.vNull); } else { - transientListOfSubMatches.push_back(mkString(state, subMatch)); + *listOfSubMatches = listOfSubMatches->push_back(mkString(state, subMatch)); } } - *listOfSubMatches = transientListOfSubMatches.persistent(); auto scratchValue = state.allocValue(); scratchValue->mkList(listOfSubMatches); - transientListOfMatches.push_back(scratchValue); + *listOfMatches = listOfMatches->push_back(scratchValue); idx++; // Add a string for non-matched suffix characters. if (idx == 2 * len) { - transientListOfMatches.push_back(mkString(state, match.suffix())); + *listOfMatches = listOfMatches->push_back(mkString(state, match.suffix())); idx++; } } assert(idx == 2 * len + 1); - *listOfMatches = transientListOfMatches.persistent(); v.mkList(listOfMatches); } catch (std::regex_error & e) { @@ -4665,13 +4622,11 @@ static void prim_splitVersion(EvalState & state, const PosIdx pos, Value * * arg } auto list = state.allocList(); - auto transientList = list->transient(); for (const auto & component : components) { auto v = state.allocValue(); v->mkString(std::move(component)); - transientList.push_back(v); + *list = list->push_back(v); } - *list = transientList.persistent(); v.mkList(list); } @@ -4915,16 +4870,14 @@ void EvalState::createBaseEnv() /* Add a value containing the current Nix expression search path. */ auto list = allocList(); - auto transientList = list->transient(); for (const auto & i : lookupPath.elements) { auto attrs = buildBindings(2); attrs.alloc("path").mkString(i.path.s); attrs.alloc("prefix").mkString(i.prefix.s); auto v = allocValue(); v->mkAttrs(attrs); - transientList.push_back(v); + *list = list->push_back(v); } - *list = transientList.persistent(); v.mkList(list); addConstant("__nixPath", v, { .type = nList, diff --git a/src/libexpr/primops/context.cc b/src/libexpr/primops/context.cc index 8d8271f4420..c783b0c4878 100644 --- a/src/libexpr/primops/context.cc +++ b/src/libexpr/primops/context.cc @@ -219,13 +219,11 @@ static void prim_getContext(EvalState & state, const PosIdx pos, Value * * args, infoAttrs.alloc(sAllOutputs).mkBool(true); if (!info.second.outputs.empty()) { auto list = state.allocList(); - auto transientList = list->transient(); for (const auto & [i, output] : enumerate(info.second.outputs)) { auto v = state.allocValue(); v->mkString(output); - transientList.push_back(v); + *list = list->push_back(v); } - *list = transientList.persistent(); infoAttrs.alloc(state.sOutputs).mkList(list); } attrs.alloc(state.store->printStorePath(info.first)).mkAttrs(infoAttrs); diff --git a/src/libexpr/primops/fromTOML.cc b/src/libexpr/primops/fromTOML.cc index ed8e578d8ca..c2e353075f6 100644 --- a/src/libexpr/primops/fromTOML.cc +++ b/src/libexpr/primops/fromTOML.cc @@ -39,13 +39,11 @@ static void prim_fromTOML(EvalState & state, const PosIdx pos, Value * * args, V auto array = toml::get>(t); auto list = state.allocList(); - auto transientList = list->transient(); for (const auto & tomlValue :array) { auto v = state.allocValue(); visit(*v, tomlValue); - transientList.push_back(v); + *list = list->push_back(v); } - *list = transientList.persistent(); v.mkList(list); } break;; diff --git a/src/libexpr/print-ambiguous.cc b/src/libexpr/print-ambiguous.cc index df4464a52a2..815cca17ecd 100644 --- a/src/libexpr/print-ambiguous.cc +++ b/src/libexpr/print-ambiguous.cc @@ -50,19 +50,7 @@ void printAmbiguous( break; } case nList: - // TODO(@connorbaker): Accessing payload directly is gross, but we need a single void* - // to insert into the set, and the way immer's flex_vector checks for equality yields - // a tuple of void*. - // TODO(@connorbaker): This currently fails the testcase - // $ nix-instantiate --eval --strict ./tests/functional/lang/eval-okay-print.nix - // [ null [ «repeated» ] ] - // It should yield: - // [ null [ [ «repeated» ] ] ] - // Oddly enough, - // $ nix eval --expr "$(cat ./tests/functional/lang/eval-okay-print.nix)" - // [ null «primop toString» «partially applied primop deepSeq» «lambda @ «string»:1:61» [ [ «repeated» ] ] ] - // So it's likely a bug in the path used by `nix-instantiate`. - if (seen && v.listSize() && !seen->insert(v.payload.list).second) + if (seen && v.listSize() && !seen->insert(&v).second) str << "«repeated»"; else { str << "[ "; diff --git a/src/libexpr/print.cc b/src/libexpr/print.cc index d8c0056e20c..55fccedb992 100644 --- a/src/libexpr/print.cc +++ b/src/libexpr/print.cc @@ -380,7 +380,7 @@ class Printer /** * @note This may force items. */ - bool shouldPrettyPrintList(const Value::List list) + bool shouldPrettyPrintList(const ValueList list) { if (!options.shouldPrettyPrint() || list.empty()) { return false; diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index 600f46dd78a..ede3f9fda38 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -2,7 +2,6 @@ ///@file #include -#include #include "eval-gc.hh" #include "symbol-table.hh" @@ -18,7 +17,6 @@ #include #include #include -#include // declare a memory policy for using a tracing garbage collector using gc_policy = immer::memory_policy, @@ -33,6 +31,9 @@ namespace nix { struct Value; 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 enum { tUninitialized = 0, @@ -217,11 +218,6 @@ public: ExprLambda * fun; }; - // alias the vector type so we are not concerned about memory policies - // in the places where we actually use it - using List = immer::flex_vector; - using TransientList = immer::flex_vector_transient; - using Payload = union { NixInt integer; @@ -232,7 +228,7 @@ public: Path path; Bindings * attrs; - List * list; + ValueList * list; ClosureThunk thunk; FunctionApplicationThunk app; Lambda lambda; @@ -341,7 +337,7 @@ public: Value & mkAttrs(BindingsBuilder & bindings); - void mkList(List * list) + void mkList(ValueList * list) { finishValue(tList, { .list = list }); } @@ -435,7 +431,7 @@ public: const Bindings * attrs() const { return payload.attrs; } - List list() const + ValueList list() const { return *payload.list; } const PrimOp * primOp() const diff --git a/src/nix-env/user-env.cc b/src/nix-env/user-env.cc index c8f4720dd35..eecb604b398 100644 --- a/src/nix-env/user-env.cc +++ b/src/nix-env/user-env.cc @@ -51,7 +51,6 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, /* Construct the whole top level derivation. */ StorePathSet references; auto list = state.allocList(); - auto listTransient = list->transient(); for (const auto & [n, i] : enumerate(elems)) { /* Create a pseudo-derivation containing the name, system, output paths, and optionally the derivation path, as well @@ -73,11 +72,10 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, // Copy each output meant for installation. auto outputsList = state.allocList(); - auto outputsListTransient = outputsList->transient(); for (const auto & j : outputs) { auto v = state.allocValue(); v->mkString(j.first); - outputsListTransient.push_back(v); + *outputsList = outputsList->push_back(v); auto outputAttrs = state.buildBindings(2); outputAttrs.alloc(state.sOutPath).mkString(state.store->printStorePath(*j.second)); attrs.alloc(j.first).mkAttrs(outputAttrs); @@ -89,7 +87,6 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, references.insert(*j.second); } - *outputsList = outputsListTransient.persistent(); attrs.alloc(state.sOutputs).mkList(outputsList); @@ -104,11 +101,10 @@ bool createUserEnv(EvalState & state, PackageInfos & elems, attrs.alloc(state.sMeta).mkAttrs(meta); auto v = state.allocValue(); v->mkAttrs(attrs); - listTransient.push_back(v); + *list = list->push_back(v); if (drvPath) references.insert(*drvPath); } - *list = listTransient.persistent(); Value manifest; manifest.mkList(list); From 72c2fa80a90886fc4b637894a1b539f6211fba57 Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Wed, 30 Oct 2024 05:29:00 +0000 Subject: [PATCH 3/9] can eval nixos system --- src/libexpr/eval.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 007c9787fd0..ac8d82c9b5c 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1904,7 +1904,13 @@ void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) e1->eval(state, env, *v1); auto v2 = state.allocValue(); e2->eval(state, env, *v2); - state.concatLists(v, ValueList({v1, v2}), pos, "while evaluating one of the elements to concatenate"); + // TODO(@connorbaker): This kills me -- why do we need to create the list on the heap? Shouldn't I be able to pass + // a ValueList by value to concatLists without worrying about it being garbage collected *while the function is running*? + // If this doesn't work, then should I allocList in the test cases? + auto list = state.allocList(); + *list = list->push_back(v1); + *list = list->push_back(v2); + state.concatLists(v, *list, pos, "while evaluating one of the elements to concatenate"); } From 39699679668a14337e8bec92b88ff6e8d5908419 Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Wed, 30 Oct 2024 07:04:41 +0000 Subject: [PATCH 4/9] specialize ExprOpConcatLists::eval; introduce EvalState::allocListFromInitializerList --- src/libexpr/eval-inline.hh | 7 +++++++ src/libexpr/eval.cc | 16 ++++++++++------ src/libexpr/eval.hh | 2 ++ src/libexpr/value.hh | 4 +++- tests/functional/lang/eval-fail-list.err.exp | 2 +- 5 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 3bbe0ad8862..e0e2b095c6a 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -34,6 +34,13 @@ 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() diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index ac8d82c9b5c..71851ebc7c1 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -20,6 +20,7 @@ #include "fetch-to-store.hh" #include "tarball.hh" #include "parser-tab.hh" +#include "value.hh" #include #include @@ -1300,6 +1301,10 @@ 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 + // 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)); @@ -1902,15 +1907,14 @@ void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) { 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); - // TODO(@connorbaker): This kills me -- why do we need to create the list on the heap? Shouldn't I be able to pass - // a ValueList by value to concatLists without worrying about it being garbage collected *while the function is running*? - // If this doesn't work, then should I allocList in the test cases? + state.forceList(*v2, pos, "in the right operand of the list concatenation operator"); + auto list = state.allocList(); - *list = list->push_back(v1); - *list = list->push_back(v2); - state.concatLists(v, *list, pos, "while evaluating one of the elements to concatenate"); + *list = v1->list() + v2->list(); + v.mkList(list); } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index e49aea98856..1cb095b38a9 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -709,6 +709,8 @@ public: * Allocation primitives. */ inline ValueList * allocList(); + template + inline ValueList * allocListFromInitializerList(std::initializer_list values); inline Value * allocValue(); inline Env & allocEnv(size_t size); diff --git a/src/libexpr/value.hh b/src/libexpr/value.hh index ede3f9fda38..4c14a628204 100644 --- a/src/libexpr/value.hh +++ b/src/libexpr/value.hh @@ -17,6 +17,7 @@ #include #include #include +#include // declare a memory policy for using a tracing garbage collector using gc_policy = immer::memory_policy, @@ -33,7 +34,8 @@ 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 ValueList; +typedef immer::flex_vector_transient ValueListTransient; typedef enum { tUninitialized = 0, diff --git a/tests/functional/lang/eval-fail-list.err.exp b/tests/functional/lang/eval-fail-list.err.exp index d492f8bd2e4..3ad791dc4d5 100644 --- a/tests/functional/lang/eval-fail-list.err.exp +++ b/tests/functional/lang/eval-fail-list.err.exp @@ -1,5 +1,5 @@ error: - … while evaluating one of the elements to concatenate + … in the left operand of the list concatenation operator at /pwd/lang/eval-fail-list.nix:1:2: 1| 8++1 | ^ From 6e372b55687a45ba9f50ff9462665f0c5d9cf4a0 Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Thu, 31 Oct 2024 23:01:25 +0000 Subject: [PATCH 5/9] wip alloc scheme and use of ranges --- flake.nix | 48 +++++++++++++++---------- src/libexpr/eval-gc.cc | 5 +-- src/libexpr/eval-inline.hh | 74 +++++++++++++++++++++++++++++--------- src/libexpr/eval.cc | 51 +++++++++++++++++--------- src/libexpr/eval.hh | 8 +++-- src/libexpr/meson.build | 8 +++++ src/libexpr/package.nix | 2 ++ src/libexpr/value.hh | 2 +- 8 files changed, 138 insertions(+), 60 deletions(-) diff --git a/flake.nix b/flake.nix index 04b16878d41..4076d9d68f5 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/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 71851ebc7c1..7b1c1e5ec91 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) @@ -1301,14 +1308,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); + // }))); } @@ -1905,9 +1920,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"); @@ -1924,12 +1942,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 1cb095b38a9..30b36c4a7dc 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -360,6 +360,11 @@ private: */ std::shared_ptr valueAllocCache; + /** + * Allocation cache for GC'd ValueList objects. + */ + std::shared_ptr listAllocCache; + /** * Allocation cache for size-1 Env objects. */ @@ -709,8 +714,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 a4a9a1f6ef2..300d67f0d14 100644 --- a/src/libexpr/package.nix +++ b/src/libexpr/package.nix @@ -13,6 +13,7 @@ , boost , boehmgc , nlohmann_json +, range-v3 , toml11 # Configuration Options @@ -79,6 +80,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, From 1def75dbe9df66a79ac5340b381d5216d4f16817 Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Fri, 8 Nov 2024 07:17:20 +0000 Subject: [PATCH 6/9] wip --- src/libexpr-tests/error_traces.cc | 4 +- src/libexpr-tests/meson.build | 2 + src/libexpr/eval-cache.cc | 6 +- src/libexpr/eval-inline.hh | 24 --- src/libexpr/eval.cc | 24 ++- src/libexpr/eval.hh | 1 - src/libexpr/get-drvs.cc | 23 ++- src/libexpr/json-to-value.cc | 9 +- src/libexpr/meson.build | 2 + src/libexpr/primops.cc | 326 +++++++++++++----------------- src/nix/meson.build | 2 + 11 files changed, 184 insertions(+), 239 deletions(-) diff --git a/src/libexpr-tests/error_traces.cc b/src/libexpr-tests/error_traces.cc index be379a90993..9fd0e3e47bd 100644 --- a/src/libexpr-tests/error_traces.cc +++ b/src/libexpr-tests/error_traces.cc @@ -708,11 +708,11 @@ namespace nix { ASSERT_TRACE2("head 1", TypeError, HintFmt("expected a list but found %s: %s", "an integer", Uncolored(ANSI_CYAN "1" ANSI_NORMAL)), - HintFmt("while evaluating the first argument passed to builtins.elemAt")); + HintFmt("while evaluating the first argument passed to builtins.head")); ASSERT_TRACE1("head []", Error, - HintFmt("list index %d is out of bounds", 0)); + HintFmt("'head' called on an empty list")); } diff --git a/src/libexpr-tests/meson.build b/src/libexpr-tests/meson.build index 5a5c9f1d48a..34df948f065 100644 --- a/src/libexpr-tests/meson.build +++ b/src/libexpr-tests/meson.build @@ -2,6 +2,8 @@ project('nix-expr-tests', 'cpp', version : files('.version'), default_options : [ 'cpp_std=c++2a', + # Set -Wno-array-bounds to avoid warnings in Immer's generated code. + 'cpp_args=-Warray-bounds=0', # TODO(Qyriad): increase the warning level 'warning_level=1', 'debug=true', diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index 61183011e3c..cd87c76dd24 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -6,6 +6,7 @@ #include "store-api.hh" // Need specialization involving `SymbolStr` just in this one module. #include "strings-inline.hh" +#include namespace nix::eval_cache { @@ -718,9 +719,10 @@ std::vector AttrCursor::getListOfStrings() root->state.error("'%s' is not a list", getAttrPathStr()).debugThrow(); std::vector res; - - for (auto & elem : v.list()) + res.reserve(v.listSize()); + immer::for_each(v.list(), [&](const auto & elem) { res.push_back(std::string(root->state.forceStringNoCtx(*elem, noPos, "while evaluating an attribute for caching"))); + }); if (root->db) cachedValue = {root->db->setListOfStrings(getKey(), res), res}; diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index e69764f9f4b..360512003f4 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -84,30 +84,6 @@ ValueList * EvalState::allocList() 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 7b1c1e5ec91..c8f38163540 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -37,9 +38,7 @@ #include #include #include -#include -#include -#include +#include #ifndef _WIN32 // TODO use portable implementation # include @@ -1938,11 +1937,12 @@ void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) void EvalState::concatLists(Value & v, const ValueList lists, const PosIdx pos, std::string_view errorCtx) { - nrListConcats++; - // 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. + // TODO(@connorbaker): Because concatenation is associative, we can reduce the number of concatenations by doing a tree-like + // concatenation instead of a linear one. Consider using a binary search to do so. v.mkList(immer::accumulate(lists, allocList(), [&](const auto & concatenated, const auto & list) { + nrListConcats++; forceList(*list, pos, errorCtx); *concatenated = *concatenated + list->list(); return concatenated; @@ -2105,6 +2105,9 @@ void EvalState::forceValueDeep(Value & v) } else if (v.isList()) { + // TODO(@connorbaker): Replacing this with + // immer::for_each(v.list(), [&](const auto & v) { recurse(*v); }); + // Increases the heap size by a fair amount, potentially because of the lambda capture. for (auto v2 : v.list()) recurse(*v2); } @@ -2736,10 +2739,13 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v return true; case nList: - if (v1.listSize() != v2.listSize()) return false; - for (size_t n = 0; n < v1.listSize(); ++n) - if (!eqValues(*v1.list()[n], *v2.list()[n], pos, errorCtx)) return false; - return true; + return v1.listSize() == v2.listSize() + && ranges::all_of( + ranges::views::zip_with( + [&](const auto & a, const auto & b) { return eqValues(*a, *b, pos, errorCtx); }, + ranges::subrange(v1.list().begin(), v1.list().end()), + ranges::subrange(v2.list().begin(), v2.list().end())), + std::identity{}); case nAttrs: { /* If both sets denote a derivation (type = "derivation"), diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 30b36c4a7dc..44cac8cfd65 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -714,7 +714,6 @@ public: * Allocation primitives. */ inline ValueList * allocList(); - template inline ValueList * allocListFromRange(Range range); inline Value * allocValue(); inline Env & allocEnv(size_t size); diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index cfb3bdae7f7..15b7f6500fd 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -5,6 +5,8 @@ #include "path-with-outputs.hh" #include +#include +#include #include @@ -115,25 +117,23 @@ PackageInfo::Outputs PackageInfo::queryOutputs(bool withPaths, bool onlyOutputsT const Attr * i; if (attrs && (i = attrs->get(state->sOutputs))) { state->forceList(*i->value, i->pos, "while evaluating the 'outputs' attribute of a derivation"); - - /* For each output... */ - for (auto elem : i->value->list()) { + immer::for_each(i->value->list(), [&](const auto & elem) { std::string output(state->forceStringNoCtx(*elem, i->pos, "while evaluating the name of an output of a derivation")); if (withPaths) { /* Evaluate the corresponding set. */ auto out = attrs->get(state->symbols.create(output)); - if (!out) continue; // FIXME: throw error? + if (!out) return; // FIXME: throw error? state->forceAttrs(*out->value, i->pos, "while evaluating an output of a derivation"); /* And evaluate its ‘outPath’ attribute. */ auto outPath = out->value->attrs()->get(state->sOutPath); - if (!outPath) continue; // FIXME: throw error? + if (!outPath) return; // FIXME: throw error? NixStringContext context; outputs.emplace(output, state->coerceToStorePath(outPath->pos, *outPath->value, context, "while evaluating an output path of a derivation")); } else outputs.emplace(output, std::nullopt); - } + }); } else outputs.emplace("out", withPaths ? std::optional{queryOutPath()} : std::nullopt); } @@ -159,12 +159,12 @@ PackageInfo::Outputs PackageInfo::queryOutputs(bool withPaths, bool onlyOutputsT /* ^ this shows during `nix-env -i` right under the bad derivation */ if (!outTI->isList()) throw errMsg; Outputs result; - for (auto elem : outTI->list()) { + immer::for_each(outTI->list(), [&](const auto & elem) { if (elem->type() != nString) throw errMsg; auto out = outputs.find(elem->c_str()); if (out == outputs.end()) throw errMsg; result.insert(*out); - } + }); return result; } } @@ -206,9 +206,10 @@ bool PackageInfo::checkMeta(Value & v) { state->forceValue(v, v.determinePos(noPos)); if (v.type() == nList) { - for (auto elem : v.list()) - if (!checkMeta(*elem)) return false; - return true; + // TODO(@connorbaker): Does immer::all_of support short-circuiting? + return ranges::all_of(v.list(), [&](const auto & elem) { + return checkMeta(*elem); + }); } else if (v.type() == nAttrs) { if (v.attrs()->get(state->sOutPath)) return false; diff --git a/src/libexpr/json-to-value.cc b/src/libexpr/json-to-value.cc index a4b13921d25..ade39d33b14 100644 --- a/src/libexpr/json-to-value.cc +++ b/src/libexpr/json-to-value.cc @@ -3,6 +3,7 @@ #include "eval.hh" #include +#include #include #include @@ -58,10 +59,10 @@ class JSONSax : nlohmann::json_sax { ValueVector values; std::unique_ptr resolve(EvalState & state) override { - auto list = state.allocList(); - for (auto & i : values) - *list = list->push_back(i); - parent->value(state).mkList(list); + parent->value(state).mkList(ranges::fold_left(values, state.allocList(), [](const auto & list, const auto & v) { + *list = list->push_back(v); + return list; + })); return std::move(parent); } void add() override { diff --git a/src/libexpr/meson.build b/src/libexpr/meson.build index 06b4e4d30a8..d3767622127 100644 --- a/src/libexpr/meson.build +++ b/src/libexpr/meson.build @@ -2,6 +2,8 @@ project('nix-expr', 'cpp', version : files('.version'), default_options : [ 'cpp_std=c++2a', + # Set -Wno-array-bounds to avoid warnings in Immer's generated code. + 'cpp_args=-Warray-bounds=0', # TODO(Qyriad): increase the warning level 'warning_level=1', 'debug=true', diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 05e54ea18a5..179f397ead9 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -16,8 +16,9 @@ #include "fetch-to-store.hh" #include +#include #include - +#include #include #include #include @@ -206,15 +207,14 @@ void derivationToValue(EvalState & state, const PosIdx pos, const SourcePath & p NixStringContextElem::DrvDeep { .drvPath = storePath }, }); attrs.alloc(state.sName).mkString(drv.env["name"]); - - auto list = state.allocList(); - for (const auto & o : drv.outputs) { - mkOutputString(state, attrs, storePath, o); - auto outputString = state.allocValue(); - outputString->mkString(o.first); - *list = list->push_back(outputString); - } - attrs.alloc(state.sOutputs).mkList(list); + attrs.alloc(state.sOutputs) + .mkList(ranges::fold_left(drv.outputs, state.allocList(), [&](const auto & list, const auto & output) { + mkOutputString(state, attrs, storePath, output); + auto outputString = state.allocValue(); + outputString->mkString(output.first); + *list = list->push_back(outputString); + return list; + })); auto w = state.allocValue(); w->mkAttrs(attrs); @@ -423,12 +423,11 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v) "while evaluating the first element of the argument passed to builtins.exec", false, false).toOwned(); Strings commandArgs; - for (unsigned int i = 1; i < args[0]->listSize(); ++i) { - commandArgs.push_back( - state.coerceToString(pos, *elems[i], context, - "while evaluating an element of the argument passed to builtins.exec", - false, false).toOwned()); - } + immer::for_each(args[0]->list().drop(1), [&](const auto & v) { + commandArgs.push_back(state.coerceToString(pos, *v, context, + "while evaluating an element of the argument passed to builtins.exec", + false, false).toOwned()); + }); try { auto _ = state.realiseContext(context); // FIXME: Handle CA derivations } catch (InvalidPathError & e) { @@ -654,15 +653,10 @@ struct CompareValues return strcmp(v1->payload.path.path, v2->payload.path.path) < 0; case nList: // Lexicographic comparison - for (size_t i = 0;; i++) { - if (i == v2->listSize()) { - return false; - } else if (i == v1->listSize()) { - return true; - } else if (!state.eqValues(*v1->list()[i], *v2->list()[i], pos, errorCtx)) { - return (*this)(v1->list()[i], v2->list()[i], "while comparing two list elements"); - } - } + return ranges::lexicographical_compare(v1->list(), v2->list(), [&](const auto & a, const auto & b) { + // If they are not equal, return the result of comparing them + return !state.eqValues(*a, *b, pos, errorCtx) && (*this)(a, b, "while comparing two list elements"); + }); default: state.error("cannot compare %s with %s; values of that type are incomparable", showType(*v1), showType(*v2)).debugThrow(); #pragma GCC diagnostic pop @@ -697,10 +691,6 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a state.forceList(*startSet->value, noPos, "while evaluating the 'startSet' attribute passed as argument to builtins.genericClosure"); - auto workSet = state.allocList(); - for (auto elem : startSet->value->list()) - *workSet = workSet->push_back(elem); - if (startSet->value->listSize() == 0) { v = *startSet->value; return; @@ -718,9 +708,11 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a // reachable from res. auto cmp = CompareValues(state, noPos, "while comparing the `key` attributes of two genericClosure elements"); std::set> doneKeys(cmp); - while (!workSet->empty()) { - Value * e = workSet->front(); - *workSet = workSet->drop(1); + + auto workSet = startSet->value->list(); + while (!workSet.empty()) { + auto e = workSet.front(); + workSet = workSet.drop(1); state.forceAttrs(*e, noPos, "while evaluating one of the elements generated by (or initially passed to) builtins.genericClosure"); @@ -734,12 +726,12 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a Value newElements; state.callFunction(*op->value, 1, &e, newElements, noPos); state.forceList(newElements, noPos, "while evaluating the return value of the `operator` passed to builtins.genericClosure"); + immer::for_each(newElements.list(), [&](const auto &elem){ + state.forceValue(*elem, noPos); + }); /* Add the values returned by the operator to the work set. */ - for (auto elem : newElements.list()) { - state.forceValue(*elem, noPos); // "while evaluating one one of the elements returned by the `operator` passed to builtins.genericClosure"); - *workSet = workSet->push_back(elem); - } + workSet = workSet + newElements.list(); } v.mkList(res); @@ -1328,12 +1320,11 @@ static void derivationStrictInternal( command-line arguments to the builder. */ else if (i->name == state.sArgs) { state.forceList(*i->value, pos, context_below); - for (auto elem : i->value->list()) { - auto s = state.coerceToString(pos, *elem, context, + immer::for_each(i->value->list(), [&](const auto & elem){ + drv.args.push_back(state.coerceToString(pos, *elem, context, "while evaluating an element of the argument list", - true).toOwned(); - drv.args.push_back(s); - } + true).toOwned()); + }); } /* All other attributes are passed to the builder through @@ -1360,8 +1351,9 @@ static void derivationStrictInternal( /* Require ‘outputs’ to be a list of strings. */ state.forceList(*i->value, pos, context_below); Strings ss; - for (auto elem : i->value->list()) + immer::for_each(i->value->list(), [&](const auto & elem){ ss.emplace_back(state.forceStringNoCtx(*elem, pos, context_below)); + }); handleOutputs(ss); } @@ -1835,8 +1827,7 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.findFile"); LookupPath lookupPath; - - for (auto v2 : args[0]->list()) { + immer::for_each(args[0]->list(), [&](const auto & v2){ state.forceAttrs(*v2, pos, "while evaluating an element of the list passed to builtins.findFile"); std::string prefix; @@ -1866,7 +1857,7 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V .prefix = LookupPath::Prefix { .s = prefix }, .path = LookupPath::Path { .s = path }, }); - } + }); auto path = state.forceStringNoCtx(*args[1], pos, "while evaluating the second argument passed to builtins.findFile"); @@ -2691,10 +2682,11 @@ static void prim_attrNames(EvalState & state, const PosIdx pos, Value * * args, std::sort(vec.begin(), vec.end(), [](Value * v1, Value * v2) { return strcmp(v1->c_str(), v2->c_str()) < 0; }); - auto list = state.allocList(); - for (auto & v : vec) + + v.mkList(ranges::fold_left(vec, state.allocList(), [](const auto & list, const auto & v) { *list = list->push_back(v); - v.mkList(list); + return list; + })); } static RegisterPrimOp primop_attrNames({ @@ -2714,28 +2706,18 @@ static void prim_attrValues(EvalState & state, const PosIdx pos, Value * * args, { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.attrValues"); - ValueVector vec; - vec.reserve(args[0]->attrs()->size()); - - for (const auto & attr: *args[0]->attrs()) - // Extremely gross mis-cast so we can sort by name - vec.push_back((Value *) &attr); - - std::sort(vec.begin(), vec.end(), - [&](Value * v1, Value * v2) { - std::string_view s1 = state.symbols[((Attr *) v1)->name], - s2 = state.symbols[((Attr *) v2)->name]; - return s1 < s2; - }); - - // Quickly convert the values to the actual values to hide the shame. - for (auto & v : vec) - v = ((Attr *) v)->value; - - auto list = state.allocList(); - for (auto & v : vec) - *list = list->push_back(v); - v.mkList(list); + v.mkList(ranges::fold_left( + ranges::views::all(*args[0]->attrs()) + | ranges::views::transform([&](const auto & attr) { return &attr; }) + | ranges::to_vector + | ranges::actions::sort([&](const auto & a, const auto & b) { + return std::string_view(state.symbols[a->name]) < std::string_view(state.symbols[b->name]); + }), + state.allocList(), + [](const auto & list, const auto & v) { + *list = list->push_back(((Attr *) v)->value); + return list; + })); } static RegisterPrimOp primop_attrValues({ @@ -2889,10 +2871,10 @@ static void prim_removeAttrs(EvalState & state, const PosIdx pos, Value * * args // 64: large enough to fit the attributes of a derivation boost::container::small_vector names; names.reserve(args[1]->listSize()); - for (auto elem : args[1]->list()) { + immer::for_each(args[1]->list(), [&](const auto & elem) { state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs"); names.emplace_back(state.symbols.create(elem->string_view()), nullptr); - } + }); std::sort(names.begin(), names.end()); /* Copy all attributes not in that set. Note that we don't need @@ -2935,7 +2917,7 @@ static void prim_listToAttrs(EvalState & state, const PosIdx pos, Value * * args std::set, traceable_allocator> seen; - for (auto v2 : args[0]->list()) { + immer::for_each(args[0]->list(), [&](const auto & v2) { state.forceAttrs(*v2, pos, "while evaluating an element of the list passed to builtins.listToAttrs"); auto j = getAttr(state, state.sName, v2->attrs(), "in a {name=...; value=...;} pair"); @@ -2947,7 +2929,7 @@ static void prim_listToAttrs(EvalState & state, const PosIdx pos, Value * * args auto j2 = getAttr(state, state.sValue, v2->attrs(), "in a {name=...; value=...;} pair"); attrs.insert(sym, j2->value, j2->pos); } - } + }); v.mkAttrs(attrs); } @@ -3065,21 +3047,12 @@ static void prim_catAttrs(EvalState & state, const PosIdx pos, Value * * args, V { auto attrName = state.symbols.create(state.forceStringNoCtx(*args[0], pos, "while evaluating the first argument passed to builtins.catAttrs")); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.catAttrs"); - - SmallValueVector res(args[1]->listSize()); - size_t found = 0; - - for (auto v2 : args[1]->list()) { + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](const auto & list, const auto & v2) { state.forceAttrs(*v2, pos, "while evaluating an element in the list passed as second argument to builtins.catAttrs"); if (auto i = v2->attrs()->get(attrName)) - res[found++] = i->value; - } - - auto list = state.allocList(); - for (unsigned int n = 0; n < found; ++n) - *list = list->push_back(res[n]); - - v.mkList(list); + *list = list->push_back(i->value); + return list; + })); } static RegisterPrimOp primop_catAttrs({ @@ -3172,33 +3145,29 @@ static RegisterPrimOp primop_mapAttrs({ static void prim_zipAttrsWith(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - ValueVectorMap attrsSeen; + std::map, traceable_allocator>> attrsSeen; state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.zipAttrsWith"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.zipAttrsWith"); - for (auto & vElem : args[1]->list()) { + immer::for_each(args[1]->list(), [&](const auto & vElem) { state.forceAttrs(*vElem, noPos, "while evaluating a value of the list passed as second argument to builtins.zipAttrsWith"); - for (auto & attr : *vElem->attrs()) { - if (!attrsSeen.contains(attr.name)) { - attrsSeen[attr.name] = ValueVector(); - } - attrsSeen[attr.name].push_back(attr.value); + for (const auto & attr : *vElem->attrs()) { + attrsSeen.try_emplace(attr.name, state.allocList()); + *attrsSeen[attr.name] = attrsSeen[attr.name]->push_back(attr.value); } - } + }); auto attrs = state.buildBindings(attrsSeen.size()); - for (auto & [sym, valueVector] : attrsSeen) { + for (const auto & [sym, list] : attrsSeen) { auto name = state.allocValue(); name->mkString(state.symbols[sym]); + auto arg = state.allocValue(); + arg->mkList(list); auto call1 = state.allocValue(); call1->mkApp(args[0], name); auto call2 = state.allocValue(); - auto arg = state.allocValue(); - auto list = state.allocList(); - for (auto v : valueVector) *list = list->push_back(v); - arg->mkList(list); call2->mkApp(call1, arg); attrs.insert(sym, call2); } @@ -3292,7 +3261,11 @@ static RegisterPrimOp primop_elemAt({ /* Return the first element of a list. */ static void prim_head(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - elemAt(state, pos, *args[0], 0, v); + state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.head"); + if (args[0]->listSize() == 0) + state.error("'head' called on an empty list").atPos(pos).debugThrow(); + state.forceValue(*args[0]->list().front(), pos); + v = *args[0]->list().front(); } static RegisterPrimOp primop_head({ @@ -3348,13 +3321,12 @@ static void prim_map(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.map"); - auto list = state.allocList(); - for (const auto & v : args[1]->list()) { + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](const auto & list, const auto & v) { auto newV = state.allocValue(); newV->mkApp(args[0], v); *list = list->push_back(newV); - } - v.mkList(list); + return list; + })); } static RegisterPrimOp primop_map({ @@ -3387,14 +3359,13 @@ static void prim_filter(EvalState & state, const PosIdx pos, Value * * args, Val state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filter"); - auto list = state.allocList(); - for (auto arg : args[1]->list()) { + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](const auto & list, const auto & arg) { Value res; state.callFunction(*args[0], *arg, res, noPos); if (state.forceBool(res, pos, "while evaluating the return value of the filtering function passed to builtins.filter")) *list = list->push_back(arg); - } - v.mkList(list); + return list; + })); } static RegisterPrimOp primop_filter({ @@ -3410,14 +3381,11 @@ static RegisterPrimOp primop_filter({ /* Return true if a list contains a given element. */ static void prim_elem(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - bool res = false; state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.elem"); - for (auto elem : args[1]->list()) - if (state.eqValues(*args[0], *elem, pos, "while searching for the presence of the given element in the list")) { - res = true; - break; - } - v.mkBool(res); + // TODO(@connorbaker): immer doesn't provide any_of. + v.mkBool(ranges::any_of(args[1]->list(), [&](const auto & elem) { + return state.eqValues(*args[0], *elem, pos, "while searching for the presence of the given element in the list"); + })); } static RegisterPrimOp primop_elem({ @@ -3482,6 +3450,17 @@ static void prim_foldlStrict(EvalState & state, const PosIdx pos, Value * * args state.forceValue(*args[1], pos); v = *args[1]; } + + // TODO(@connorbaker): Find a way to rewrite this so it doesn't need to allocate a new value through each iteration. + // Value * vs[3] {state.allocValue(), state.allocValue(), state.allocValue()}; + // *vs[2] = *args[1]; + // for (size_t n = 0; n < args[2]->listSize(); ++n) { + // *vs[0] = *vs[2]; + // *vs[1] = *args[2]->list()[n]; + // state.callFunction(*args[0], 2, vs, *vs[2], pos); + // } + // v = *vs[2]; + // state.forceValue(v, pos); } static RegisterPrimOp primop_foldlStrict({ @@ -3504,32 +3483,15 @@ static RegisterPrimOp primop_foldlStrict({ .fun = prim_foldlStrict, }); -static void anyOrAll(bool any, EvalState & state, const PosIdx pos, Value * * args, Value & v) -{ - state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.") + (any ? "any" : "all")); - state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.") + (any ? "any" : "all")); - - std::string_view errorCtx = any - ? "while evaluating the return value of the function passed to builtins.any" - : "while evaluating the return value of the function passed to builtins.all"; - - Value vTmp; - for (auto elem : args[1]->list()) { - state.callFunction(*args[0], *elem, vTmp, pos); - bool res = state.forceBool(vTmp, pos, errorCtx); - if (res == any) { - v.mkBool(any); - return; - } - } - - v.mkBool(!any); -} - - static void prim_any(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - anyOrAll(true, state, pos, args, v); + state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.any")); + state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.any")); + std::string_view errorCtx = "while evaluating the return value of the function passed to builtins.any"; + v.mkBool(ranges::any_of(args[1]->list(), [&](const auto & elem) { + state.callFunction(*args[0], *elem, v, pos); + return state.forceBool(v, pos, errorCtx); + })); } static RegisterPrimOp primop_any({ @@ -3544,7 +3506,13 @@ static RegisterPrimOp primop_any({ static void prim_all(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - anyOrAll(false, state, pos, args, v); + state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.all")); + state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.all")); + std::string_view errorCtx = "while evaluating the return value of the function passed to builtins.all"; + v.mkBool(ranges::all_of(args[1]->list(), [&](const auto & elem) { + state.callFunction(*args[0], *elem, v, pos); + return state.forceBool(v, pos, errorCtx); + })); } static RegisterPrimOp primop_all({ @@ -3570,15 +3538,14 @@ static void prim_genList(EvalState & state, const PosIdx pos, Value * * args, Va // as evaluating map without accessing any values makes little sense. state.forceFunction(*args[0], noPos, "while evaluating the first argument passed to builtins.genList"); - auto list = state.allocList(); - for (size_t n = 0; n < len; ++n) { + v.mkList(ranges::fold_left(ranges::views::iota(size_t(0), len), state.allocList(), [&](const auto & list, const auto & n) { auto arg = state.allocValue(); arg->mkInt(n); auto app = state.allocValue(); app->mkApp(args[0], arg); *list = list->push_back(app); - } - v.mkList(list); + return list; + })); } static RegisterPrimOp primop_genList({ @@ -3614,11 +3581,10 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value ValueVector vec; vec.reserve(len); - - for (const auto & v : args[1]->list()) { + immer::for_each(args[1]->list(), [&](const auto & v) { state.forceValue(*v, pos); vec.push_back(v); - } + }); auto comparator = [&](Value * a, Value * b) { /* Optimization: if the comparator is lessThan, bypass @@ -3640,10 +3606,10 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value resilient, but no guarantees... */ std::stable_sort(vec.begin(), vec.end(), comparator); - auto list = state.allocList(); - for (const auto v : vec) + v.mkList(ranges::fold_left(vec, state.allocList(), [&](const auto & list, const auto & v) { *list = list->push_back(v); - v.mkList(list); + return list; + })); } static RegisterPrimOp primop_sort({ @@ -3675,7 +3641,7 @@ static void prim_partition(EvalState & state, const PosIdx pos, Value * * args, auto rlist = state.allocList(); auto wlist = state.allocList(); - for (const auto v : args[1]->list()) { + immer::for_each(args[1]->list(), [&](const auto & v) { state.forceValue(*v, pos); Value res; state.callFunction(*args[0], *v, res, pos); @@ -3683,7 +3649,7 @@ static void prim_partition(EvalState & state, const PosIdx pos, Value * * args, *rlist = rlist->push_back(v); else *wlist = wlist->push_back(v); - } + }); auto attrs = state.buildBindings(2); @@ -3721,29 +3687,24 @@ static void prim_groupBy(EvalState & state, const PosIdx pos, Value * * args, Va state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.groupBy"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.groupBy"); - ValueVectorMap attrsSeen; + // TODO(@connorbaker): Rewrite and optimize this function. + std::map, traceable_allocator>> attrsSeen; - for (auto vElem : args[1]->list()) { + immer::for_each(args[1]->list(), [&](const auto & vElem) { Value res; state.callFunction(*args[0], *vElem, res, pos); auto name = state.forceStringNoCtx(res, pos, "while evaluating the return value of the grouping function passed to builtins.groupBy"); auto sym = state.symbols.create(name); - if (!attrsSeen.contains(sym)) { - attrsSeen[sym] = ValueVector(); - } - attrsSeen[sym].push_back(vElem); - } - - auto attrs2 = state.buildBindings(attrsSeen.size()); - - for (auto & [name, valueVector] : attrsSeen) { - auto list = state.allocList(); - for (auto v : valueVector) *list = list->push_back(v); - attrs2.alloc(name).mkList(list); - } + attrsSeen.try_emplace(sym, state.allocList()); + *attrsSeen[sym] = attrsSeen[sym]->push_back(vElem); + }); - v.mkAttrs(attrs2.alreadySorted()); + auto attrs = state.buildBindings(attrsSeen.size()); + ranges::for_each(attrsSeen, [&](const auto & pair) { + attrs.alloc(pair.first).mkList(pair.second); + }); + v.mkAttrs(attrs.alreadySorted()); } static RegisterPrimOp primop_groupBy({ @@ -3774,17 +3735,14 @@ static void prim_concatMap(EvalState & state, const PosIdx pos, Value * * args, { state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.concatMap"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.concatMap"); - - auto list = state.allocList(); - - for (auto arg : args[1]->list()) { + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](const auto & list, const auto & v) { // TODO(@connorbaker): Will the list stored in res be available after the loop, or will it be garbage collected? Value res; - state.callFunction(*args[0], *arg, res, pos); + state.callFunction(*args[0], *v, res, pos); state.forceList(res, res.determinePos(args[0]->determinePos(pos)), "while evaluating the return value of the function passed to builtins.concatMap"); *list = *list + res.list(); - } - v.mkList(list); + return list; + })); } static RegisterPrimOp primop_concatMap({ @@ -4275,17 +4233,11 @@ void prim_match(EvalState & state, const PosIdx pos, Value * * args, Value & v) return; } - auto listOfMatches = state.allocList(); - // the first match is the whole string - for (auto match : std::span(matches).subspan(1)) { - if (!match.matched) { - *listOfMatches = listOfMatches->push_back(&state.vNull); - } else { - *listOfMatches = listOfMatches->push_back(mkString(state, match)); - } - } - v.mkList(listOfMatches); + v.mkList(ranges::fold_left(std::span(matches).subspan(1), state.allocList(), [&](const auto & listOfMatches, const auto & match) { + *listOfMatches = listOfMatches->push_back(match.matched ? mkString(state, match) : &state.vNull); + return listOfMatches; + })); } catch (std::regex_error & e) { if (e.code() == std::regex_constants::error_space) { @@ -4461,6 +4413,8 @@ static void prim_concatStringsSep(EvalState & state, const PosIdx pos, Value * * 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. + 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"); diff --git a/src/nix/meson.build b/src/nix/meson.build index 60ee480358c..023b597d174 100644 --- a/src/nix/meson.build +++ b/src/nix/meson.build @@ -2,6 +2,8 @@ project('nix', 'cpp', version : files('.version'), default_options : [ 'cpp_std=c++2a', + # Set -Wno-array-bounds to avoid warnings in Immer's generated code. + 'cpp_args=-Warray-bounds=0', # TODO(Qyriad): increase the warning level 'warning_level=1', 'debug=true', From aa801d3ec484df92413d4e849ba08e9235fa5dfb Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Thu, 14 Nov 2024 05:42:57 +0000 Subject: [PATCH 7/9] wip --- src/libcmd/repl.cc | 2 +- src/libexpr/eval-cache.cc | 2 +- src/libexpr/eval-inline.hh | 159 +++++++++++-------- src/libexpr/eval.cc | 158 ++++++++++-------- src/libexpr/eval.hh | 8 +- src/libexpr/get-drvs.cc | 26 +-- src/libexpr/json-to-value.cc | 4 +- src/libexpr/primops.cc | 299 ++++++++++++++++++----------------- 8 files changed, 358 insertions(+), 300 deletions(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index f292f06bb81..beb90143b6d 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -741,7 +741,7 @@ void NixRepl::loadFlake(const std::string & flakeRefS) void NixRepl::initEnv() { - env = &state->allocEnv(envSize); + env = state->allocEnv(envSize); env->up = &state->baseEnv; displ = 0; staticEnv->vars.clear(); diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc index cd87c76dd24..ffc7b3fd3b6 100644 --- a/src/libexpr/eval-cache.cc +++ b/src/libexpr/eval-cache.cc @@ -720,7 +720,7 @@ std::vector AttrCursor::getListOfStrings() std::vector res; res.reserve(v.listSize()); - immer::for_each(v.list(), [&](const auto & elem) { + immer::for_each(v.list(), [&](auto * elem) { res.push_back(std::string(root->state.forceStringNoCtx(*elem, noPos, "while evaluating an attribute for caching"))); }); diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh index 360512003f4..4539f7ce46e 100644 --- a/src/libexpr/eval-inline.hh +++ b/src/libexpr/eval-inline.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include "config-expr.hh" #include "print.hh" #include "eval.hh" #include "eval-error.hh" @@ -12,45 +13,58 @@ namespace nix { /** * Note: Various places expect the allocated memory to be zeroed. */ -[[gnu::always_inline]] +[[nodiscard]] +[[using gnu: hot, always_inline, returns_nonnull, malloc, alloc_size(1)]] inline void * allocBytes(size_t n) { - void * p; -#if HAVE_BOEHMGC - p = GC_MALLOC(n); -#else - p = calloc(n, 1); -#endif - if (!p) throw std::bad_alloc(); + void * p = nullptr; + + if constexpr (HAVE_BOEHMGC) { + p = GC_MALLOC(n); + } else { + p = calloc(n, 1); + } + + if (p == nullptr) [[unlikely]] { + throw std::bad_alloc(); + } return p; } -[[gnu::always_inline]] -Value * EvalState::allocValue() + +[[nodiscard]] +[[using gnu: hot, always_inline, returns_nonnull, malloc]] +inline Value * EvalState::allocValue() { -#if HAVE_BOEHMGC - /* We use the boehm batch allocator to speed up allocations of Values (of which there are many). + void * p = nullptr; + + if constexpr (HAVE_BOEHMGC) { + /* We use the boehm batch allocator to speed up allocations of Values (of which there are many). GC_malloc_many returns a linked list of objects of the given size, where the first word of each object is also the pointer to the next object in the list. This also means that we have to explicitly clear the first word of every object we take. */ - if (!*valueAllocCache) { - *valueAllocCache = GC_malloc_many(sizeof(Value)); - if (!*valueAllocCache) throw std::bad_alloc(); - } + if (*valueAllocCache == nullptr) [[unlikely]] { + *valueAllocCache = GC_malloc_many(sizeof(Value)); + } - /* GC_NEXT is a convenience macro for accessing the first word of an object. - Take the first list item, advance the list to the next item, and clear the next pointer. */ - void * p = *valueAllocCache; - *valueAllocCache = GC_NEXT(p); - GC_NEXT(p) = nullptr; -#else - void * p = allocBytes(sizeof(Value)); -#endif + if (*valueAllocCache == nullptr) [[unlikely]] { + throw std::bad_alloc(); + } + + /* GC_NEXT is a convenience macro for accessing the first word of an object. + Take the first list item, advance the list to the next item, and clear the next pointer. */ + p = *valueAllocCache; + *valueAllocCache = GC_NEXT(p); + GC_NEXT(p) = nullptr; + } else { + p = allocBytes(sizeof(Value)); + } nrValues++; - return (Value *) p; + return static_cast(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' @@ -61,62 +75,73 @@ Value * EvalState::allocValue() // 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() +[[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 HAVE_BOEHMGC - if (!*listAllocCache) { - *listAllocCache = GC_malloc_many(sizeof(ValueList)); - if (!*listAllocCache) throw std::bad_alloc(); - } + if constexpr (HAVE_BOEHMGC) { + if (*listAllocCache == nullptr) [[unlikely]] { + *listAllocCache = GC_malloc_many(sizeof(ValueList)); + } - void * p = *listAllocCache; - *listAllocCache = GC_NEXT(p); - GC_NEXT(p) = nullptr; -#else - void * p = allocBytes(sizeof(ValueList)); -#endif + if (*listAllocCache == nullptr) [[unlikely]] { + throw std::bad_alloc(); + } + + p = *listAllocCache; + *listAllocCache = GC_NEXT(p); + GC_NEXT(p) = nullptr; + } else { + p = allocBytes(sizeof(ValueList)); + } return ::new (p) ValueList; } -[[gnu::always_inline]] -Env & EvalState::allocEnv(size_t size) +[[nodiscard]] +[[using gnu: hot, always_inline, returns_nonnull, malloc]] +inline Env * EvalState::allocEnv(size_t size) { nrEnvs++; nrValuesInEnvs += size; - Env * env; + void * p = nullptr; + + if constexpr (HAVE_BOEHMGC) { + if (size == 1) { + /* see allocValue for explanations. */ + if (*env1AllocCache == nullptr) [[unlikely]] { + *env1AllocCache = GC_malloc_many(sizeof(Env) + sizeof(Value *)); + } -#if HAVE_BOEHMGC - if (size == 1) { - /* see allocValue for explanations. */ - if (!*env1AllocCache) { - *env1AllocCache = GC_malloc_many(sizeof(Env) + sizeof(Value *)); - if (!*env1AllocCache) throw std::bad_alloc(); + if (*env1AllocCache == nullptr) [[unlikely]] { + throw std::bad_alloc(); + } + + p = *env1AllocCache; + *env1AllocCache = GC_NEXT(p); + GC_NEXT(p) = nullptr; } + } - void * p = *env1AllocCache; - *env1AllocCache = GC_NEXT(p); - GC_NEXT(p) = nullptr; - env = (Env *) p; - } else -#endif - env = (Env *) allocBytes(sizeof(Env) + size * sizeof(Value *)); + if (p == nullptr) { + p = allocBytes(sizeof(Env) + size * sizeof(Value *)); + } /* We assume that env->values has been cleared by the allocator; maybeThunk() and lookupVar fromWith expect this. */ - - return *env; + return static_cast(p); } -[[gnu::always_inline]] -void EvalState::forceValue(Value & v, const PosIdx pos) +[[using gnu: hot, always_inline]] +inline void EvalState::forceValue(Value & v, const PosIdx pos) { if (v.isThunk()) { Env * env = v.payload.thunk.env; @@ -139,7 +164,7 @@ void EvalState::forceValue(Value & v, const PosIdx pos) } -[[gnu::always_inline]] +[[using gnu: hot, always_inline]] inline void EvalState::forceAttrs(Value & v, const PosIdx pos, std::string_view errorCtx) { forceAttrs(v, [&]() { return pos; }, errorCtx); @@ -147,12 +172,12 @@ inline void EvalState::forceAttrs(Value & v, const PosIdx pos, std::string_view template -[[gnu::always_inline]] +[[using gnu: hot, always_inline]] inline void EvalState::forceAttrs(Value & v, Callable getPos, std::string_view errorCtx) { PosIdx pos = getPos(); forceValue(v, pos); - if (v.type() != nAttrs) { + if (v.type() != nAttrs) [[ unlikely ]] { error( "expected a set but found %1%: %2%", showType(v), @@ -162,11 +187,11 @@ inline void EvalState::forceAttrs(Value & v, Callable getPos, std::string_view e } -[[gnu::always_inline]] +[[using gnu: hot, always_inline]] inline void EvalState::forceList(Value & v, const PosIdx pos, std::string_view errorCtx) { forceValue(v, pos); - if (!v.isList()) { + if (!v.isList()) [[ unlikely ]] { error( "expected a list but found %1%: %2%", showType(v), @@ -175,12 +200,14 @@ inline void EvalState::forceList(Value & v, const PosIdx pos, std::string_view e } } -[[gnu::always_inline]] + +[[using gnu: hot, always_inline]] inline CallDepth EvalState::addCallDepth(const PosIdx pos) { - if (callDepth > settings.maxCallDepth) + if (callDepth > settings.maxCallDepth) [[unlikely ]] { error("stack overflow; max-call-depth exceeded").atPos(pos).debugThrow(); + } - return CallDepth(callDepth); + return {callDepth}; }; } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index c8f38163540..9c696bc1a82 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -281,7 +282,7 @@ EvalState::EvalState( , 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))) + , baseEnvP(std::allocate_shared(traceable_allocator(), allocEnv(BASE_ENV_SIZE))) , baseEnv(**baseEnvP) #else , baseEnv(allocEnv(BASE_ENV_SIZE)) @@ -1167,29 +1168,30 @@ void ExprPath::eval(EvalState & state, Env & env, Value & v) Env * ExprAttrs::buildInheritFromEnv(EvalState & state, Env & up) { - Env & inheritEnv = state.allocEnv(inheritFromExprs->size()); - inheritEnv.up = &up; + Env * inheritEnv = state.allocEnv(inheritFromExprs->size()); + inheritEnv->up = &up; Displacement displ = 0; - for (auto from : *inheritFromExprs) - inheritEnv.values[displ++] = from->maybeThunk(state, up); + for (auto * from : *inheritFromExprs) { + inheritEnv->values[displ++] = from->maybeThunk(state, up); + } - return &inheritEnv; + return inheritEnv; } void ExprAttrs::eval(EvalState & state, Env & env, Value & v) { auto bindings = state.buildBindings(attrs.size() + dynamicAttrs.size()); - auto dynamicEnv = &env; + auto * dynamicEnv = &env; bool sort = false; if (recursive) { /* Create a new environment that contains the attributes in this `rec'. */ - Env & env2(state.allocEnv(attrs.size())); - env2.up = &env; - dynamicEnv = &env2; - Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env2) : nullptr; + Env * env2(state.allocEnv(attrs.size())); + env2->up = &env; + dynamicEnv = env2; + Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, *env2) : nullptr; AttrDefs::iterator overrides = attrs.find(state.sOverrides); bool hasOverrides = overrides != attrs.end(); @@ -1202,10 +1204,11 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) Value * vAttr; if (hasOverrides && i.second.kind != AttrDef::Kind::Inherited) { vAttr = state.allocValue(); - mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, inheritEnv), i.second.e); - } else - vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(&env2, &env, inheritEnv)); - env2.values[displ++] = vAttr; + mkThunk(*vAttr, *i.second.chooseByKind(env2, &env, inheritEnv), i.second.e); + } else { + vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(env2, &env, inheritEnv)); + } + env2->values[displ++] = vAttr; bindings.insert(i.first, vAttr, i.second.pos); } @@ -1225,7 +1228,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) AttrDefs::iterator j = attrs.find(i.name); if (j != attrs.end()) { (*bindings.bindings)[j->second.displ] = i; - env2.values[j->second.displ] = i.value; + env2->values[j->second.displ] = i.value; } else bindings.push_back(i); } @@ -1273,26 +1276,26 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) { /* Create a new environment that contains the attributes in this `let'. */ - Env & env2(state.allocEnv(attrs->attrs.size())); - env2.up = &env; + Env * env2(state.allocEnv(attrs->attrs.size())); + env2->up = &env; - Env * inheritEnv = attrs->inheritFromExprs ? attrs->buildInheritFromEnv(state, env2) : nullptr; + Env * inheritEnv = attrs->inheritFromExprs ? attrs->buildInheritFromEnv(state, *env2) : nullptr; /* The recursive attributes are evaluated in the new environment, while the inherited attributes are evaluated in the original environment. */ Displacement displ = 0; for (auto & i : attrs->attrs) { - env2.values[displ++] = i.second.e->maybeThunk( + env2->values[displ++] = i.second.e->maybeThunk( state, - *i.second.chooseByKind(&env2, &env, inheritEnv)); + *i.second.chooseByKind(env2, &env, inheritEnv)); } auto dts = state.debugRepl ? makeDebugTraceStacker( state, *this, - env2, + *env2, getPos() ? std::make_shared(state.positions[getPos()]) : nullptr, @@ -1301,20 +1304,24 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) ) : nullptr; - body->eval(state, env2, v); + body->eval(state, *env2, v); } void ExprList::eval(EvalState & state, Env & env, Value & v) { + if (elems.empty()) { + v = state.vEmptyList; + return; + } // 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. - v.mkList(ranges::fold_left(elems, state.allocList(), [&](const auto & list, const auto & elem) { - *list = list->push_back(elem->maybeThunk(state, env)); + v.mkList(ranges::fold_left(elems, state.allocList(), [&](auto * const list, auto * const elem) { + *list = std::move(*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 @@ -1505,13 +1512,13 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & auto size = (!lambda.arg ? 0 : 1) + (lambda.hasFormals() ? lambda.formals->formals.size() : 0); - Env & env2(allocEnv(size)); - env2.up = vCur.payload.lambda.env; + Env * env2(allocEnv(size)); + env2->up = vCur.payload.lambda.env; Displacement displ = 0; if (!lambda.hasFormals()) - env2.values[displ++] = args[0]; + env2->values[displ++] = args[0]; else { try { forceAttrs(*args[0], lambda.pos, "while evaluating the value passed for the lambda argument"); @@ -1521,7 +1528,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & } if (lambda.arg) - env2.values[displ++] = args[0]; + env2->values[displ++] = args[0]; /* For each formal argument, get the actual argument. If there is no matching actual argument but the formal @@ -1539,10 +1546,10 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & .withFrame(*fun.payload.lambda.env, lambda) .debugThrow(); } - env2.values[displ++] = i.def->maybeThunk(*this, env2); + env2->values[displ++] = i.def->maybeThunk(*this, *env2); } else { attrsUsed++; - env2.values[displ++] = j->value; + env2->values[displ++] = j->value; } } @@ -1551,7 +1558,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & if (!lambda.formals->ellipsis && attrsUsed != args[0]->attrs()->size()) { /* Nope, so show the first unexpected argument to the user. */ - for (auto & i : *args[0]->attrs()) + for (const auto & i : *args[0]->attrs()) if (!lambda.formals->has(i.name)) { std::set formalNames; for (auto & formal : lambda.formals->formals) @@ -1577,14 +1584,14 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & try { auto dts = debugRepl ? makeDebugTraceStacker( - *this, *lambda.body, env2, positions[lambda.pos], + *this, *lambda.body, *env2, positions[lambda.pos], "while calling %s", lambda.name ? concatStrings("'", symbols[lambda.name], "'") : "anonymous lambda") : nullptr; - lambda.body->eval(*this, env2, vCur); + lambda.body->eval(*this, *env2, vCur); } catch (Error & e) { if (loggerSettings.showTrace.get()) { addErrorTrace( @@ -1800,11 +1807,11 @@ values, or passed explicitly with '--arg' or '--argstr'. See void ExprWith::eval(EvalState & state, Env & env, Value & v) { - Env & env2(state.allocEnv(1)); - env2.up = &env; - env2.values[0] = attrs->maybeThunk(state, env); + Env * env2(state.allocEnv(1)); + env2->up = &env; + env2->values[0] = attrs->maybeThunk(state, env); - body->eval(state, env2, v); + body->eval(state, *env2, v); } @@ -1921,32 +1928,43 @@ 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"); + Value v1; 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"); + Value v2; e2->eval(state, env, v2); + state.forceList(v2, pos, "in the right operand of the list concatenation operator"); - auto list = state.allocList(); - *list = v1->list() + v2->list(); - v.mkList(list); + if (v1.listSize() == 0) { + v = v2; + } else if (v2.listSize() == 0) { + v = v1; + } else { + auto * const list = state.allocList(); + *list = v1.list() + v2.list(); + v.mkList(list); + } } -void EvalState::concatLists(Value & v, const ValueList lists, const PosIdx pos, std::string_view errorCtx) +void EvalState::concatLists(Value & v, const ValueList & lists, const PosIdx pos, std::string_view errorCtx) { - // 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. // TODO(@connorbaker): Because concatenation is associative, we can reduce the number of concatenations by doing a tree-like // concatenation instead of a linear one. Consider using a binary search to do so. - v.mkList(immer::accumulate(lists, allocList(), [&](const auto & concatenated, const auto & list) { - nrListConcats++; - forceList(*list, pos, errorCtx); - *concatenated = *concatenated + list->list(); - return concatenated; - })); + const auto numElems = lists.size(); + if (numElems == 0) { + v = vEmptyList; + } else if (numElems == 1) { + auto *const head = lists.front(); + forceList(*head, pos, errorCtx); + v = *head; + } else { + nrListConcats += numElems - 1; + v.mkList(immer::accumulate(lists, allocList(), [&](auto * const concatenated, auto * const list) { + forceList(*list, pos, errorCtx); + *concatenated = std::move(*concatenated) + list->list(); + return concatenated; + })); + } } @@ -2089,7 +2107,7 @@ void EvalState::forceValueDeep(Value & v) forceValue(v, v.determinePos(noPos)); if (v.type() == nAttrs) { - for (auto & i : *v.attrs()) + for (const auto & i : *v.attrs()) { try { // If the value is a thunk, we're evaling. Otherwise no trace necessary. auto dts = debugRepl && i.value->isThunk() @@ -2102,14 +2120,16 @@ void EvalState::forceValueDeep(Value & v) addErrorTrace(e, i.pos, "while evaluating the attribute '%1%'", symbols[i.name]); throw; } +} } else if (v.isList()) { // TODO(@connorbaker): Replacing this with // immer::for_each(v.list(), [&](const auto & v) { recurse(*v); }); // Increases the heap size by a fair amount, potentially because of the lambda capture. - for (auto v2 : v.list()) + for (auto * const v2 : v.list()) { recurse(*v2); +} } }; @@ -2335,7 +2355,7 @@ BackedStringView EvalState::coerceToString( if (v.isList()) { std::string result; - for (auto [n, v2] : enumerate(v.list())) { + for (const auto [n, v2] : enumerate(v.list())) { try { result += *coerceToString(pos, *v2, context, "while evaluating one element of the list", @@ -2346,8 +2366,9 @@ BackedStringView EvalState::coerceToString( } if (n < v.listSize() - 1 /* !!! not quite correct */ - && (!v2->isList() || v2->listSize() != 0)) + && (!v2->isList() || v2->listSize() != 0)) { result += " "; +} } return result; } @@ -2587,6 +2608,7 @@ void EvalState::assertEqValues(Value & v1, Value & v2, const PosIdx pos, std::st ValuePrinter(*this, v2, errorPrintOptions)) .debugThrow(); } + // TODO(@connorbaker): Rewrite to optimize for Immer. for (size_t n = 0; n < v1.listSize(); ++n) { try { assertEqValues(*v1.list()[n], *v2.list()[n], pos, errorCtx); @@ -2738,15 +2760,19 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v case nNull: return true; - case nList: - return v1.listSize() == v2.listSize() + case nList: { + // TODO(@connorbaker): Cannot inline l1 and l2 because ranges::views::zip_with requires a borrowed range. + // If we do it inline, since v1.list() and v2.list() are rvalues, they're owned ranges. + const auto l1 = v1.list(); + const auto l2 = v2.list(); + return l1.size() == l2.size() && ranges::all_of( ranges::views::zip_with( - [&](const auto & a, const auto & b) { return eqValues(*a, *b, pos, errorCtx); }, - ranges::subrange(v1.list().begin(), v1.list().end()), - ranges::subrange(v2.list().begin(), v2.list().end())), + [&](auto * const a, auto * const b) { return eqValues(*a, *b, pos, errorCtx); }, + l1, + l2), std::identity{}); - + } case nAttrs: { /* If both sets denote a derivation (type = "derivation"), then compare their outPaths. */ diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 44cac8cfd65..05bf9c8493c 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -713,9 +713,9 @@ public: /** * Allocation primitives. */ - inline ValueList * allocList(); - inline Value * allocValue(); - inline Env & allocEnv(size_t size); + ValueList * allocList(); + Value * allocValue(); + Env * allocEnv(size_t size); Bindings * allocBindings(size_t capacity); @@ -775,7 +775,7 @@ public: const SingleDerivedPath & p, Value & v); - void concatLists(Value & v, const ValueList lists, const PosIdx pos, std::string_view errorCtx); + void concatLists(Value & v, const ValueList & lists, const PosIdx pos, std::string_view errorCtx); /** * Print statistics, if enabled. diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 15b7f6500fd..4d35fab6164 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -117,36 +117,40 @@ PackageInfo::Outputs PackageInfo::queryOutputs(bool withPaths, bool onlyOutputsT const Attr * i; if (attrs && (i = attrs->get(state->sOutputs))) { state->forceList(*i->value, i->pos, "while evaluating the 'outputs' attribute of a derivation"); - immer::for_each(i->value->list(), [&](const auto & elem) { + immer::for_each(i->value->list(), [&](auto * elem) { std::string output(state->forceStringNoCtx(*elem, i->pos, "while evaluating the name of an output of a derivation")); if (withPaths) { /* Evaluate the corresponding set. */ - auto out = attrs->get(state->symbols.create(output)); + const auto *out = attrs->get(state->symbols.create(output)); if (!out) return; // FIXME: throw error? state->forceAttrs(*out->value, i->pos, "while evaluating an output of a derivation"); /* And evaluate its ‘outPath’ attribute. */ - auto outPath = out->value->attrs()->get(state->sOutPath); + const auto *outPath = out->value->attrs()->get(state->sOutPath); if (!outPath) return; // FIXME: throw error? NixStringContext context; outputs.emplace(output, state->coerceToStorePath(outPath->pos, *outPath->value, context, "while evaluating an output path of a derivation")); - } else + } else { outputs.emplace(output, std::nullopt); + } }); - } else + } else { outputs.emplace("out", withPaths ? std::optional{queryOutPath()} : std::nullopt); + } } - if (!onlyOutputsToInstall || !attrs) + if (!onlyOutputsToInstall || (attrs == nullptr)) { return outputs; + } const Attr * i; if (attrs && (i = attrs->get(state->sOutputSpecified)) && state->forceBool(*i->value, i->pos, "while evaluating the 'outputSpecified' attribute of a derivation")) { Outputs result; auto out = outputs.find(queryOutputName()); - if (out == outputs.end()) + if (out == outputs.end()) { throw Error("derivation does not have output '%s'", queryOutputName()); + } result.insert(*out); return result; } @@ -213,9 +217,9 @@ bool PackageInfo::checkMeta(Value & v) } else if (v.type() == nAttrs) { if (v.attrs()->get(state->sOutPath)) return false; - for (auto & i : *v.attrs()) - if (!checkMeta(*i.value)) return false; - return true; + return ranges::all_of(*v.attrs(), [&](const auto & i) { + return checkMeta(*i.value); + }); } else return v.type() == nInt || v.type() == nBool || v.type() == nString || v.type() == nFloat; @@ -225,7 +229,7 @@ bool PackageInfo::checkMeta(Value & v) Value * PackageInfo::queryMeta(const std::string & name) { if (!getMeta()) return 0; - auto a = meta->get(state->symbols.create(name)); + const auto *a = meta->get(state->symbols.create(name)); if (!a || !checkMeta(*a->value)) return 0; return a->value; } diff --git a/src/libexpr/json-to-value.cc b/src/libexpr/json-to-value.cc index ade39d33b14..0206b5fac5e 100644 --- a/src/libexpr/json-to-value.cc +++ b/src/libexpr/json-to-value.cc @@ -59,8 +59,8 @@ class JSONSax : nlohmann::json_sax { ValueVector values; std::unique_ptr resolve(EvalState & state) override { - parent->value(state).mkList(ranges::fold_left(values, state.allocList(), [](const auto & list, const auto & v) { - *list = list->push_back(v); + parent->value(state).mkList(ranges::fold_left(values, state.allocList(), [](auto * const list, auto * const v) { + *list = std::move(*list).push_back(v); return list; })); return std::move(parent); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 179f397ead9..8f423ac3334 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -16,8 +16,12 @@ #include "fetch-to-store.hh" #include +#include +#include #include #include +#include +#include #include #include #include @@ -47,6 +51,8 @@ namespace nix { // TODO(@connorbaker): Rewrite functions involving lists to make use of persistence -- currently they're largely written // in a way that duplicates data. +// TODO(@connorbaker): Lift placeholder values out of lambdas and into the enclosing scope to avoid repeated construction/destruction. + /************************************************************* * Miscellaneous *************************************************************/ @@ -208,15 +214,15 @@ void derivationToValue(EvalState & state, const PosIdx pos, const SourcePath & p }); attrs.alloc(state.sName).mkString(drv.env["name"]); attrs.alloc(state.sOutputs) - .mkList(ranges::fold_left(drv.outputs, state.allocList(), [&](const auto & list, const auto & output) { + .mkList(ranges::fold_left(drv.outputs, state.allocList(), [&](auto * const list, const auto & output) { mkOutputString(state, attrs, storePath, output); - auto outputString = state.allocValue(); + auto * const outputString = state.allocValue(); outputString->mkString(output.first); - *list = list->push_back(outputString); + *list = std::move(*list).push_back(outputString); return list; })); - auto w = state.allocValue(); + auto *w = state.allocValue(); w->mkAttrs(attrs); if (!state.vImportedDrvToDerivation) { @@ -243,7 +249,7 @@ void derivationToValue(EvalState & state, const PosIdx pos, const SourcePath & p static void scopedImport(EvalState & state, const PosIdx pos, SourcePath & path, Value * vScope, Value & v) { state.forceAttrs(*vScope, pos, "while evaluating the first argument passed to builtins.scopedImport"); - Env * env = &state.allocEnv(vScope->attrs()->size()); + Env * env = state.allocEnv(vScope->attrs()->size()); env->up = &state.baseEnv; auto staticEnv = std::make_shared(nullptr, state.staticBaseEnv.get(), vScope->attrs()->size()); @@ -415,7 +421,7 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v) { state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.exec"); auto elems = args[0]->list(); - auto count = args[0]->listSize(); + auto count = elems.size(); if (count == 0) state.error("at least one argument to 'exec' required").atPos(pos).debugThrow(); NixStringContext context; @@ -423,7 +429,7 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v) "while evaluating the first element of the argument passed to builtins.exec", false, false).toOwned(); Strings commandArgs; - immer::for_each(args[0]->list().drop(1), [&](const auto & v) { + immer::for_each(elems.drop(1), [&](auto * const v) { commandArgs.push_back(state.coerceToString(pos, *v, context, "while evaluating an element of the argument passed to builtins.exec", false, false).toOwned()); @@ -435,7 +441,7 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v) } auto output = runProgram(program, true, commandArgs); - Expr * parsed; + Expr * parsed = nullptr; try { parsed = state.parseExprFromString(std::move(output), state.rootPath(CanonPath::root)); } catch (Error & e) { @@ -653,7 +659,7 @@ struct CompareValues return strcmp(v1->payload.path.path, v2->payload.path.path) < 0; case nList: // Lexicographic comparison - return ranges::lexicographical_compare(v1->list(), v2->list(), [&](const auto & a, const auto & b) { + return ranges::lexicographical_compare(v1->list(), v2->list(), [&](auto * const a, auto * const b) { // If they are not equal, return the result of comparing them return !state.eqValues(*a, *b, pos, errorCtx) && (*this)(a, b, "while comparing two list elements"); }); @@ -687,7 +693,7 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a state.forceAttrs(*args[0], noPos, "while evaluating the first argument passed to builtins.genericClosure"); /* Get the start set. */ - auto startSet = getAttr(state, state.sStartSet, args[0]->attrs(), "in the attrset passed as argument to builtins.genericClosure"); + const auto *const startSet = getAttr(state, state.sStartSet, args[0]->attrs(), "in the attrset passed as argument to builtins.genericClosure"); state.forceList(*startSet->value, noPos, "while evaluating the 'startSet' attribute passed as argument to builtins.genericClosure"); @@ -697,41 +703,41 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value * * a } /* Get the operator. */ - auto op = getAttr(state, state.sOperator, args[0]->attrs(), "in the attrset passed as argument to builtins.genericClosure"); + const auto *const op = getAttr(state, state.sOperator, args[0]->attrs(), "in the attrset passed as argument to builtins.genericClosure"); state.forceFunction(*op->value, noPos, "while evaluating the 'operator' attribute passed as argument to builtins.genericClosure"); /* Construct the closure by applying the operator to elements of `workSet', adding the result to `workSet', continuing until no new elements are found. */ - auto res = state.allocList(); + auto * const res = state.allocList(); // `doneKeys' doesn't need to be a GC root, because its values are // reachable from res. - auto cmp = CompareValues(state, noPos, "while comparing the `key` attributes of two genericClosure elements"); + const auto cmp = CompareValues(state, noPos, "while comparing the `key` attributes of two genericClosure elements"); std::set> doneKeys(cmp); + Value newElements; auto workSet = startSet->value->list(); while (!workSet.empty()) { - auto e = workSet.front(); - workSet = workSet.drop(1); + auto * e = workSet.front(); + workSet = std::move(workSet).drop(1); state.forceAttrs(*e, noPos, "while evaluating one of the elements generated by (or initially passed to) builtins.genericClosure"); - auto key = getAttr(state, state.sKey, e->attrs(), "in one of the attrsets generated by (or initially passed to) builtins.genericClosure"); + const auto *const key = getAttr(state, state.sKey, e->attrs(), "in one of the attrsets generated by (or initially passed to) builtins.genericClosure"); state.forceValue(*key->value, noPos); if (!doneKeys.insert(key->value).second) continue; - *res = res->push_back(e); + *res = std::move(*res).push_back(e); /* Call the `operator' function with `e' as argument. */ - Value newElements; state.callFunction(*op->value, 1, &e, newElements, noPos); state.forceList(newElements, noPos, "while evaluating the return value of the `operator` passed to builtins.genericClosure"); - immer::for_each(newElements.list(), [&](const auto &elem){ + immer::for_each(newElements.list(), [&](auto * const elem){ state.forceValue(*elem, noPos); }); /* Add the values returned by the operator to the work set. */ - workSet = workSet + newElements.list(); + workSet = std::move(workSet) + newElements.list(); } v.mkList(res); @@ -1320,7 +1326,7 @@ static void derivationStrictInternal( command-line arguments to the builder. */ else if (i->name == state.sArgs) { state.forceList(*i->value, pos, context_below); - immer::for_each(i->value->list(), [&](const auto & elem){ + immer::for_each(i->value->list(), [&](auto * const elem){ drv.args.push_back(state.coerceToString(pos, *elem, context, "while evaluating an element of the argument list", true).toOwned()); @@ -1351,7 +1357,7 @@ static void derivationStrictInternal( /* Require ‘outputs’ to be a list of strings. */ state.forceList(*i->value, pos, context_below); Strings ss; - immer::for_each(i->value->list(), [&](const auto & elem){ + immer::for_each(i->value->list(), [&](auto * const elem){ ss.emplace_back(state.forceStringNoCtx(*elem, pos, context_below)); }); handleOutputs(ss); @@ -1827,17 +1833,17 @@ static void prim_findFile(EvalState & state, const PosIdx pos, Value * * args, V state.forceList(*args[0], pos, "while evaluating the first argument passed to builtins.findFile"); LookupPath lookupPath; - immer::for_each(args[0]->list(), [&](const auto & v2){ + std::string prefix; + NixStringContext context; + immer::for_each(args[0]->list(), [&](auto * const v2){ state.forceAttrs(*v2, pos, "while evaluating an element of the list passed to builtins.findFile"); - std::string prefix; auto i = v2->attrs()->find(state.sPrefix); if (i != v2->attrs()->end()) prefix = state.forceStringNoCtx(*i->value, pos, "while evaluating the `prefix` attribute of an element of the list passed to builtins.findFile"); i = getAttr(state, state.sPath, v2->attrs(), "in an element of the __nixPath"); - NixStringContext context; auto path = state.coerceToString(pos, *i->value, context, "while evaluating the `path` attribute of an element of the list passed to builtins.findFile", false, false).toOwned(); @@ -2669,22 +2675,17 @@ static RegisterPrimOp primop_path({ static void prim_attrNames(EvalState & state, const PosIdx pos, Value * * args, Value & v) { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.attrNames"); + const auto *const attrs = args[0]->attrs(); - ValueVector vec; - vec.reserve(args[0]->attrs()->size()); - - for (const auto & attr : *args[0]->attrs()) { - auto v = state.allocValue(); - v->mkString(state.symbols[attr.name]); - vec.push_back(v); - } - - std::sort(vec.begin(), vec.end(), - [](Value * v1, Value * v2) { return strcmp(v1->c_str(), v2->c_str()) < 0; }); + const auto symbols = *attrs + | ranges::views::transform([&](const auto & attr) { return std::string_view(state.symbols[attr.name]); }) + | ranges::to + | ranges::actions::sort; - - v.mkList(ranges::fold_left(vec, state.allocList(), [](const auto & list, const auto & v) { - *list = list->push_back(v); + v.mkList(ranges::fold_left(symbols, state.allocList(), [&](auto * const list, const auto & symbol) { + auto * const v = state.allocValue(); + v->mkString(symbol); + *list = std::move(*list).push_back(v); return list; })); } @@ -2706,18 +2707,16 @@ static void prim_attrValues(EvalState & state, const PosIdx pos, Value * * args, { state.forceAttrs(*args[0], pos, "while evaluating the argument passed to builtins.attrValues"); - v.mkList(ranges::fold_left( - ranges::views::all(*args[0]->attrs()) - | ranges::views::transform([&](const auto & attr) { return &attr; }) - | ranges::to_vector + const auto attrs = *args[0]->attrs() + | ranges::to | ranges::actions::sort([&](const auto & a, const auto & b) { - return std::string_view(state.symbols[a->name]) < std::string_view(state.symbols[b->name]); - }), - state.allocList(), - [](const auto & list, const auto & v) { - *list = list->push_back(((Attr *) v)->value); - return list; - })); + return std::string_view(state.symbols[a.name]) < std::string_view(state.symbols[b.name]); + }); + + v.mkList(ranges::fold_left(attrs, state.allocList(), [](auto * const list, const auto & attr) { + *list = std::move(*list).push_back(attr.value); + return list; + })); } static RegisterPrimOp primop_attrValues({ @@ -2868,23 +2867,24 @@ static void prim_removeAttrs(EvalState & state, const PosIdx pos, Value * * args /* Get the attribute names to be removed. We keep them as Attrs instead of Symbols so std::set_difference can be used to remove them from attrs[0]. */ - // 64: large enough to fit the attributes of a derivation - boost::container::small_vector names; - names.reserve(args[1]->listSize()); - immer::for_each(args[1]->list(), [&](const auto & elem) { - state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs"); - names.emplace_back(state.symbols.create(elem->string_view()), nullptr); - }); - std::sort(names.begin(), names.end()); + const auto names = args[1]->list() + | std::views::transform([&](auto * const elem) { + state.forceStringNoCtx(*elem, pos, "while evaluating the values of the second argument passed to builtins.removeAttrs"); + return Attr(state.symbols.create(elem->string_view()), nullptr); + }) + | ranges::to + | ranges::actions::sort(std::less<>()); /* Copy all attributes not in that set. Note that we don't need to sort v.attrs because it's a subset of an already sorted vector. */ auto attrs = state.buildBindings(args[0]->attrs()->size()); - std::set_difference( - args[0]->attrs()->begin(), args[0]->attrs()->end(), - names.begin(), names.end(), - std::back_inserter(attrs)); + std::ranges::set_difference( + *args[0]->attrs(), + names, + std::back_inserter(attrs), + std::less<>() + ); v.mkAttrs(attrs.alreadySorted()); } @@ -2915,18 +2915,18 @@ static void prim_listToAttrs(EvalState & state, const PosIdx pos, Value * * args auto attrs = state.buildBindings(args[0]->listSize()); - std::set, traceable_allocator> seen; + std::set, traceable_allocator> seen; - immer::for_each(args[0]->list(), [&](const auto & v2) { + immer::for_each(args[0]->list(), [&](auto * const v2) { state.forceAttrs(*v2, pos, "while evaluating an element of the list passed to builtins.listToAttrs"); - auto j = getAttr(state, state.sName, v2->attrs(), "in a {name=...; value=...;} pair"); + const auto * const j = getAttr(state, state.sName, v2->attrs(), "in a {name=...; value=...;} pair"); - auto name = state.forceStringNoCtx(*j->value, j->pos, "while evaluating the `name` attribute of an element of the list passed to builtins.listToAttrs"); + const auto name = state.forceStringNoCtx(*j->value, j->pos, "while evaluating the `name` attribute of an element of the list passed to builtins.listToAttrs"); - auto sym = state.symbols.create(name); + const auto sym = state.symbols.create(name); if (seen.insert(sym).second) { - auto j2 = getAttr(state, state.sValue, v2->attrs(), "in a {name=...; value=...;} pair"); + const auto * const j2 = getAttr(state, state.sValue, v2->attrs(), "in a {name=...; value=...;} pair"); attrs.insert(sym, j2->value, j2->pos); } }); @@ -3047,10 +3047,11 @@ static void prim_catAttrs(EvalState & state, const PosIdx pos, Value * * args, V { auto attrName = state.symbols.create(state.forceStringNoCtx(*args[0], pos, "while evaluating the first argument passed to builtins.catAttrs")); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.catAttrs"); - v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](const auto & list, const auto & v2) { + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](auto * const list, auto * const v2) { state.forceAttrs(*v2, pos, "while evaluating an element in the list passed as second argument to builtins.catAttrs"); - if (auto i = v2->attrs()->get(attrName)) - *list = list->push_back(i->value); + if (const auto * const i = v2->attrs()->get(attrName)) { + *list = std::move(*list).push_back(i->value); + } return list; })); } @@ -3145,29 +3146,29 @@ static RegisterPrimOp primop_mapAttrs({ static void prim_zipAttrsWith(EvalState & state, const PosIdx pos, Value * * args, Value & v) { - std::map, traceable_allocator>> attrsSeen; + std::map, traceable_allocator>> attrsSeen; state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.zipAttrsWith"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.zipAttrsWith"); - immer::for_each(args[1]->list(), [&](const auto & vElem) { + immer::for_each(args[1]->list(), [&](auto * const vElem) { state.forceAttrs(*vElem, noPos, "while evaluating a value of the list passed as second argument to builtins.zipAttrsWith"); - for (const auto & attr : *vElem->attrs()) { + for (const auto attr : *vElem->attrs()) { attrsSeen.try_emplace(attr.name, state.allocList()); - *attrsSeen[attr.name] = attrsSeen[attr.name]->push_back(attr.value); + *attrsSeen[attr.name] = std::move(*attrsSeen[attr.name]).push_back(attr.value); } }); auto attrs = state.buildBindings(attrsSeen.size()); - for (const auto & [sym, list] : attrsSeen) { - auto name = state.allocValue(); + for (const auto [sym, list] : attrsSeen) { + auto * const name = state.allocValue(); name->mkString(state.symbols[sym]); - auto arg = state.allocValue(); + auto * const arg = state.allocValue(); arg->mkList(list); - auto call1 = state.allocValue(); + auto * const call1 = state.allocValue(); call1->mkApp(args[0], name); - auto call2 = state.allocValue(); + auto * const call2 = state.allocValue(); call2->mkApp(call1, arg); attrs.insert(sym, call2); } @@ -3288,7 +3289,7 @@ static void prim_tail(EvalState & state, const PosIdx pos, Value * * args, Value if (args[0]->listSize() == 0) state.error("'tail' called on an empty list").atPos(pos).debugThrow(); - auto tail = state.allocList(); + auto * const tail = state.allocList(); *tail = args[0]->list().drop(1); v.mkList(tail); } @@ -3321,10 +3322,10 @@ static void prim_map(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.map"); - v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](const auto & list, const auto & v) { - auto newV = state.allocValue(); + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](auto * const list, auto * const v) { + auto * const newV = state.allocValue(); newV->mkApp(args[0], v); - *list = list->push_back(newV); + *list = std::move(*list).push_back(newV); return list; })); } @@ -3359,11 +3360,12 @@ static void prim_filter(EvalState & state, const PosIdx pos, Value * * args, Val state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.filter"); - v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](const auto & list, const auto & arg) { - Value res; + Value res; + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](auto * const list, auto * const arg) { state.callFunction(*args[0], *arg, res, noPos); - if (state.forceBool(res, pos, "while evaluating the return value of the filtering function passed to builtins.filter")) - *list = list->push_back(arg); + if (state.forceBool(res, pos, "while evaluating the return value of the filtering function passed to builtins.filter")) { + *list = std::move(*list).push_back(arg); + } return list; })); } @@ -3383,7 +3385,7 @@ static void prim_elem(EvalState & state, const PosIdx pos, Value * * args, Value { state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.elem"); // TODO(@connorbaker): immer doesn't provide any_of. - v.mkBool(ranges::any_of(args[1]->list(), [&](const auto & elem) { + v.mkBool(ranges::any_of(args[1]->list(), [&](auto * const elem) { return state.eqValues(*args[0], *elem, pos, "while searching for the presence of the given element in the list"); })); } @@ -3488,7 +3490,7 @@ static void prim_any(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.any")); state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.any")); std::string_view errorCtx = "while evaluating the return value of the function passed to builtins.any"; - v.mkBool(ranges::any_of(args[1]->list(), [&](const auto & elem) { + v.mkBool(ranges::any_of(args[1]->list(), [&](auto * const elem) { state.callFunction(*args[0], *elem, v, pos); return state.forceBool(v, pos, errorCtx); })); @@ -3509,7 +3511,7 @@ static void prim_all(EvalState & state, const PosIdx pos, Value * * args, Value state.forceFunction(*args[0], pos, std::string("while evaluating the first argument passed to builtins.all")); state.forceList(*args[1], pos, std::string("while evaluating the second argument passed to builtins.all")); std::string_view errorCtx = "while evaluating the return value of the function passed to builtins.all"; - v.mkBool(ranges::all_of(args[1]->list(), [&](const auto & elem) { + v.mkBool(ranges::all_of(args[1]->list(), [&](auto * const elem) { state.callFunction(*args[0], *elem, v, pos); return state.forceBool(v, pos, errorCtx); })); @@ -3538,12 +3540,12 @@ static void prim_genList(EvalState & state, const PosIdx pos, Value * * args, Va // as evaluating map without accessing any values makes little sense. state.forceFunction(*args[0], noPos, "while evaluating the first argument passed to builtins.genList"); - v.mkList(ranges::fold_left(ranges::views::iota(size_t(0), len), state.allocList(), [&](const auto & list, const auto & n) { - auto arg = state.allocValue(); + v.mkList(ranges::fold_left(ranges::views::iota(size_t(0), len), state.allocList(), [&](auto * const list, const auto n) { + auto * const arg = state.allocValue(); arg->mkInt(n); - auto app = state.allocValue(); + auto * const app = state.allocValue(); app->mkApp(args[0], arg); - *list = list->push_back(app); + *list = std::move(*list).push_back(app); return list; })); } @@ -3581,18 +3583,19 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value ValueVector vec; vec.reserve(len); - immer::for_each(args[1]->list(), [&](const auto & v) { + immer::for_each(args[1]->list(), [&](auto * const v) { state.forceValue(*v, pos); vec.push_back(v); }); - auto comparator = [&](Value * a, Value * b) { + auto comparator = [&](Value * const a, Value * const b) { /* Optimization: if the comparator is lessThan, bypass callFunction. */ if (args[0]->isPrimOp()) { - auto ptr = args[0]->primOp()->fun.target(); - if (ptr && *ptr == prim_lessThan) + const auto * const ptr = args[0]->primOp()->fun.target(); + if ((ptr != nullptr) && *ptr == prim_lessThan) { return CompareValues(state, noPos, "while evaluating the ordering function passed to builtins.sort")(a, b); + } } Value * vs[] = {a, b}; @@ -3606,8 +3609,8 @@ static void prim_sort(EvalState & state, const PosIdx pos, Value * * args, Value resilient, but no guarantees... */ std::stable_sort(vec.begin(), vec.end(), comparator); - v.mkList(ranges::fold_left(vec, state.allocList(), [&](const auto & list, const auto & v) { - *list = list->push_back(v); + v.mkList(ranges::fold_left(vec, state.allocList(), [&](auto * const list, auto * const v) { + *list = std::move(*list).push_back(v); return list; })); } @@ -3638,17 +3641,19 @@ static void prim_partition(EvalState & state, const PosIdx pos, Value * * args, state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.partition"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.partition"); - auto rlist = state.allocList(); - auto wlist = state.allocList(); + auto * const rlist = state.allocList(); + auto * const wlist = state.allocList(); - immer::for_each(args[1]->list(), [&](const auto & v) { + Value res; + immer::for_each(args[1]->list(), [&](auto * const v) { state.forceValue(*v, pos); - Value res; state.callFunction(*args[0], *v, res, pos); - if (state.forceBool(res, pos, "while evaluating the return value of the partition function passed to builtins.partition")) - *rlist = rlist->push_back(v); - else - *wlist = wlist->push_back(v); + if (state.forceBool(res, pos, "while evaluating the return value of the partition function passed to builtins.partition")) { + *rlist = std::move(*rlist).push_back(v); + } + else { + *wlist = std::move(*wlist).push_back(v); + } }); auto attrs = state.buildBindings(2); @@ -3688,20 +3693,20 @@ static void prim_groupBy(EvalState & state, const PosIdx pos, Value * * args, Va state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.groupBy"); // TODO(@connorbaker): Rewrite and optimize this function. - std::map, traceable_allocator>> attrsSeen; + std::map, traceable_allocator>> attrsSeen; - immer::for_each(args[1]->list(), [&](const auto & vElem) { - Value res; + Value res; + immer::for_each(args[1]->list(), [&](auto * const vElem) { state.callFunction(*args[0], *vElem, res, pos); - auto name = state.forceStringNoCtx(res, pos, "while evaluating the return value of the grouping function passed to builtins.groupBy"); - auto sym = state.symbols.create(name); + const auto name = state.forceStringNoCtx(res, pos, "while evaluating the return value of the grouping function passed to builtins.groupBy"); + const auto sym = state.symbols.create(name); attrsSeen.try_emplace(sym, state.allocList()); - *attrsSeen[sym] = attrsSeen[sym]->push_back(vElem); + *attrsSeen[sym] = std::move(*attrsSeen[sym]).push_back(vElem); }); auto attrs = state.buildBindings(attrsSeen.size()); - ranges::for_each(attrsSeen, [&](const auto & pair) { + ranges::for_each(attrsSeen, [&](const auto pair) { attrs.alloc(pair.first).mkList(pair.second); }); v.mkAttrs(attrs.alreadySorted()); @@ -3735,12 +3740,11 @@ static void prim_concatMap(EvalState & state, const PosIdx pos, Value * * args, { state.forceFunction(*args[0], pos, "while evaluating the first argument passed to builtins.concatMap"); state.forceList(*args[1], pos, "while evaluating the second argument passed to builtins.concatMap"); - v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](const auto & list, const auto & v) { - // TODO(@connorbaker): Will the list stored in res be available after the loop, or will it be garbage collected? - Value res; + Value res; + v.mkList(immer::accumulate(args[1]->list(), state.allocList(), [&](auto * const list, auto * const v) { state.callFunction(*args[0], *v, res, pos); state.forceList(res, res.determinePos(args[0]->determinePos(pos)), "while evaluating the return value of the function passed to builtins.concatMap"); - *list = *list + res.list(); + *list = std::move(*list) + res.list(); return list; })); } @@ -4234,8 +4238,8 @@ void prim_match(EvalState & state, const PosIdx pos, Value * * args, Value & v) } // the first match is the whole string - v.mkList(ranges::fold_left(std::span(matches).subspan(1), state.allocList(), [&](const auto & listOfMatches, const auto & match) { - *listOfMatches = listOfMatches->push_back(match.matched ? mkString(state, match) : &state.vNull); + v.mkList(ranges::fold_left(std::span(matches).subspan(1), state.allocList(), [&](auto * const listOfMatches, const auto match) { + *listOfMatches = std::move(*listOfMatches).push_back(match.matched ? mkString(state, match) : &state.vNull); return listOfMatches; })); @@ -4307,9 +4311,9 @@ void prim_split(EvalState & state, const PosIdx pos, Value * * args, Value & v) // Any matches results are surrounded by non-matching results. const size_t len = std::distance(begin, end); - auto listOfMatches = state.allocList(); + auto * const listOfMatches = state.allocList(); if (len == 0) { - *listOfMatches = listOfMatches->push_back(args[1]); + *listOfMatches = std::move(*listOfMatches).push_back(args[1]); v.mkList(listOfMatches); return; } @@ -4318,32 +4322,29 @@ void prim_split(EvalState & state, const PosIdx pos, Value * * args, Value & v) for (auto i = begin; i != end; ++i) { assert(idx <= 2 * len + 1 - 3); - auto match = *i; + const auto match = *i; // Add a string for non-matched characters. - *listOfMatches = listOfMatches->push_back(mkString(state, match.prefix())); + *listOfMatches = std::move(*listOfMatches).push_back(mkString(state, match.prefix())); idx++; // Add a list for matched substrings. - auto listOfSubMatches = state.allocList(); + auto * const listOfSubMatches = state.allocList(); // NOTE: Start at 1, beacause the first match is the whole string. - for (auto subMatch : std::span(match).subspan(1)) { - if (!subMatch.matched) { - *listOfSubMatches = listOfSubMatches->push_back(&state.vNull); - } else { - *listOfSubMatches = listOfSubMatches->push_back(mkString(state, subMatch)); - } - } + for (const auto subMatch : std::span(match).subspan(1)) + *listOfSubMatches = std::move(*listOfSubMatches).push_back(subMatch.matched + ? mkString(state, subMatch) + : &state.vNull); - auto scratchValue = state.allocValue(); + auto * const scratchValue = state.allocValue(); scratchValue->mkList(listOfSubMatches); - *listOfMatches = listOfMatches->push_back(scratchValue); + *listOfMatches = std::move(*listOfMatches).push_back(scratchValue); idx++; // Add a string for non-matched suffix characters. if (idx == 2 * len) { - *listOfMatches = listOfMatches->push_back(mkString(state, match.suffix())); + *listOfMatches = std::move(*listOfMatches).push_back(mkString(state, match.suffix())); idx++; } } @@ -4445,8 +4446,9 @@ static void prim_replaceStrings(EvalState & state, const PosIdx pos, Value * * a std::vector from; from.reserve(args[0]->listSize()); - for (auto elem : args[0]->list()) - from.emplace_back(state.forceString(*elem, pos, "while evaluating one of the strings to replace passed to builtins.replaceStrings")); + immer::for_each(args[0]->list(), [&](auto * const elem) { + from.emplace_back(state.forceStringNoCtx(*elem, pos, "while evaluating one of the strings to replace passed to builtins.replaceStrings")); + }); std::unordered_map cache; auto to = args[1]->list(); @@ -4575,13 +4577,12 @@ static void prim_splitVersion(EvalState & state, const PosIdx pos, Value * * arg components.emplace_back(component); } - auto list = state.allocList(); - for (const auto & component : components) { - auto v = state.allocValue(); - v->mkString(std::move(component)); - *list = list->push_back(v); - } - v.mkList(list); + v.mkList(ranges::fold_left(components, state.allocList(), [&](auto * const list, const auto component) { + auto * const v = state.allocValue(); + v->mkString(component); + *list = std::move(*list).push_back(std::move(v)); + return list; + })); } static RegisterPrimOp primop_splitVersion({ @@ -4823,14 +4824,14 @@ void EvalState::createBaseEnv() }); /* Add a value containing the current Nix expression search path. */ - auto list = allocList(); + auto * const list = allocList(); for (const auto & i : lookupPath.elements) { auto attrs = buildBindings(2); attrs.alloc("path").mkString(i.path.s); attrs.alloc("prefix").mkString(i.prefix.s); - auto v = allocValue(); + auto * const v = allocValue(); v->mkAttrs(attrs); - *list = list->push_back(v); + *list = std::move(*list).push_back(std::move(v)); } v.mkList(list); addConstant("__nixPath", v, { From 803b98c298aedceff983cce1fb4750d88b513892 Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Thu, 14 Nov 2024 05:43:54 +0000 Subject: [PATCH 8/9] wip --- .clang-tidy | 16 +++++++++++++--- packaging/dev-shell.nix | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 0887b867087..6092c4e4678 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,3 +1,13 @@ -# We use pointers to aggregates in a couple of places, intentionally. -# void * would look weird. -Checks: '-bugprone-sizeof-expression' +Checks: + - bugprone-* + - clang-analyzer-* + - cppcoreguidelines-* + - modernize-* + - -modernize-use-trailing-return-type + - performance-* + - portability-* + - readability-* + - -readability-identifier-length +FormatStyle: file +InheritParentConfig: true +User: connorbaker \ No newline at end of file diff --git a/packaging/dev-shell.nix b/packaging/dev-shell.nix index b64dc0858fb..b30f1f0d830 100644 --- a/packaging/dev-shell.nix +++ b/packaging/dev-shell.nix @@ -113,7 +113,7 @@ in { # https://github.com/NixOS/nixpkgs/pull/291814 is available ++ lib.optional (stdenv.cc.isClang && !stdenv.buildPlatform.isDarwin) pkgs.buildPackages.bear ++ lib.optional (stdenv.cc.isClang && stdenv.hostPlatform == stdenv.buildPlatform) [ - (lib.hiPrio pkgs.buildPackages.clang-tools-18) + (lib.hiPrio pkgs.buildPackages.clang-tools_18) pkgs.buildPackages.llvmPackages_18.lldb ]; From f742092d6a9131d81daf6dd836ce415aee31ba75 Mon Sep 17 00:00:00 2001 From: Connor Baker Date: Sat, 16 Nov 2024 07:41:11 +0000 Subject: [PATCH 9/9] 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 7a879cdda6c..b37ac44b317 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 4539f7ce46e..8ef0acfc235 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 9c696bc1a82..c54a942534b 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 4d35fab6164..7ffbdf247d4 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 8f423ac3334..8ebf03286c4 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);