refactor(tvix/eval): Box Value::String

NixString is *quite* large - like 80 bytes - because of the extra
capacity value for BString and because of the context. We want to keep
Value small since we're passing it around a lot, so let's box the
NixString inside Value::String to save on some memory, and make cloning
ostensibly a little cheaper

Change-Id: I343c8b4e7f61dc3dcbbaba4382efb3b3e5bbabb2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10729
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>

tvix/eval/src/builtins/impure.rs

@@ -50,15 +50,12 @@ mod impure_builtins {
                         NixString::from(
                             String::from_utf8(name.to_vec()).expect("parsing file name as string"),
                         ),
-                        Value::String(
+                        Value::from(match ftype {
-                            match ftype {
+                            FileType::Directory => "directory",
-                                FileType::Directory => "directory",
+                            FileType::Regular => "regular",
-                                FileType::Regular => "regular",
+                            FileType::Symlink => "symlink",
-                                FileType::Symlink => "symlink",
+                            FileType::Unknown => "unknown",
-                                FileType::Unknown => "unknown",
+                        }),
-                            }
-                            .into(),
-                        ),
                     )
                 });
 

tvix/eval/src/builtins/mod.rs

@@ -167,7 +167,7 @@ mod pure_builtins {
         let mut output = Vec::with_capacity(xs.len());
 
         for (key, _val) in xs.iter() {
-            output.push(Value::String(key.clone()));
+            output.push(Value::from(key.clone()));
         }
 
         Ok(Value::List(NixList::construct(output.len(), output)))
@@ -404,7 +404,7 @@ mod pure_builtins {
                 result.to_owned(),
             ))))
         } else {
-            Ok(Value::String(NixString::new_inherit_context_from(
+            Ok(Value::from(NixString::new_inherit_context_from(
                 &str,
                 result.into(),
             )))
@@ -1095,8 +1095,7 @@ mod pure_builtins {
                         // can be observed in make-initrd.nix when it comes
                         // to compressors which are matched over their full command
                         // and then a compressor name will be extracted from that.
-                        grp.map(|g| Value::String(g.as_str().into()))
+                        grp.map(|g| Value::from(g.as_str())).unwrap_or(Value::Null)
-                            .unwrap_or(Value::Null)
                     })
                     .collect::<imbl::Vector<Value>>()
                     .into(),
@@ -1308,7 +1307,7 @@ mod pure_builtins {
             }
         }
 
-        Ok(Value::String(NixString::new_context_from(context, res)))
+        Ok(Value::from(NixString::new_context_from(context, res)))
     }
 
     #[builtin("seq")]
@@ -1396,9 +1395,9 @@ mod pure_builtins {
 
         let parts = s
             .map(|s| {
-                Value::String(match s {
+                Value::from(match s {
-                    VersionPart::Number(n) => n.into(),
+                    VersionPart::Number(n) => n,
-                    VersionPart::Word(w) => w.into(),
+                    VersionPart::Word(w) => w,
                 })
             })
             .collect::<Vec<Value>>();
@@ -1466,7 +1465,7 @@ mod pure_builtins {
         // non-negative when the starting index is GTE the
         // string's length.
         if beg >= x.len() {
-            return Ok(Value::String(NixString::new_inherit_context_from(
+            return Ok(Value::from(NixString::new_inherit_context_from(
                 &x,
                 BString::default(),
             )));
@@ -1478,7 +1477,7 @@ mod pure_builtins {
             cmp::min(beg + (len as usize), x.len())
         };
 
-        Ok(Value::String(NixString::new_inherit_context_from(
+        Ok(Value::from(NixString::new_inherit_context_from(
             &x,
             (&x[beg..end]).into(),
         )))
@@ -1599,7 +1598,7 @@ mod pure_builtins {
             return Ok(x);
         }
 
-        Ok(Value::String(x.type_of().into()))
+        Ok(Value::from(x.type_of()))
     }
 }
 
@@ -1634,7 +1633,7 @@ pub fn pure_builtins() -> Vec<(&'static str, Value)> {
     let mut result = pure_builtins::builtins();
 
     // Pure-value builtins
-    result.push(("nixVersion", Value::String("2.3-compat-tvix-0.1".into())));
+    result.push(("nixVersion", Value::from("2.3-compat-tvix-0.1")));
     result.push(("langVersion", Value::Integer(6)));
     result.push(("null", Value::Null));
     result.push(("true", Value::Bool(true)));
@@ -1685,7 +1684,7 @@ mod placeholder_builtins {
             .await?
             .to_contextful_str()?;
         v.clear_context();
-        Ok(Value::String(v))
+        Ok(Value::from(v))
     }
 
     #[builtin("addErrorContext")]

tvix/eval/src/compiler/bindings.rs

@@ -361,7 +361,7 @@ impl Compiler<'_> {
 
                         // Place key on the stack when compiling attribute sets.
                         if kind.is_attrs() {
-                            self.emit_constant(Value::String(name.as_str().into()), &attr);
+                            self.emit_constant(name.as_str().into(), &attr);
                             let span = self.span_for(&attr);
                             self.scope_mut().declare_phantom(span, true);
                         }
@@ -569,7 +569,7 @@ impl Compiler<'_> {
 
                 KeySlot::Static { slot, name } => {
                     let span = self.scope()[slot].span;
-                    self.emit_constant(Value::String(name.as_str().into()), &span);
+                    self.emit_constant(name.as_str().into(), &span);
                     self.scope_mut().mark_initialised(slot);
                 }
 
@@ -593,7 +593,7 @@ impl Compiler<'_> {
                         c.compile(s, namespace.clone());
                         c.emit_force(&namespace);
 
-                        c.emit_constant(Value::String(name.as_str().into()), &span);
+                        c.emit_constant(name.as_str().into(), &span);
                         c.push_op(OpCode::OpAttrsSelect, &span);
                     })
                 }
@@ -685,7 +685,7 @@ impl Compiler<'_> {
         // (OpAttrs consumes all of these locals).
         self.scope_mut().end_scope();
 
-        self.emit_constant(Value::String("body".into()), node);
+        self.emit_constant("body".into(), node);
         self.push_op(OpCode::OpAttrsSelect, node);
     }
 
@@ -730,7 +730,7 @@ impl Compiler<'_> {
                 if self.has_dynamic_ancestor() {
                     self.thunk(slot, node, |c, _| {
                         c.context_mut().captures_with_stack = true;
-                        c.emit_constant(Value::String(ident.into()), node);
+                        c.emit_constant(ident.into(), node);
                         c.push_op(OpCode::OpResolveWith, node);
                     });
                     return;

tvix/eval/src/compiler/mod.rs

@@ -369,7 +369,7 @@ impl Compiler<'_> {
 
             ast::LiteralKind::Uri(u) => {
                 self.emit_warning(node, WarningKind::DeprecatedLiteralURL);
-                Value::String(u.syntax().text().into())
+                Value::from(u.syntax().text())
             }
         };
 
@@ -458,7 +458,7 @@ impl Compiler<'_> {
                 }
 
                 ast::InterpolPart::Literal(lit) => {
-                    self.emit_constant(Value::String(lit.as_str().into()), parent_node);
+                    self.emit_constant(Value::from(lit.as_str()), parent_node);
                 }
             }
         }
@@ -1360,7 +1360,7 @@ impl Compiler<'_> {
     /// several operations related to attribute sets, where
     /// identifiers are used as string keys.
     fn emit_literal_ident(&mut self, ident: &ast::Ident) {
-        self.emit_constant(Value::String(ident.clone().into()), ident);
+        self.emit_constant(Value::String(Box::new(ident.clone().into())), ident);
     }
 
     /// Patch the jump instruction at the given index, setting its

tvix/eval/src/value/arbitrary.rs

@@ -91,7 +91,7 @@ fn leaf_value() -> impl Strategy<Value = Value> {
         any::<bool>().prop_map(Bool),
         any::<i64>().prop_map(Integer),
         any::<f64>().prop_map(Float),
-        any::<NixString>().prop_map(String),
+        any::<Box<NixString>>().prop_map(String),
         any::<OsString>().prop_map(|s| Path(PathBuf::from(s).into_boxed_path())),
     ]
 }

tvix/eval/src/value/attrs.rs

@@ -360,7 +360,7 @@ impl NixAttrs {
             let key = stack_slice.pop().unwrap();
 
             match key {
-                Value::String(ks) => set_attr(&mut attrs, ks, value)?,
+                Value::String(ks) => set_attr(&mut attrs, *ks, value)?,
 
                 Value::Null => {
                     // This is in fact valid, but leads to the value
@@ -414,9 +414,13 @@ impl IntoIterator for NixAttrs {
 fn attempt_optimise_kv(slice: &mut [Value]) -> Option<NixAttrs> {
     let (name_idx, value_idx) = {
         match (&slice[2], &slice[0]) {
-            (Value::String(s1), Value::String(s2)) if (*s1 == *NAME_S && *s2 == *VALUE_S) => (3, 1),
+            (Value::String(s1), Value::String(s2)) if (**s1 == *NAME_S && **s2 == *VALUE_S) => {
+                (3, 1)
+            }
 
-            (Value::String(s1), Value::String(s2)) if (*s1 == *VALUE_S && *s2 == *NAME_S) => (1, 3),
+            (Value::String(s1), Value::String(s2)) if (**s1 == *VALUE_S && **s2 == *NAME_S) => {
+                (1, 3)
+            }
 
             // Technically this branch lets type errors pass,
             // but they will be caught during normal attribute

tvix/eval/src/value/attrs/tests.rs

@@ -14,11 +14,8 @@ fn test_empty_attrs() {
 
 #[test]
 fn test_simple_attrs() {
-    let attrs = NixAttrs::construct(
+    let attrs = NixAttrs::construct(1, vec![Value::from("key"), Value::from("value")])
-        1,
+        .expect("simple attr construction should succeed");
-        vec![Value::String("key".into()), Value::String("value".into())],
-    )
-    .expect("simple attr construction should succeed");
 
     assert!(
         matches!(attrs, NixAttrs(AttrsRep::Im(_))),
@@ -28,9 +25,9 @@ fn test_simple_attrs() {
 
 #[test]
 fn test_kv_attrs() {
-    let name_val = Value::String("name".into());
+    let name_val = Value::from("name");
-    let value_val = Value::String("value".into());
+    let value_val = Value::from("value");
-    let meaning_val = Value::String("meaning".into());
+    let meaning_val = Value::from("meaning");
     let forty_two_val = Value::Integer(42);
 
     let kv_attrs = NixAttrs::construct(
@@ -64,9 +61,9 @@ fn test_empty_attrs_iter() {
 
 #[test]
 fn test_kv_attrs_iter() {
-    let name_val = Value::String("name".into());
+    let name_val = Value::from("name");
-    let value_val = Value::String("value".into());
+    let value_val = Value::from("value");
-    let meaning_val = Value::String("meaning".into());
+    let meaning_val = Value::from("meaning");
     let forty_two_val = Value::Integer(42);
 
     let kv_attrs = NixAttrs::construct(
@@ -92,11 +89,8 @@ fn test_kv_attrs_iter() {
 
 #[test]
 fn test_map_attrs_iter() {
-    let attrs = NixAttrs::construct(
+    let attrs = NixAttrs::construct(1, vec![Value::from("key"), Value::from("value")])
-        1,
+        .expect("simple attr construction should succeed");
-        vec![Value::String("key".into()), Value::String("value".into())],
-    )
-    .expect("simple attr construction should succeed");
 
     let mut iter = attrs.iter().collect::<Vec<_>>().into_iter();
     let (k, v) = iter.next().unwrap();

tvix/eval/src/value/mod.rs

@@ -47,7 +47,7 @@ pub enum Value {
     Bool(bool),
     Integer(i64),
     Float(f64),
-    String(NixString),
+    String(Box<NixString>),
 
     #[serde(skip)]
     Path(Box<Path>),
@@ -186,7 +186,7 @@ where
     T: Into<NixString>,
 {
     fn from(t: T) -> Self {
-        Self::String(t.into())
+        Self::String(Box::new(t.into()))
     }
 }
 
@@ -327,7 +327,9 @@ impl Value {
             let value = if let Some(v) = vals.pop() {
                 v.force(co, span.clone()).await?
             } else {
-                return Ok(Value::String(NixString::new_context_from(context, result)));
+                return Ok(Value::String(Box::new(NixString::new_context_from(
+                    context, result,
+                ))));
             };
             let coerced: Result<BString, _> = match (value, kind) {
                 // coercions that are always done
@@ -335,7 +337,7 @@ impl Value {
                     if let Some(ctx) = s.context_mut() {
                         context = context.join(ctx);
                     }
-                    Ok(s.into())
+                    Ok((*s).into())
                 }
 
                 // TODO(sterni): Think about proper encoding handling here. This needs
@@ -692,7 +694,7 @@ impl Value {
     /// everytime you want a string.
     pub fn to_str(&self) -> Result<NixString, ErrorKind> {
         match self {
-            Value::String(s) if !s.has_context() => Ok(s.clone()),
+            Value::String(s) if !s.has_context() => Ok((**s).clone()),
             Value::Thunk(thunk) => Self::to_str(&thunk.value()),
             other => Err(type_error("contextless strings", other)),
         }
@@ -703,7 +705,7 @@ impl Value {
         NixString,
         "contextful string",
         Value::String(s),
-        s.clone()
+        (**s).clone()
     );
     gen_cast!(to_path, Box<Path>, "path", Value::Path(p), p.clone());
     gen_cast!(to_attrs, Box<NixAttrs>, "set", Value::Attrs(a), a.clone());

tvix/eval/src/vm/generators.rs

@@ -436,7 +436,7 @@ where
                                 })
                                 .with_span(&span, self)?;
 
-                            message = VMResponse::Value(Value::String(content.into()))
+                            message = VMResponse::Value(content.into())
                         }
 
                         VMRequest::PathExists(path) => {
@@ -607,7 +607,7 @@ pub async fn request_string_coerce(
     kind: CoercionKind,
 ) -> Result<NixString, CatchableErrorKind> {
     match val {
-        Value::String(s) => Ok(s),
+        Value::String(s) => Ok(*s),
         _ => match co.yield_(VMRequest::StringCoerce(val, kind)).await {
             VMResponse::Value(Value::Catchable(c)) => Err(c),
             VMResponse::Value(value) => Ok(value

tvix/eval/src/vm/mod.rs

@@ -987,9 +987,10 @@ where
             }
         }
 
-        // FIXME: consume immediately here the String.
         self.stack
-            .push(Value::String(NixString::new_context_from(context, out)));
+            .push(Value::String(Box::new(NixString::new_context_from(
+                context, out,
+            ))));
         Ok(())
     }
 
@@ -1250,7 +1251,7 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result<Value, ErrorKind> {
                 Err(c) => Value::Catchable(c),
             }
         }
-        (Value::String(s1), Value::String(s2)) => Value::String(s1.concat(&s2)),
+        (Value::String(s1), Value::String(s2)) => Value::String(Box::new(s1.concat(&s2))),
         (Value::String(s1), v) => generators::request_string_coerce(
             &co,
             v,
@@ -1261,7 +1262,7 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result<Value, ErrorKind> {
             },
         )
         .await
-        .map(|s2| Value::String(s1.concat(&s2)))
+        .map(|s2| Value::String(Box::new(s1.concat(&s2))))
         .into(),
         (a @ Value::Integer(_), b) | (a @ Value::Float(_), b) => arithmetic_op!(&a, &b, +)?,
         (a, b) => {
@@ -1284,7 +1285,7 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result<Value, ErrorKind> {
             )
             .await;
             match (r1, r2) {
-                (Ok(s1), Ok(s2)) => Value::String(s1.concat(&s2)),
+                (Ok(s1), Ok(s2)) => Value::String(Box::new(s1.concat(&s2))),
                 (Err(c), _) => return Ok(Value::Catchable(c)),
                 (_, Err(c)) => return Ok(Value::Catchable(c)),
             }

tvix/glue/src/builtins/derivation.rs

@@ -530,7 +530,7 @@ pub(crate) mod derivation_builtins {
 
         // TODO: actually persist the file in the store at that path ...
 
-        Ok(Value::String(NixString::new_context_from(
+        Ok(Value::from(NixString::new_context_from(
             context,
             path.into(),
         )))

tvix/glue/src/builtins/mod.rs

@@ -74,7 +74,7 @@ mod tests {
 
         match value {
             tvix_eval::Value::String(s) => {
-                assert_eq!(s, "/nix/store/xpcvxsx5sw4rbq666blz6sxqlmsqphmr-foo",);
+                assert_eq!(*s, "/nix/store/xpcvxsx5sw4rbq666blz6sxqlmsqphmr-foo",);
             }
             _ => panic!("unexpected value type: {:?}", value),
         }
@@ -159,7 +159,7 @@ mod tests {
 
         match value {
             tvix_eval::Value::String(s) => {
-                assert_eq!(s, expected_path);
+                assert_eq!(*s, expected_path);
             }
             _ => panic!("unexpected value type: {:?}", value),
         }
@@ -282,7 +282,7 @@ mod tests {
 
         match value {
             tvix_eval::Value::String(s) => {
-                assert_eq!(s, expected_drvpath);
+                assert_eq!(*s, expected_drvpath);
             }
 
             _ => panic!("unexpected value type: {:?}", value),
@@ -311,7 +311,7 @@ mod tests {
 
         match value {
             tvix_eval::Value::String(s) => {
-                assert_eq!(s, expected_path);
+                assert_eq!(*s, expected_path);
             }
             _ => panic!("unexpected value type: {:?}", value),
         }

tvix/glue/src/tvix_store_io.rs

@@ -356,7 +356,7 @@ mod tests {
 
         let value = result.value.expect("must be some");
         match value {
-            tvix_eval::Value::String(s) => Some((**s).clone().into_string_lossy()),
+            tvix_eval::Value::String(s) => Some((***s).clone().into_string_lossy()),
             _ => panic!("unexpected value type: {:?}", value),
         }
     }
@@ -422,7 +422,7 @@ mod tests {
 
         match value {
             tvix_eval::Value::String(s) => {
-                assert_eq!(s, "/deep/thought");
+                assert_eq!(*s, "/deep/thought");
             }
             _ => panic!("unexpected value type: {:?}", value),
         }

tvix/serde/src/de.rs

@@ -347,7 +347,7 @@ impl<'de> de::Deserializer<'de> for NixDeserializer {
         if let Value::Attrs(attrs) = self.value {
             let mut map = MapDeserializer::new(attrs.into_iter().map(|(k, v)| {
                 (
-                    NixDeserializer::new(Value::String(k)),
+                    NixDeserializer::new(Value::from(k)),
                     NixDeserializer::new(v),
                 )
             }));

tvix/serde/src/de_tests.rs

@@ -222,7 +222,7 @@ mod test_builtins {
         match x {
             Value::String(s) => {
                 let new_string = NixString::from(format!("hello {}", s.to_str().unwrap()));
-                Ok(Value::String(new_string))
+                Ok(Value::from(new_string))
             }
             _ => Err(ErrorKind::TypeError {
                 expected: "string",