diff options
author | Aspen Smith <root@gws.fyi> | 2024-02-01T17·28-0500 |
---|---|---|
committer | aspen <root@gws.fyi> | 2024-02-02T16·16+0000 |
commit | 5f0f4ea3746d6107839454bb5f4967d8757f5bb8 (patch) | |
tree | e618eca064cb7e263c58136f2c07b1dead63f49c /tvix | |
parent | 4c5d9fa356bcb6dcd746129dde934412b44fdd35 (diff) |
refactor(tvix/eval): Box Value::String r/7467
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>
Diffstat (limited to 'tvix')
-rw-r--r-- | tvix/eval/src/builtins/impure.rs | 15 | ||||
-rw-r--r-- | tvix/eval/src/builtins/mod.rs | 25 | ||||
-rw-r--r-- | tvix/eval/src/compiler/bindings.rs | 10 | ||||
-rw-r--r-- | tvix/eval/src/compiler/mod.rs | 6 | ||||
-rw-r--r-- | tvix/eval/src/value/arbitrary.rs | 2 | ||||
-rw-r--r-- | tvix/eval/src/value/attrs.rs | 10 | ||||
-rw-r--r-- | tvix/eval/src/value/attrs/tests.rs | 26 | ||||
-rw-r--r-- | tvix/eval/src/value/mod.rs | 14 | ||||
-rw-r--r-- | tvix/eval/src/vm/generators.rs | 4 | ||||
-rw-r--r-- | tvix/eval/src/vm/mod.rs | 11 | ||||
-rw-r--r-- | tvix/glue/src/builtins/derivation.rs | 2 | ||||
-rw-r--r-- | tvix/glue/src/builtins/mod.rs | 8 | ||||
-rw-r--r-- | tvix/glue/src/tvix_store_io.rs | 4 | ||||
-rw-r--r-- | tvix/serde/src/de.rs | 2 | ||||
-rw-r--r-- | 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 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::<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 { - VersionPart::Number(n) => n.into(), - VersionPart::Word(w) => w.into(), + Value::from(match s { + VersionPart::Number(n) => n, + 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")] 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<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())), ] } 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<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 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::<Vec<_>>().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<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()); 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<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 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<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)), } diff --git a/tvix/glue/src/builtins/derivation.rs b/tvix/glue/src/builtins/derivation.rs index 93a885bdd994..b1487f4505e0 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 c3c267a98782..dff6a8947c16 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 fea336e2350b..4146ededb40c 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 cf85ffab2e81..6a020c978f64 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 54c2fdf8f7fe..1c3acd1c2f52 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", |