From 92149d827cf9506dbd00fe721fe7998006fd8448 Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Fri, 3 Jul 2026 11:56:42 +0200
Subject: [PATCH] zipAttrsWith: Avoid GC-visible temporary storage

The previous implementation used a std::map with traceable_allocator,
which costs one GC_MALLOC_UNCOLLECTABLE() and one GC_FREE() per
distinct attribute name. Uncollectable allocations always take the
global GC allocation lock, making this map a major source of mutex
contention during parallel evaluation (~12% of all futex calls in a
16-core 'nix search' run, plus the corresponding GC_free traffic).

Instead, collect the (name, value) pairs into a plain std::vector and
group them by stable-sorting on the name, which yields the same
attribute order and per-attribute value order as the map. The vector
doesn't need to be visible to the GC since the values it points to
are kept alive by the attrsets in args[1] for the duration of the
call.

At 16 eval cores (with GC disabled via GC_INITIAL_HEAP_SIZE=40G),
this cuts kernel time from ~21s to ~8-9s, context switches from 2.6M
to 1.5M, and elapsed time from ~6.2s to ~4.9s for 'nix search nixpkgs
--no-eval-cache fizzbuzz'. Single-core performance is unchanged.

Assisted-by: Claude Fable 5 <noreply@anthropic.com>
---
 src/libexpr/primops.cc | 69 +++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 28 deletions(-)

diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index ff71c7640024..ab828e8b6522 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -3700,54 +3700,67 @@ static RegisterPrimOp primop_filterAttrs({
 
 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
+    // We collect all (name, value) pairs into a vector and group them by
+    // stable-sorting on the name, which yields the same attribute order and
+    // per-attribute value order as inserting into a std::map<Symbol, ...>.
+    // Note that it's fine that the vector is invisible to the GC: the values
+    // it points to are kept alive by the attrsets in *args[1], which is
+    // reachable for the duration of this function. This avoids allocating
+    // GC-visible (uncollectable) storage for temporaries, which is expensive
+    // and a source of GC allocation lock contention during parallel
+    // evaluation.
+
+    struct NameValue
     {
-        size_t size = 0;
-        size_t pos = 0;
-        std::optional<ListBuilder> list;
+        Symbol name;
+        Value * value;
     };
 
-    std::map<Symbol, Item, std::less<Symbol>, traceable_allocator<std::pair<const Symbol, Item>>> 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]->listView();
 
+    size_t nrAttrs = 0;
     for (auto & vElem : listItems) {
         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;
-        }
+        nrAttrs += vElem->attrs()->size();
     }
 
-    auto attrs = state.buildBindings(attrsSeen.size());
+    std::vector<NameValue> attrsSeen;
+    attrsSeen.reserve(nrAttrs);
 
-    for (auto & [sym, elem] : attrsSeen) {
+    for (auto & vElem : listItems)
+        for (auto & attr : *vElem->attrs())
+            attrsSeen.push_back({attr.name, attr.value});
+
+    std::stable_sort(
+        attrsSeen.begin(), attrsSeen.end(), [](const NameValue & a, const NameValue & b) { return a.name < b.name; });
+
+    size_t nrNames = 0;
+    for (size_t i = 0; i < attrsSeen.size(); ++i)
+        if (i == 0 || attrsSeen[i - 1].name != attrsSeen[i].name)
+            nrNames++;
+
+    auto attrs = state.buildBindings(nrNames);
+
+    for (size_t i = 0; i < attrsSeen.size();) {
+        auto sym = attrsSeen[i].name;
+        size_t j = i;
+        while (j < attrsSeen.size() && attrsSeen[j].name == sym)
+            j++;
+        auto list = state.buildList(j - i);
+        for (size_t k = i; k < j; ++k)
+            list[k - i] = attrsSeen[k].value;
         auto name = Value::toPtr(state.symbols[sym]);
         auto call1 = state.allocValue();
         call1->mkApp(args[0], name);
         auto call2 = state.allocValue();
         auto arg = state.allocValue();
-        arg->mkList(*elem.list);
+        arg->mkList(list);
         call2->mkApp(call1, arg);
         attrs.insert(sym, call2);
+        i = j;
     }
 
     v.mkAttrs(attrs.alreadySorted());