Allocate RootValues more efficiently

src/libcmd/include/nix/cmd/installable-attr-path.hh

@@ -20,6 +20,7 @@
 
 #include <regex>
 #include <queue>
+#include "nix/expr/root-value.hh"
 
 namespace nix {
 

src/libcmd/installable-attr-path.cc

@@ -21,7 +21,7 @@ InstallableAttrPath::InstallableAttrPath(
     ExtendedOutputsSpec extendedOutputsSpec)
     : InstallableValue(state)
     , cmd(cmd)
-    , v(allocRootValue(v))
+    , v(RootValue(v))
     , attrPath(attrPath)
     , extendedOutputsSpec(std::move(extendedOutputsSpec))
 {

src/libcmd/repl.cc

@@ -344,7 +344,7 @@ void NixRepl::loadDebugTraceEnv(DebugTrace & dt)
 
         // add staticenv vars.
         for (auto & [name, value] : *(vm.get()))
-            addVarToScope(state->symbols.create(name), *value);
+            addVarToScope(state->symbols.create(name), **value);
     }
 }
 
@@ -954,7 +954,7 @@ ReplExitStatus AbstractNixRepl::runSimple(ref<EvalState> evalState, const ValMap
 
     // add 'extra' vars.
     for (auto & [name, value] : extraEnv)
-        repl->addVarToScope(repl->state->symbols.create(name), *value);
+        repl->addVarToScope(repl->state->symbols.create(name), **value);
 
     return repl->mainLoop();
 }

src/libexpr/eval-cache.cc

@@ -363,7 +363,7 @@ Value * EvalCache::getRootValue()
 {
     if (!value) {
         debug("getting root value");
-        value = allocRootValue(rootLoader());
+        value = RootValue(rootLoader());
     }
     return *value;
 }
@@ -380,7 +380,7 @@ AttrCursor::AttrCursor(
     , cachedValue(std::move(cachedValue))
 {
     if (value)
-        _value = allocRootValue(value);
+        _value = RootValue(value);
 }
 
 AttrKey AttrCursor::getKey()
@@ -403,9 +403,9 @@ Value & AttrCursor::getValue()
             auto attr = vParent.attrs()->get(parent->second);
             if (!attr)
                 throw Error("attribute '%s' is unexpectedly missing", getAttrPathStr());
-            _value = allocRootValue(attr->value);
+            _value = RootValue(attr->value);
         } else
-            _value = allocRootValue(root->getRootValue());
+            _value = RootValue(root->getRootValue());
     }
     return **_value;
 }

src/libexpr/eval.cc

@@ -104,11 +104,6 @@ const StringData & StringData::make(EvalMemory & mem, std::string_view s)
     return res;
 }
 
-RootValue allocRootValue(Value * v)
-{
-    return std::allocate_shared<Value *>(traceable_allocator<Value *>(), v);
-}
-
 // Pretty print types for assertion errors
 std::ostream & operator<<(std::ostream & os, const ValueType t)
 {
@@ -564,7 +559,7 @@ Value * EvalState::addPrimOp(PrimOp && primOp)
     v->mkPrimOp(new PrimOp(primOp));
 
     if (primOp.internal)
-        internalPrimOps.emplace(primOp.name, v);
+        internalPrimOps.emplace(primOp.name, RootValue(v));
     else {
         staticBaseEnv->vars.emplace_back(envName, baseEnvDispl);
         baseEnv.values[baseEnvDispl++] = v;
@@ -745,11 +740,11 @@ void mapStaticEnvBindings(const SymbolTable & st, const StaticEnv & se, const En
         if (se.isWith && !env.values[0]->isThunk()) {
             // add 'with' bindings.
             for (auto & j : *env.values[0]->attrs())
-                vm.insert_or_assign(std::string(st[j.name]), j.value);
+                vm.insert_or_assign(std::string(st[j.name]), RootValue(j.value));
         } else {
             // iterate through staticenv bindings and add them.
             for (auto & i : se.vars)
-                vm.insert_or_assign(std::string(st[i.first]), env.values[i.second]);
+                vm.insert_or_assign(std::string(st[i.first]), RootValue(env.values[i.second]));
         }
     }
 }
@@ -1155,10 +1150,14 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
         importResolutionCache->emplace(path, *resolvedPath);
     }
 
-    if (auto v2 = getConcurrent(*fileEvalCache, *resolvedPath)) {
-        forceValue(**v2, noPos);
-        v = **v2;
-        return;
+    {
+        Value * v2 = nullptr;
+        fileEvalCache->cvisit(*resolvedPath, [&](auto & i) { v2 = *i.second; });
+        if (v2) {
+            forceValue(*v2, noPos);
+            v = *v2;
+            return;
+        }
     }
 
     Value * vExpr;
@@ -1170,13 +1169,13 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
 
     fileEvalCache->try_emplace_and_cvisit(
         *resolvedPath,
-        nullptr,
+        RootValue(nullptr),
         [&](auto & i) {
             vExpr = allocValue();
             vExpr->mkThunk(&baseEnv, expr);
-            i.second = vExpr;
+            *i.second = vExpr;
         },
-        [&](auto & i) { vExpr = i.second; });
+        [&](auto & i) { vExpr = *i.second; });
 
     forceValue(*vExpr, noPos);
 

src/libexpr/include/nix/expr/eval-cache.hh

@@ -8,6 +8,7 @@
 
 #include <functional>
 #include <variant>
+#include "nix/expr/root-value.hh"
 
 namespace nix::eval_cache {
 

src/libexpr/include/nix/expr/eval.hh

@@ -6,6 +6,7 @@
 #include "nix/expr/eval-profiler.hh"
 #include "nix/util/types.hh"
 #include "nix/expr/value.hh"
+#include "nix/expr/root-value.hh"
 #include "nix/expr/nixexpr.hh"
 #include "nix/expr/symbol-table.hh"
 #include "nix/util/configuration.hh"
@@ -165,9 +166,7 @@ struct Constant
     bool impureOnly = false;
 };
 
-typedef std::
-    map<std::string, Value *, std::less<std::string>, traceable_allocator<std::pair<const std::string, Value *>>>
-        ValMap;
+typedef std::map<std::string, RootValue> ValMap;
 
 typedef boost::unordered_flat_map<PosIdx, DocComment, std::hash<PosIdx>> DocCommentMap;
 
@@ -468,13 +467,7 @@ private:
     /**
      * A cache from resolved paths to values.
      */
-    const ref<boost::concurrent_flat_map<
-        SourcePath,
-        Value *,
-        std::hash<SourcePath>,
-        std::equal_to<SourcePath>,
-        traceable_allocator<std::pair<const SourcePath, Value *>>>>
-        fileEvalCache;
+    const ref<boost::concurrent_flat_map<SourcePath, RootValue>> fileEvalCache;
 
     /**
      * Associate source positions of certain AST nodes with their preceding doc comment, if they have one.
@@ -833,13 +826,7 @@ public:
     /**
      * Internal primops not exposed to the user.
      */
-    boost::unordered_flat_map<
-        std::string,
-        Value *,
-        StringViewHash,
-        std::equal_to<>,
-        traceable_allocator<std::pair<const std::string, Value *>>>
-        internalPrimOps;
+    boost::unordered_flat_map<std::string, RootValue, StringViewHash> internalPrimOps;
 
     /**
      * Name and documentation about every constant.

src/libexpr/include/nix/expr/get-drvs.hh

@@ -32,6 +32,7 @@ private:
      */
     bool failed = false;
 
+    // FIXME: make this a RootValue.
     const Bindings *attrs = nullptr, *meta = nullptr;
 
     const Bindings * getMeta();

src/libexpr/include/nix/expr/meson.build

@@ -32,6 +32,7 @@ headers = [ config_pub_h ] + files(
   'print-options.hh',
   'print.hh',
   'repl-exit-status.hh',
+  'root-value.hh',
   'search-path.hh',
   'static-string-data.hh',
   'symbol-table.hh',

src/libexpr/include/nix/expr/root-value.hh

@@ -0,0 +1,78 @@
+#pragma once
+///@file
+
+#include <memory>
+#include <utility>
+
+namespace nix {
+
+struct Value;
+
+/**
+ * A move-only handle rooting a Value, i.e. keeping it and everything
+ * reachable from it alive across garbage collections. Prefer this
+ * over `RootValue` unless the handle must be copyable (e.g. when it's
+ * captured in a `std::function`-backed lambda).
+ */
+class RootValue
+{
+    Value ** slot = nullptr;
+
+    /**
+     * Clear the given slot and return it to the root value pool.
+     */
+    void freeRootValueSlot();
+
+public:
+    RootValue() = default;
+
+    /**
+     * Allocate a slot from the root value pool, i.e. a GC-visible
+     * `Value *` cell that keeps the value it points to alive across
+     * garbage collections. Use `RootValue`/`RootValue` rather than
+     * calling this directly.
+     */
+    explicit RootValue(Value * v);
+
+    RootValue(const RootValue &) = delete;
+    RootValue & operator=(const RootValue &) = delete;
+
+    RootValue(RootValue && other) noexcept
+        : slot(std::exchange(other.slot, nullptr))
+    {
+    }
+
+    RootValue & operator=(RootValue && other) noexcept
+    {
+        if (slot)
+            freeRootValueSlot();
+        slot = std::exchange(other.slot, nullptr);
+        return *this;
+    }
+
+    ~RootValue()
+    {
+        reset();
+    }
+
+    /**
+     * Release the slot, i.e. stop rooting the value.
+     */
+    void reset()
+    {
+        if (slot)
+            freeRootValueSlot();
+    }
+
+    Value *& operator*() const
+    {
+        return *slot;
+    }
+
+    explicit operator bool() const
+    {
+        return slot != nullptr;
+    }
+};
+
+} // namespace nix

src/libexpr/include/nix/expr/value.hh

@@ -9,6 +9,7 @@
 #include <memory_resource>
 #include <exception>
 #include <span>
+#include <utility>
 #include <string_view>
 #include <type_traits>
 #include <concepts>
@@ -1576,12 +1577,5 @@ typedef boost::unordered_flat_map<
 typedef std::map<Symbol, ValueVector, std::less<Symbol>, traceable_allocator<std::pair<const Symbol, ValueVector>>>
     ValueVectorMap;
 
-/**
- * A value allocated in traceable memory.
- */
-typedef std::shared_ptr<Value *> RootValue;
-
-RootValue allocRootValue(Value * v);
-
 void forceNoNullByte(std::string_view s, std::function<Pos()> = nullptr);
 } // namespace nix

src/libexpr/json-to-value.cc

@@ -32,7 +32,7 @@ class JSONSax : nlohmann::json_sax<json>
         }
 
         explicit JSONState(Value * v)
-            : v(allocRootValue(v))
+            : v(RootValue(v))
         {
         }
 
@@ -41,7 +41,7 @@ class JSONSax : nlohmann::json_sax<json>
         Value & value(EvalState & state)
         {
             if (!v)
-                v = allocRootValue(state.allocValue());
+                v = RootValue(state.allocValue());
             return **v;
         }
 
@@ -66,7 +66,7 @@ class JSONSax : nlohmann::json_sax<json>
 
         void add() override
         {
-            v = nullptr;
+            v.reset();
         }
     public:
         void key(string_t & name, EvalState & state)
@@ -92,7 +92,7 @@ class JSONSax : nlohmann::json_sax<json>
         void add() override
         {
             values.push_back(*v);
-            v = nullptr;
+            v.reset();
         }
     public:
         JSONListState(std::unique_ptr<JSONState> && p, std::size_t reserve)

src/libexpr/meson.build

@@ -173,6 +173,7 @@ sources = files(
   'primops.cc',
   'print-ambiguous.cc',
   'print.cc',
+  'root-value.cc',
   'search-path.cc',
   'value-to-json.cc',
   'value-to-xml.cc',