From 5f0f4ea3746d6107839454bb5f4967d8757f5bb8 Mon Sep 17 00:00:00 2001 From: Aspen Smith Date: Thu, 1 Feb 2024 12:28:29 -0500 Subject: [PATCH] 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 --- tvix/eval/src/builtins/impure.rs | 15 ++++++--------- tvix/eval/src/builtins/mod.rs | 25 ++++++++++++------------- tvix/eval/src/compiler/bindings.rs | 10 +++++----- tvix/eval/src/compiler/mod.rs | 6 +++--- tvix/eval/src/value/arbitrary.rs | 2 +- tvix/eval/src/value/attrs.rs | 10 +++++++--- tvix/eval/src/value/attrs/tests.rs | 26 ++++++++++---------------- tvix/eval/src/value/mod.rs | 14 ++++++++------ tvix/eval/src/vm/generators.rs | 4 ++-- tvix/eval/src/vm/mod.rs | 11 ++++++----- tvix/glue/src/builtins/derivation.rs | 2 +- tvix/glue/src/builtins/mod.rs | 8 ++++---- tvix/glue/src/tvix_store_io.rs | 4 ++-- tvix/serde/src/de.rs | 2 +- tvix/serde/src/de_tests.rs | 2 +- 15 files changed, 69 insertions(+), 72 deletions(-) diff --git a/tvix/eval/src/builtins/impure.rs b/tvix/eval/src/builtins/impure.rs index 28b869764..5852f148f 100644 --- a/tvix/eval/src/builtins/impure.rs +++ b/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( - match ftype { - FileType::Directory => "directory", - FileType::Regular => "regular", - FileType::Symlink => "symlink", - FileType::Unknown => "unknown", - } - .into(), - ), + Value::from(match ftype { + FileType::Directory => "directory", + FileType::Regular => "regular", + FileType::Symlink => "symlink", + FileType::Unknown => "unknown", + }), ) }); diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index 31cd78fad..0374b4226 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/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())) - .unwrap_or(Value::Null) + grp.map(|g| Value::from(g.as_str())).unwrap_or(Value::Null) }) .collect::>() .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 { - VersionPart::Number(n) => n.into(), - VersionPart::Word(w) => w.into(), + Value::from(match s { + VersionPart::Number(n) => n, + VersionPart::Word(w) => w, }) }) .collect::>(); @@ -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")] diff --git a/tvix/eval/src/compiler/bindings.rs b/tvix/eval/src/compiler/bindings.rs index a3d7c6fbf..236b9dd2c 100644 --- a/tvix/eval/src/compiler/bindings.rs +++ b/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; diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index 0fa3d4139..4a7b5fd5b 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/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 diff --git a/tvix/eval/src/value/arbitrary.rs b/tvix/eval/src/value/arbitrary.rs index d894aa3fe..a2e8cb899 100644 --- a/tvix/eval/src/value/arbitrary.rs +++ b/tvix/eval/src/value/arbitrary.rs @@ -91,7 +91,7 @@ fn leaf_value() -> impl Strategy { any::().prop_map(Bool), any::().prop_map(Integer), any::().prop_map(Float), - any::().prop_map(String), + any::>().prop_map(String), any::().prop_map(|s| Path(PathBuf::from(s).into_boxed_path())), ] } diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs index 2027653c3..6fd43e064 100644 --- a/tvix/eval/src/value/attrs.rs +++ b/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 { 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 diff --git a/tvix/eval/src/value/attrs/tests.rs b/tvix/eval/src/value/attrs/tests.rs index d269044b3..d7e2f113e 100644 --- a/tvix/eval/src/value/attrs/tests.rs +++ b/tvix/eval/src/value/attrs/tests.rs @@ -14,11 +14,8 @@ fn test_empty_attrs() { #[test] fn test_simple_attrs() { - let attrs = NixAttrs::construct( - 1, - vec![Value::String("key".into()), Value::String("value".into())], - ) - .expect("simple attr construction should succeed"); + let attrs = NixAttrs::construct(1, vec![Value::from("key"), Value::from("value")]) + .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 value_val = Value::String("value".into()); - let meaning_val = Value::String("meaning".into()); + let name_val = Value::from("name"); + let value_val = Value::from("value"); + 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 value_val = Value::String("value".into()); - let meaning_val = Value::String("meaning".into()); + let name_val = Value::from("name"); + let value_val = Value::from("value"); + 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( - 1, - vec![Value::String("key".into()), Value::String("value".into())], - ) - .expect("simple attr construction should succeed"); + let attrs = NixAttrs::construct(1, vec![Value::from("key"), Value::from("value")]) + .expect("simple attr construction should succeed"); let mut iter = attrs.iter().collect::>().into_iter(); let (k, v) = iter.next().unwrap(); diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index e9b509834..e0c5a2f51 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -47,7 +47,7 @@ pub enum Value { Bool(bool), Integer(i64), Float(f64), - String(NixString), + String(Box), #[serde(skip)] Path(Box), @@ -186,7 +186,7 @@ where T: Into, { 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 = 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 { 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", Value::Path(p), p.clone()); gen_cast!(to_attrs, Box, "set", Value::Attrs(a), a.clone()); diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs index 3d2fd9d26..e5468fb06 100644 --- a/tvix/eval/src/vm/generators.rs +++ b/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 { 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 diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index 6e36edd95..d23bef674 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/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 { 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 { }, ) .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 { ) .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)), } diff --git a/tvix/glue/src/builtins/derivation.rs b/tvix/glue/src/builtins/derivation.rs index 93a885bdd..b1487f450 100644 --- a/tvix/glue/src/builtins/derivation.rs +++ b/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(), ))) diff --git a/tvix/glue/src/builtins/mod.rs b/tvix/glue/src/builtins/mod.rs index c3c267a98..dff6a8947 100644 --- a/tvix/glue/src/builtins/mod.rs +++ b/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), } diff --git a/tvix/glue/src/tvix_store_io.rs b/tvix/glue/src/tvix_store_io.rs index fea336e23..4146ededb 100644 --- a/tvix/glue/src/tvix_store_io.rs +++ b/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), } diff --git a/tvix/serde/src/de.rs b/tvix/serde/src/de.rs index cf85ffab2..6a020c978 100644 --- a/tvix/serde/src/de.rs +++ b/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), ) })); diff --git a/tvix/serde/src/de_tests.rs b/tvix/serde/src/de_tests.rs index 54c2fdf8f..1c3acd1c2 100644 --- a/tvix/serde/src/de_tests.rs +++ b/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",