feat(tvix/eval): deduplicate overlap between Closure and Thunk r/5159

This commit deduplicates the Thunk-like functionality from Closure
and unifies it with Thunk.

Specifically, we now have one and only one way of breaking reference
cycles in the Value-graph: Thunk.  No other variant contains a
RefCell.  This should make it easier to reason about the behavior of
the VM.  InnerClosure and UpvaluesCarrier are no longer necessary.

This refactoring allowed an improvement in code generation:
`Rc<RefCell<>>`s are now created only for closures which do not have
self-references or deferred upvalues, instead of for all closures.
OpClosure has been split into two separate opcodes:

- OpClosure creates non-recursive closures with no deferred
  upvalues.  The VM will not create an `Rc<RefCell<>>` when executing
  this instruction.

- OpThunkClosure is used for closures with self-references or
  deferred upvalues.  The VM will create a Thunk when executing this
  opcode, but the Thunk will start out already in the
  `ThunkRepr::Evaluated` state, rather than in the
  `ThunkRepr::Suspeneded` state.

To avoid confusion, OpThunk has been renamed OpThunkSuspended.

Thanks to @sterni for suggesting that all this could be done without
adding an additional variant to ThunkRepr.  This does however mean
that there will be mutating accesses to `ThunkRepr::Evaluated`,
which was not previously the case.  The field `is_finalised:bool`
has been added to `Closure` to ensure that these mutating accesses
are performed only on finalised Closures.  Both the check and the
field are present only if `#[cfg(debug_assertions)]`.

Change-Id: I04131501029772f30e28da8281d864427685097f
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7019
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>

1 files changed, 11 insertions, 30 deletions

diff --git a/tvix/eval/src/upvalues.rs b/tvix/eval/src/upvalues.rs
index 6da9bcf7cc10..450ea130ff24 100644
--- a/tvix/eval/src/upvalues.rs
+++ b/tvix/eval/src/upvalues.rs
@@ -7,10 +7,7 @@
 //! in order to resolve each free variable in the scope to a value.
 //! "Upvalue" is a term taken from Lua.
 
-use std::{
-    cell::{Ref, RefMut},
-    ops::Index,
-};
+use std::ops::Index;
 
 use crate::{opcode::UpvalueIdx, Value};
 
@@ -68,6 +65,16 @@ impl Upvalues {
             Some(stack) => stack.len(),
         }
     }
+
+    /// Resolve deferred upvalues from the provided stack slice,
+    /// mutating them in the internal upvalue slots.
+    pub fn resolve_deferred_upvalues(&mut self, stack: &[Value]) {
+        for upvalue in self.static_upvalues.iter_mut() {
+            if let Value::DeferredUpvalue(update_from_idx) = upvalue {
+                *upvalue = stack[update_from_idx.0].clone();
+            }
+        }
+    }
 }
 
 impl Index<UpvalueIdx> for Upvalues {
@@ -77,29 +84,3 @@ impl Index<UpvalueIdx> for Upvalues {
         &self.static_upvalues[index.0]
     }
 }
-
-/// `UpvalueCarrier` is implemented by all types that carry upvalues.
-pub trait UpvalueCarrier {
-    fn upvalue_count(&self) -> usize;
-
-    /// Read-only accessor for the stored upvalues.
-    fn upvalues(&self) -> Ref<'_, Upvalues>;
-
-    /// Mutable accessor for stored upvalues.
-    fn upvalues_mut(&self) -> RefMut<'_, Upvalues>;
-
-    /// Read an upvalue at the given index.
-    fn upvalue(&self, idx: UpvalueIdx) -> Ref<'_, Value> {
-        Ref::map(self.upvalues(), |v| &v.static_upvalues[idx.0])
-    }
-
-    /// Resolve deferred upvalues from the provided stack slice,
-    /// mutating them in the internal upvalue slots.
-    fn resolve_deferred_upvalues(&self, stack: &[Value]) {
-        for upvalue in self.upvalues_mut().static_upvalues.iter_mut() {
-            if let Value::DeferredUpvalue(idx) = upvalue {
-                *upvalue = stack[idx.0].clone();
-            }
-        }
-    }
-}