From 5f0f4ea3746d6107839454bb5f4967d8757f5bb8 Mon Sep 17 00:00:00 2001 From: Aspen Smith Date: Thu, 1 Feb 2024 12:28:29 -0500 Subject: 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 ++++++----- 10 files changed, 60 insertions(+), 63 deletions(-) (limited to 'tvix/eval/src') diff --git a/tvix/eval/src/builtins/impure.rs b/tvix/eval/src/builtins/impure.rs index 28b8697644dd..5852f148fe6e 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 31cd78fadeab..0374b4226839 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 a3d7c6fbfb33..236b9dd2c3d5 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 0fa3d4139308..4a7b5fd5ba9c 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 d894aa3fe937..a2e8cb899c92 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 2027653c3f39..6fd43e064cf2 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 d269044b32ba..d7e2f113ebdf 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 e9b509834273..e0c5a2f512cf 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 3d2fd9d266c7..e5468fb06d4a 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 6e36edd95670..d23bef6743ef 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)), } -- cgit 1.4.1