Root values cleanup

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

@@ -27,7 +27,7 @@ namespace nix {
 class InstallableAttrPath : public InstallableValue
 {
     SourceExprCommand & cmd;
-    UniqueRootValue v;
+    RootValue v;
     std::string attrPath;
     ExtendedOutputsSpec extendedOutputsSpec;
 

src/libcmd/installable-attr-path.cc

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

src/libexpr/eval-cache.cc

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

src/libexpr/eval.cc

@@ -575,7 +575,7 @@ Value * EvalState::addPrimOp(PrimOp && primOp)
     v->mkPrimOp(new PrimOp(primOp));
 
     if (primOp.internal)
-        internalPrimOps.emplace(primOp.name, UniqueRootValue(v));
+        internalPrimOps.emplace(primOp.name, RootValue(v));
     else {
         staticBaseEnv->vars.emplace_back(envName, baseEnvDispl);
         baseEnv.values[baseEnvDispl++] = v;
@@ -749,11 +749,11 @@ void mapStaticEnvBindings(const SymbolTable & st, const StaticEnv & se, const En
         if (se.isWith && env.values[0]->isFinished()) {
             // add 'with' bindings.
             for (auto & j : *env.values[0]->attrs())
-                vm.insert_or_assign(std::string(st[j.name]), UniqueRootValue(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]), UniqueRootValue(env.values[i.second]));
+                vm.insert_or_assign(std::string(st[i.first]), RootValue(env.values[i.second]));
         }
     }
 }
@@ -1177,7 +1177,7 @@ void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
 
     fileEvalCache->try_emplace_and_cvisit(
         *resolvedPath,
-        UniqueRootValue(nullptr),
+        RootValue(nullptr),
         [&](auto & i) {
             vExpr = allocValue();
             vExpr->mkThunk(&baseEnv, &expr);

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

@@ -45,7 +45,7 @@ public:
 private:
     typedef fun<Value *()> RootLoader;
     RootLoader rootLoader;
-    UniqueRootValue value;
+    RootValue value;
 
     Value * getRootValue();
 
@@ -112,7 +112,7 @@ public:
 private:
     using Parent = std::optional<std::pair<ref<AttrCursor>, Symbol>>;
     const Parent parent;
-    UniqueRootValue _value;
+    RootValue _value;
     std::optional<std::pair<AttrId, AttrValue>> cachedValue;
 
     AttrKey getKey();

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

@@ -168,7 +168,7 @@ struct Constant
     bool impureOnly = false;
 };
 
-typedef std::map<std::string, UniqueRootValue> ValMap;
+typedef std::map<std::string, RootValue> ValMap;
 
 typedef boost::unordered_flat_map<PosIdx, DocComment, std::hash<PosIdx>> DocCommentMap;
 
@@ -472,7 +472,7 @@ private:
     /**
      * A cache from resolved paths to values.
      */
-    const ref<boost::concurrent_flat_map<SourcePath, UniqueRootValue>> 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.
@@ -810,7 +810,7 @@ public:
     /**
      * Internal primops not exposed to the user.
      */
-    boost::unordered_flat_map<std::string, UniqueRootValue, StringViewHash> 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,7 +32,7 @@ private:
      */
     bool failed = false;
 
-    // FIXME: make this a UniqueRootValue.
+    // FIXME: make this a RootValue.
     const Bindings *attrs = nullptr, *meta = nullptr;
 
     const Bindings * getMeta();

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

@@ -21,6 +21,7 @@ namespace nix {
 
 struct Executor
 {
+    // FIXME: support std::moveable_function.
     using work_t = std::function<void()>;
 
     struct Item

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

@@ -8,70 +8,62 @@ namespace nix {
 
 struct Value;
 
-/**
- * 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 `UniqueRootValue`/`RootValue` rather than
- * calling this directly.
- */
-Value ** allocRootValueSlot(Value * v);
-
-/**
- * Clear the given slot and return it to the root value pool.
- */
-void freeRootValueSlot(Value ** slot);
-
 /**
  * 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 UniqueRootValue
+class RootValue
 {
     Value ** slot = nullptr;
 
+    /**
+     * Clear the given slot and return it to the root value pool.
+     */
+    void freeRootValueSlot();
+
 public:
-    UniqueRootValue() = default;
+    RootValue() = default;
 
-    explicit UniqueRootValue(Value * v)
-        : slot(allocRootValueSlot(v))
-    {
-    }
+    /**
+     * 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);
 
-    UniqueRootValue(const UniqueRootValue &) = delete;
-    UniqueRootValue & operator=(const UniqueRootValue &) = delete;
+    RootValue(const RootValue &) = delete;
+    RootValue & operator=(const RootValue &) = delete;
 
-    UniqueRootValue(UniqueRootValue && other) noexcept
+    RootValue(RootValue && other) noexcept
         : slot(std::exchange(other.slot, nullptr))
     {
     }
 
-    UniqueRootValue & operator=(UniqueRootValue && other) noexcept
+    RootValue & operator=(RootValue && other) noexcept
     {
         if (this == &other)
             return *this;
         if (slot)
-            freeRootValueSlot(slot);
+            freeRootValueSlot();
         slot = std::exchange(other.slot, nullptr);
         return *this;
     }
 
-    ~UniqueRootValue()
+    ~RootValue()
     {
-        if (slot)
-            freeRootValueSlot(slot);
+        reset();
     }
 
     /**
      * Release the slot, i.e. stop rooting the value.
      */
     void reset()
     {
-        if (slot) {
-            freeRootValueSlot(slot);
-            slot = nullptr;
-        }
+        if (slot)
+            freeRootValueSlot();
     }
 
     Value *& operator*() const
@@ -85,12 +77,4 @@ public:
     }
 };
 
-/**
- * A copyable, shared handle rooting a Value. Only use this instead of
- * `UniqueRootValue` if the handle must be copyable.
- */
-typedef std::shared_ptr<Value *> RootValue;
-
-RootValue allocRootValue(Value * v);
-
 } // namespace nix

src/libexpr/json-to-value.cc

@@ -18,7 +18,7 @@ class JSONSax : nlohmann::json_sax<json>
     {
     protected:
         std::unique_ptr<JSONState> parent;
-        UniqueRootValue v;
+        RootValue v;
     public:
         virtual std::unique_ptr<JSONState> resolve(EvalState &)
         {
@@ -31,7 +31,7 @@ class JSONSax : nlohmann::json_sax<json>
         }
 
         explicit JSONState(Value * v)
-            : v(UniqueRootValue(v))
+            : v(RootValue(v))
         {
         }
 
@@ -40,7 +40,7 @@ class JSONSax : nlohmann::json_sax<json>
         Value & value(EvalState & state)
         {
             if (!v)
-                v = UniqueRootValue(state.allocValue());
+                v = RootValue(state.allocValue());
             return **v;
         }
 

src/libexpr/parallel-eval.cc

@@ -289,8 +289,9 @@ static void prim_parallel(EvalState & state, const PosIdx pos, Value ** args, Va
         Executor::WorkItems work;
         for (auto value : args[0]->listView())
             if (!value->isFinished())
-                state.addWork(
-                    work, 0, [value(allocRootValue(value)), &state, pos]() { state.forceValue(**value, pos); });
+                state.addWork(work, 0, [value(std::make_shared<RootValue>(value)), &state, pos]() {
+                    state.forceValue(***value, pos);
+                });
         state.executor->spawn(std::move(work));
     }
 

src/libexpr/primops.cc

@@ -786,9 +786,9 @@ struct CompareValues
 
 static void prim_genericClosure(EvalState & state, const PosIdx pos, Value ** args, Value & v)
 {
-    /* The values are rooted via UniqueRootValue, so the lists themselves
+    /* The values are rooted via RootValue, so the lists themselves
        don't need to be visible to the GC. */
-    using ValueList = std::list<UniqueRootValue>;
+    using ValueList = std::list<RootValue>;
 
     state.forceAttrs(*args[0], noPos, "while evaluating the first argument passed to builtins.genericClosure");
 
@@ -803,7 +803,7 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value ** ar
 
     ValueList workSet;
     for (auto elem : startSet->value->listView())
-        workSet.push_back(UniqueRootValue(elem));
+        workSet.push_back(RootValue(elem));
 
     if (startSet->value->listSize() == 0) {
         v = *startSet->value;
@@ -869,7 +869,7 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value ** ar
             }
             throw;
         }
-        res.push_back(UniqueRootValue(e));
+        res.push_back(RootValue(e));
 
         /* Call the `operator' function with `e' as argument. */
         Value newElements;
@@ -884,7 +884,7 @@ static void prim_genericClosure(EvalState & state, const PosIdx pos, Value ** ar
             for (auto elem : newElements.listView()) {
                 state.forceValue(*elem, noPos); // "while evaluating one one of the elements returned by the `operator`
                                                 // passed to builtins.genericClosure");
-                workSet.push_back(UniqueRootValue(elem));
+                workSet.push_back(RootValue(elem));
             }
         } catch (Error & err) {
             err.addTrace(

src/libexpr/root-value.cc

@@ -36,7 +36,7 @@ static auto & freeSlots = *new Sync<Slot *>{nullptr};
 
 #endif
 
-Value ** allocRootValueSlot(Value * v)
+RootValue::RootValue(Value * v)
 {
 #if NIX_USE_BOEHMGC
     Slot * slot;
@@ -58,13 +58,13 @@ Value ** allocRootValueSlot(Value * v)
 
     slot->value = v;
 
-    return &slot->value;
+    this->slot = &slot->value;
 #else
-    return new Value *(v);
+    this->slot = new Value *(v);
 #endif
 }
 
-void freeRootValueSlot(Value ** slot)
+void RootValue::freeRootValueSlot()
 {
 #if NIX_USE_BOEHMGC
     /* Note: writing `nextFree` overwrites the `Value *`, so this also
@@ -76,11 +76,7 @@ void freeRootValueSlot(Value ** slot)
 #else
     delete slot;
 #endif
-}
-
-RootValue allocRootValue(Value * v)
-{
-    return RootValue(allocRootValueSlot(v), freeRootValueSlot);
+    slot = nullptr;
 }
 
 } // namespace nix

src/libexpr/value-to-json.cc

@@ -29,8 +29,8 @@ static void parallelForceDeep(EvalState & state, Value & v, PosIdx pos)
         if (v.attrs()->get(state.s.outPath))
             return;
         for (auto & a : *v.attrs())
-            state.addWork(work, 0, [value(allocRootValue(a.value)), pos(a.pos), &state]() {
-                parallelForceDeep(state, **value, pos);
+            state.addWork(work, 0, [value(std::make_shared<RootValue>(a.value)), pos(a.pos), &state]() {
+                parallelForceDeep(state, ***value, pos);
             });
         break;
     }