feat(tvix/eval): nonrecursive deep_force() r/7175

This commit implements deep_force() nonrecursively, by maintaining
an explicit stack rather than using the call stack for recursion.

As an added bonus, we don't need to pass around the SharedThunkSet
anymore, and can in fact completely eliminate SharedThunkSet.

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

5 files changed, 82 insertions, 80 deletions

diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs
index 09b341886c59..5f99704e63c3 100644
--- a/tvix/eval/src/builtins/mod.rs
+++ b/tvix/eval/src/builtins/mod.rs
@@ -19,7 +19,7 @@ use crate::warnings::WarningKind;
 use crate::{
     self as tvix_eval,
     errors::{CatchableErrorKind, ErrorKind},
-    value::{CoercionKind, NixAttrs, NixList, NixString, SharedThunkSet, Thunk, Value},
+    value::{CoercionKind, NixAttrs, NixList, NixString, Thunk, Value},
 };
 
 use self::versions::{VersionPart, VersionPartsIter};
@@ -236,7 +236,7 @@ mod pure_builtins {
 
     #[builtin("deepSeq")]
     async fn builtin_deep_seq(co: GenCo, x: Value, y: Value) -> Result<Value, ErrorKind> {
-        generators::request_deep_force(&co, x, SharedThunkSet::default()).await;
+        generators::request_deep_force(&co, x).await;
         Ok(y)
     }
 
@@ -983,7 +983,7 @@ mod pure_builtins {
 
     #[builtin("toXML")]
     async fn builtin_to_xml(co: GenCo, value: Value) -> Result<Value, ErrorKind> {
-        let value = generators::request_deep_force(&co, value, SharedThunkSet::default()).await;
+        let value = generators::request_deep_force(&co, value).await;
         let mut buf: Vec<u8> = vec![];
         to_xml::value_to_xml(&mut buf, &value)?;
         Ok(String::from_utf8(buf)?.into())
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index 9be57d43f561..596dddba520b 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -34,7 +34,7 @@ pub use path::canon_path;
 pub use string::NixString;
 pub use thunk::Thunk;
 
-pub use self::thunk::{SharedThunkSet, ThunkSet};
+pub use self::thunk::ThunkSet;
 
 use lazy_static::lazy_static;
 
@@ -206,74 +206,87 @@ pub enum PointerEquality {
 }
 
 impl Value {
-    // TODO(amjoseph): de-asyncify this (when called directly by the VM)
     /// Deeply forces a value, traversing e.g. lists and attribute sets and forcing
     /// their contents, too.
     ///
     /// This is a generator function.
-    pub(super) async fn deep_force(
-        self,
-        co: GenCo,
-        thunk_set: SharedThunkSet,
-    ) -> Result<Value, ErrorKind> {
-        // Get rid of any top-level thunks, and bail out of self-recursive
-        // thunks.
-        let value = if let Value::Thunk(ref t) = &self {
-            if !thunk_set.insert(t) {
-                return Ok(self);
-            }
-            generators::request_force(&co, self).await
+    pub(super) async fn deep_force(self, co: GenCo, span: LightSpan) -> Result<Value, ErrorKind> {
+        if let Some(v) = Self::deep_force_(self.clone(), co, span).await? {
+            Ok(v)
         } else {
-            self
-        };
-
-        match &value {
-            // Short-circuit on already evaluated values, or fail on internal values.
-            Value::Null
-            | Value::Bool(_)
-            | Value::Integer(_)
-            | Value::Float(_)
-            | Value::String(_)
-            | Value::Path(_)
-            | Value::Closure(_)
-            | Value::Builtin(_) => return Ok(value),
-
-            Value::List(list) => {
-                for val in list {
-                    if let c @ Value::Catchable(_) =
-                        generators::request_deep_force(&co, val.clone(), thunk_set.clone()).await
-                    {
-                        return Ok(c);
-                    }
+            Ok(self)
+        }
+    }
+
+    /// Returns Some(v) or None to indicate the returned value is myself
+    async fn deep_force_(
+        myself: Value,
+        co: GenCo,
+        span: LightSpan,
+    ) -> Result<Option<Value>, ErrorKind> {
+        // This is a stack of values which still remain to be forced.
+        let mut vals = vec![myself];
+
+        let mut thunk_set: ThunkSet = Default::default();
+
+        loop {
+            let v = if let Some(v) = vals.pop() {
+                v
+            } else {
+                return Ok(None);
+            };
+
+            // Get rid of any top-level thunks, and bail out of self-recursive
+            // thunks.
+            let value = if let Value::Thunk(t) = &v {
+                if !thunk_set.insert(t) {
+                    continue;
                 }
-            }
+                Thunk::force_(t.clone(), &co, span.clone()).await?
+            } else {
+                v
+            };
 
-            Value::Attrs(attrs) => {
-                for (_, val) in attrs.iter() {
-                    if let c @ Value::Catchable(_) =
-                        generators::request_deep_force(&co, val.clone(), thunk_set.clone()).await
-                    {
-                        return Ok(c);
+            match value {
+                // Short-circuit on already evaluated values, or fail on internal values.
+                Value::Null
+                | Value::Bool(_)
+                | Value::Integer(_)
+                | Value::Float(_)
+                | Value::String(_)
+                | Value::Path(_)
+                | Value::Closure(_)
+                | Value::Builtin(_) => continue,
+
+                Value::List(list) => {
+                    for val in list.into_iter().rev() {
+                        vals.push(val);
                     }
+                    continue;
                 }
-            }
 
-            Value::Thunk(_) => panic!("Tvix bug: force_value() returned a thunk"),
+                Value::Attrs(attrs) => {
+                    for (_, val) in attrs.into_iter().rev() {
+                        vals.push(val);
+                    }
+                    continue;
+                }
 
-            Value::Catchable(_) => return Ok(value),
+                Value::Thunk(_) => panic!("Tvix bug: force_value() returned a thunk"),
 
-            Value::AttrNotFound
-            | Value::Blueprint(_)
-            | Value::DeferredUpvalue(_)
-            | Value::UnresolvedPath(_)
-            | Value::Json(_)
-            | Value::FinaliseRequest(_) => panic!(
-                "Tvix bug: internal value left on stack: {}",
-                value.type_of()
-            ),
-        };
+                Value::Catchable(_) => return Ok(Some(value)),
 
-        Ok(value)
+                Value::AttrNotFound
+                | Value::Blueprint(_)
+                | Value::DeferredUpvalue(_)
+                | Value::UnresolvedPath(_)
+                | Value::Json(_)
+                | Value::FinaliseRequest(_) => panic!(
+                    "Tvix bug: internal value left on stack: {}",
+                    value.type_of()
+                ),
+            }
+        }
     }
 
     // TODO(amjoseph): de-asyncify this (when called directly by the VM)
diff --git a/tvix/eval/src/value/thunk.rs b/tvix/eval/src/value/thunk.rs
index cafa8275dec6..a67537f945a9 100644
--- a/tvix/eval/src/value/thunk.rs
+++ b/tvix/eval/src/value/thunk.rs
@@ -432,14 +432,3 @@ impl ThunkSet {
         self.0.insert(ptr)
     }
 }
-
-#[derive(Default, Clone)]
-pub struct SharedThunkSet(Rc<RefCell<ThunkSet>>);
-
-impl SharedThunkSet {
-    /// Check whether the given thunk has already been seen. Will mark the thunk
-    /// as seen otherwise.
-    pub fn insert(&self, thunk: &Thunk) -> bool {
-        self.0.borrow_mut().insert(thunk)
-    }
-}
diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs
index 4fff498fe7c6..7b92c1f0af35 100644
--- a/tvix/eval/src/vm/generators.rs
+++ b/tvix/eval/src/vm/generators.rs
@@ -13,7 +13,7 @@ pub use genawaiter::rc::Gen;
 use std::fmt::Display;
 use std::future::Future;
 
-use crate::value::{PointerEquality, SharedThunkSet};
+use crate::value::PointerEquality;
 use crate::warnings::{EvalWarning, WarningKind};
 use crate::FileType;
 use crate::NixString;
@@ -43,7 +43,7 @@ pub enum VMRequest {
     ForceValue(Value),
 
     /// Request that the VM deep-forces the value.
-    DeepForceValue(Value, SharedThunkSet),
+    DeepForceValue(Value),
 
     /// Request the value at the given index from the VM's with-stack, in forced
     /// state.
@@ -128,7 +128,7 @@ impl Display for VMRequest {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             VMRequest::ForceValue(v) => write!(f, "force_value({})", v.type_of()),
-            VMRequest::DeepForceValue(v, _) => {
+            VMRequest::DeepForceValue(v) => {
                 write!(f, "deep_force_value({})", v.type_of())
             }
             VMRequest::WithValue(_) => write!(f, "with_value"),
@@ -294,10 +294,10 @@ impl<'o> VM<'o> {
                         }
 
                         // Generator has requested a deep-force.
-                        VMRequest::DeepForceValue(value, thunk_set) => {
+                        VMRequest::DeepForceValue(value) => {
                             self.reenqueue_generator(name, span.clone(), generator);
-                            self.enqueue_generator("deep_force", span, |co| {
-                                value.deep_force(co, thunk_set)
+                            self.enqueue_generator("deep_force", span.clone(), |co| {
+                                value.deep_force(co, span)
                             });
                             return Ok(false);
                         }
@@ -606,8 +606,8 @@ pub async fn request_string_coerce(
 }
 
 /// Deep-force any value and return the evaluated result from the VM.
-pub async fn request_deep_force(co: &GenCo, val: Value, thunk_set: SharedThunkSet) -> Value {
-    match co.yield_(VMRequest::DeepForceValue(val, thunk_set)).await {
+pub async fn request_deep_force(co: &GenCo, val: Value) -> Value {
+    match co.yield_(VMRequest::DeepForceValue(val)).await {
         VMResponse::Value(value) => value,
         msg => panic!(
             "Tvix bug: VM responded with incorrect generator message: {}",
diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs
index e11b3b5d13c4..5ab095b22056 100644
--- a/tvix/eval/src/vm/mod.rs
+++ b/tvix/eval/src/vm/mod.rs
@@ -30,7 +30,7 @@ use crate::{
     upvalues::Upvalues,
     value::{
         Builtin, BuiltinResult, Closure, CoercionKind, Lambda, NixAttrs, NixList, PointerEquality,
-        SharedThunkSet, Thunk, Value,
+        Thunk, Value,
     },
     vm::generators::GenCo,
     warnings::{EvalWarning, WarningKind},
@@ -1202,7 +1202,7 @@ pub struct RuntimeResult {
 /// before returning.
 async fn final_deep_force(co: GenCo) -> Result<Value, ErrorKind> {
     let value = generators::request_stack_pop(&co).await;
-    Ok(generators::request_deep_force(&co, value, SharedThunkSet::default()).await)
+    Ok(generators::request_deep_force(&co, value).await)
 }
 
 pub fn run_lambda(