Merge pull request #13987 from xokdvium/bindings-bananza

libexpr: Structural sharing of attrsets

Author: roberth

doc/manual/rl-next/c-api-byidx.md

@@ -0,0 +1,7 @@
+---
+synopsis: "C API: `nix_get_attr_name_byidx`, `nix_get_attr_byidx` take a `nix_value *` instead of `const nix_value *`"
+prs: [13987]
+---
+
+In order to accommodate a more optimized internal representation of attribute set merges these functions require
+a mutable `nix_value *` that might be modified on access. This does *not* break the ABI of these functions.

src/libexpr-c/nix_api_value.cc

@@ -371,13 +371,24 @@ bool nix_has_attr_byname(nix_c_context * context, const nix_value * value, EvalS
     NIXC_CATCH_ERRS_RES(false);
 }
 
-nix_value * nix_get_attr_byidx(
-    nix_c_context * context, const nix_value * value, EvalState * state, unsigned int i, const char ** name)
+static void collapse_attrset_layer_chain_if_needed(nix::Value & v, EvalState * state)
+{
+    auto & attrs = *v.attrs();
+    if (attrs.isLayered()) {
+        auto bindings = state->state.buildBindings(attrs.size());
+        std::ranges::copy(attrs, std::back_inserter(bindings));
+        v.mkAttrs(bindings);
+    }
+}
+
+nix_value *
+nix_get_attr_byidx(nix_c_context * context, nix_value * value, EvalState * state, unsigned int i, const char ** name)
 {
     if (context)
         context->last_err_code = NIX_OK;
     try {
         auto & v = check_value_in(value);
+        collapse_attrset_layer_chain_if_needed(v, state);
         const nix::Attr & a = (*v.attrs())[i];
         *name = state->state.symbols[a.name].c_str();
         nix_gc_incref(nullptr, a.value);
@@ -387,13 +398,13 @@ nix_value * nix_get_attr_byidx(
     NIXC_CATCH_ERRS_NULL
 }
 
-const char *
-nix_get_attr_name_byidx(nix_c_context * context, const nix_value * value, EvalState * state, unsigned int i)
+const char * nix_get_attr_name_byidx(nix_c_context * context, nix_value * value, EvalState * state, unsigned int i)
 {
     if (context)
         context->last_err_code = NIX_OK;
     try {
         auto & v = check_value_in(value);
+        collapse_attrset_layer_chain_if_needed(v, state);
         const nix::Attr & a = (*v.attrs())[i];
         return state->state.symbols[a.name].c_str();
     }

src/libexpr-c/nix_api_value.h

@@ -297,8 +297,8 @@ bool nix_has_attr_byname(nix_c_context * context, const nix_value * value, EvalS
  * @param[out] name will store a pointer to the attribute name
  * @return value, NULL in case of errors
  */
-nix_value * nix_get_attr_byidx(
-    nix_c_context * context, const nix_value * value, EvalState * state, unsigned int i, const char ** name);
+nix_value *
+nix_get_attr_byidx(nix_c_context * context, nix_value * value, EvalState * state, unsigned int i, const char ** name);
 
 /** @brief Get an attribute name by index in the sorted bindings
  *
@@ -311,8 +311,7 @@ nix_value * nix_get_attr_byidx(
  * @param[in] i attribute index
  * @return name, NULL in case of errors
  */
-const char *
-nix_get_attr_name_byidx(nix_c_context * context, const nix_value * value, EvalState * state, unsigned int i);
+const char * nix_get_attr_name_byidx(nix_c_context * context, nix_value * value, EvalState * state, unsigned int i);
 
 /**@}*/
 /** @name Initializers

src/libexpr-tests/nix_api_expr.cc

@@ -437,4 +437,31 @@ TEST_F(nix_api_expr_test, nix_value_call_multi_no_args)
     assert_ctx_ok();
     ASSERT_EQ(3, rInt);
 }
+
+TEST_F(nix_api_expr_test, nix_expr_attrset_update)
+{
+    nix_expr_eval_from_string(ctx, state, "{ a = 0; b = 2; } // { a = 1; b = 3; } // { a = 2; }", ".", value);
+    assert_ctx_ok();
+
+    ASSERT_EQ(nix_get_attrs_size(ctx, value), 2);
+    assert_ctx_ok();
+    std::array<std::pair<std::string_view, nix_value *>, 2> values;
+    for (unsigned int i = 0; i < 2; ++i) {
+        const char * name;
+        values[i].second = nix_get_attr_byidx(ctx, value, state, i, &name);
+        assert_ctx_ok();
+        values[i].first = name;
+    }
+    std::sort(values.begin(), values.end(), [](const auto & lhs, const auto & rhs) { return lhs.first < rhs.first; });
+
+    nix_value * a = values[0].second;
+    ASSERT_EQ("a", values[0].first);
+    ASSERT_EQ(nix_get_int(ctx, a), 2);
+    assert_ctx_ok();
+    nix_value * b = values[1].second;
+    ASSERT_EQ("b", values[1].first);
+    ASSERT_EQ(nix_get_int(ctx, b), 3);
+    assert_ctx_ok();
+}
+
 } // namespace nixC

src/libexpr/attr-set.cc

@@ -14,7 +14,7 @@ Bindings * EvalState::allocBindings(size_t capacity)
 {
     if (capacity == 0)
         return &Bindings::emptyBindings;
-    if (capacity > std::numeric_limits<Bindings::size_t>::max())
+    if (capacity > std::numeric_limits<Bindings::size_type>::max())
         throw Error("attribute set of size %d is too big", capacity);
     nrAttrsets++;
     nrAttrsInAttrsets += capacity;
@@ -35,7 +35,7 @@ Value & BindingsBuilder::alloc(std::string_view name, PosIdx pos)
 
 void Bindings::sort()
 {
-    std::sort(attrs, attrs + size_);
+    std::sort(attrs, attrs + numAttrs);
 }
 
 Value & Value::mkAttrs(BindingsBuilder & bindings)

src/libexpr/eval.cc

@@ -1873,37 +1873,71 @@ void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v)
 
     state.nrOpUpdates++;
 
-    if (v1.attrs()->size() == 0) {
+    const Bindings & bindings1 = *v1.attrs();
+    if (bindings1.empty()) {
         v = v2;
         return;
     }
-    if (v2.attrs()->size() == 0) {
+
+    const Bindings & bindings2 = *v2.attrs();
+    if (bindings2.empty()) {
         v = v1;
         return;
     }
 
-    auto attrs = state.buildBindings(v1.attrs()->size() + v2.attrs()->size());
+    /* Simple heuristic for determining whether attrs2 should be "layered" on top of
+       attrs1 instead of copying to a new Bindings. */
+    bool shouldLayer = [&]() -> bool {
+        if (bindings1.isLayerListFull())
+            return false;
+
+        if (bindings2.size() > state.settings.bindingsUpdateLayerRhsSizeThreshold)
+            return false;
+
+        return true;
+    }();
+
+    if (shouldLayer) {
+        auto attrs = state.buildBindings(bindings2.size());
+        attrs.layerOnTopOf(bindings1);
+
+        std::ranges::copy(bindings2, std::back_inserter(attrs));
+        v.mkAttrs(attrs.alreadySorted());
+
+        state.nrOpUpdateValuesCopied += bindings2.size();
+        return;
+    }
+
+    auto attrs = state.buildBindings(bindings1.size() + bindings2.size());
 
     /* Merge the sets, preferring values from the second set.  Make
        sure to keep the resulting vector in sorted order. */
-    auto i = v1.attrs()->begin();
-    auto j = v2.attrs()->begin();
+    auto i = bindings1.begin();
+    auto j = bindings2.begin();
 
-    while (i != v1.attrs()->end() && j != v2.attrs()->end()) {
+    while (i != bindings1.end() && j != bindings2.end()) {
         if (i->name == j->name) {
             attrs.insert(*j);
             ++i;
             ++j;
-        } else if (i->name < j->name)
-            attrs.insert(*i++);
-        else
-            attrs.insert(*j++);
+        } else if (i->name < j->name) {
+            attrs.insert(*i);
+            ++i;
+        } else {
+            attrs.insert(*j);
+            ++j;
+        }
     }
 
-    while (i != v1.attrs()->end())
-        attrs.insert(*i++);
-    while (j != v2.attrs()->end())
-        attrs.insert(*j++);
+    while (i != bindings1.end()) {
+        attrs.insert(*i);
+        ++i;
+    }
+
+    while (j != bindings2.end()) {
+        attrs.insert(*j);
+        ++j;
+    }
 
     v.mkAttrs(attrs.alreadySorted());