From 7e286aab1a573edc9aef16bb68e9907371917adc Mon Sep 17 00:00:00 2001 From: Aspen Smith Date: Sat, 10 Feb 2024 12:39:24 -0500 Subject: [PATCH] feat(tvix/eval): Box Value::Catchable This is now the only enum variant for Value that is larger than 8 bytes (it's 16 bytes), so boxing it (especially since it's not perf-critical) allows us to get the Value size down to only 16 bytes! Change-Id: I98598e2b762944448bef982e8ff7da6d6683c4aa Reviewed-on: https://cl.tvl.fyi/c/depot/+/10798 Tested-by: BuildkiteCI Reviewed-by: raitobezarius Autosubmit: aspen --- tvix/eval/src/builtins/impure.rs | 6 +++--- tvix/eval/src/builtins/mod.rs | 18 +++++++++--------- tvix/eval/src/compiler/import.rs | 2 +- tvix/eval/src/compiler/mod.rs | 4 ++-- tvix/eval/src/value/attrs.rs | 2 +- tvix/eval/src/value/json.rs | 6 +++--- tvix/eval/src/value/mod.rs | 15 ++++++++++----- tvix/eval/src/vm/generators.rs | 6 +++--- tvix/eval/src/vm/macros.rs | 2 +- tvix/eval/src/vm/mod.rs | 8 ++++---- tvix/glue/src/builtins/derivation.rs | 16 ++++++++-------- 11 files changed, 45 insertions(+), 40 deletions(-) diff --git a/tvix/eval/src/builtins/impure.rs b/tvix/eval/src/builtins/impure.rs index 5852f148f..def6ce290 100644 --- a/tvix/eval/src/builtins/impure.rs +++ b/tvix/eval/src/builtins/impure.rs @@ -33,7 +33,7 @@ mod impure_builtins { #[builtin("pathExists")] async fn builtin_path_exists(co: GenCo, path: Value) -> Result { match coerce_value_to_path(&co, path).await? { - Err(cek) => Ok(Value::Catchable(cek)), + Err(cek) => Ok(Value::from(cek)), Ok(path) => Ok(generators::request_path_exists(&co, path).await), } } @@ -41,7 +41,7 @@ mod impure_builtins { #[builtin("readDir")] async fn builtin_read_dir(co: GenCo, path: Value) -> Result { match coerce_value_to_path(&co, path).await? { - Err(cek) => Ok(Value::Catchable(cek)), + Err(cek) => Ok(Value::from(cek)), Ok(path) => { let dir = generators::request_read_dir(&co, path).await; let res = dir.into_iter().map(|(name, ftype)| { @@ -67,7 +67,7 @@ mod impure_builtins { #[builtin("readFile")] async fn builtin_read_file(co: GenCo, path: Value) -> Result { match coerce_value_to_path(&co, path).await? { - Err(cek) => Ok(Value::Catchable(cek)), + Err(cek) => Ok(Value::from(cek)), Ok(path) => Ok(generators::request_read_to_string(&co, path).await), } } diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index d63b035c0..c070bd44b 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -303,7 +303,7 @@ mod pure_builtins { context = context.join(other_context); } } - Err(c) => return Ok(Value::Catchable(c)), + Err(c) => return Ok(Value::Catchable(Box::new(c))), } } // FIXME: pass immediately the string res. @@ -365,7 +365,7 @@ mod pure_builtins { { Ok(true) => return Ok(true.into()), Ok(false) => continue, - Err(cek) => return Ok(Value::Catchable(cek)), + Err(cek) => return Ok(Value::from(cek)), } } Ok(false.into()) @@ -452,7 +452,7 @@ mod pure_builtins { #[builtin("toJSON")] async fn builtin_to_json(co: GenCo, val: Value) -> Result { match val.into_json(&co).await? { - Err(cek) => Ok(Value::Catchable(cek)), + Err(cek) => Ok(Value::from(cek)), Ok(json_value) => { let json_str = serde_json::to_string(&json_value)?; Ok(json_str.into()) @@ -471,7 +471,7 @@ mod pure_builtins { #[allow(non_snake_case)] async fn builtin_filterSource(_co: GenCo, #[lazy] _e: Value) -> Result { // TODO: implement for nixpkgs compatibility - Ok(Value::Catchable(CatchableErrorKind::UnimplementedFeature( + Ok(Value::from(CatchableErrorKind::UnimplementedFeature( "filterSource".into(), ))) } @@ -690,7 +690,7 @@ mod pure_builtins { _string: Value, ) -> Result { // FIXME: propagate contexts here. - Ok(Value::Catchable(CatchableErrorKind::UnimplementedFeature( + Ok(Value::from(CatchableErrorKind::UnimplementedFeature( "hashString".into(), ))) } @@ -884,7 +884,7 @@ mod pure_builtins { async fn builtin_less_than(co: GenCo, x: Value, y: Value) -> Result { let span = generators::request_span(&co).await; match x.nix_cmp_ordering(y, co, span).await? { - Err(cek) => Ok(Value::Catchable(cek)), + Err(cek) => Ok(Value::from(cek)), Ok(Ordering::Less) => Ok(Value::Bool(true)), Ok(_) => Ok(Value::Bool(false)), } @@ -1387,7 +1387,7 @@ mod pure_builtins { } // TODO(sterni): coerces to string // We do not care about the context here explicitly. - Ok(Value::Catchable(CatchableErrorKind::Throw( + Ok(Value::from(CatchableErrorKind::Throw( message.to_contextful_str()?.to_string().into(), ))) } @@ -1444,7 +1444,7 @@ mod pure_builtins { } match coerce_value_to_path(&co, s).await? { - Err(cek) => Ok(Value::Catchable(cek)), + Err(cek) => Ok(Value::from(cek)), Ok(path) => { let path: Value = crate::value::canon_path(path).into(); let span = generators::request_span(&co).await; @@ -1527,7 +1527,7 @@ pub fn pure_builtins() -> Vec<(&'static str, Value)> { // TODO: implement for nixpkgs compatibility result.push(( "__curPos", - Value::Catchable(CatchableErrorKind::UnimplementedFeature("__curPos".into())), + Value::from(CatchableErrorKind::UnimplementedFeature("__curPos".into())), )); result diff --git a/tvix/eval/src/compiler/import.rs b/tvix/eval/src/compiler/import.rs index 4e8ea7195..6774c7b89 100644 --- a/tvix/eval/src/compiler/import.rs +++ b/tvix/eval/src/compiler/import.rs @@ -27,7 +27,7 @@ async fn import_impl( ) -> Result { // TODO(sterni): canon_path()? let mut path = match coerce_value_to_path(&co, args.pop().unwrap()).await? { - Err(cek) => return Ok(Value::Catchable(cek)), + Err(cek) => return Ok(Value::Catchable(Box::new(cek))), Ok(path) => path, }; diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index 0a9ee2320..d64202c2a 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/tvix/eval/src/compiler/mod.rs @@ -399,9 +399,9 @@ impl Compiler<'_> { // TODO: decide what to do with findFile if raw_path.len() == 2 { return self.emit_constant( - Value::Catchable(CatchableErrorKind::NixPathResolution( + Value::Catchable(Box::new(CatchableErrorKind::NixPathResolution( "Empty <> path not allowed".into(), - )), + ))), node, ); } diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs index 23a2acb0b..c9f04b54c 100644 --- a/tvix/eval/src/value/attrs.rs +++ b/tvix/eval/src/value/attrs.rs @@ -375,7 +375,7 @@ impl NixAttrs { continue; } - Value::Catchable(err) => return Ok(Err(err)), + Value::Catchable(err) => return Ok(Err(*err)), other => return Err(ErrorKind::InvalidAttributeName(other)), } diff --git a/tvix/eval/src/value/json.rs b/tvix/eval/src/value/json.rs index efd2763f1..7ab1655d9 100644 --- a/tvix/eval/src/value/json.rs +++ b/tvix/eval/src/value/json.rs @@ -61,7 +61,7 @@ impl Value { ) .await? { - Value::Catchable(cek) => return Ok(Err(cek)), + Value::Catchable(cek) => return Ok(Err(*cek)), Value::String(s) => return Ok(Ok(Json::String(s.to_str()?.to_owned()))), _ => panic!("Value::coerce_to_string_() returned a non-string!"), } @@ -88,7 +88,7 @@ impl Value { Json::Object(out) } - Value::Catchable(c) => return Ok(Err(c)), + Value::Catchable(c) => return Ok(Err(*c)), val @ Value::Closure(_) | val @ Value::Thunk(_) @@ -110,7 +110,7 @@ impl Value { /// Value::Json. pub(crate) async fn into_json_generator(self, co: GenCo) -> Result { match self.into_json(&co).await? { - Err(cek) => Ok(Value::Catchable(cek)), + Err(cek) => Ok(Value::from(cek)), Ok(json) => Ok(Value::Json(Box::new(json))), } } diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index 3407dd520..efe2d09e3 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -84,12 +84,13 @@ pub enum Value { FinaliseRequest(bool), #[serde(skip)] - Catchable(CatchableErrorKind), + Catchable(Box), } impl From for Value { + #[inline] fn from(c: CatchableErrorKind) -> Value { - Value::Catchable(c) + Value::Catchable(Box::new(c)) } } @@ -97,8 +98,12 @@ impl From> for Value where Value: From, { + #[inline] fn from(v: Result) -> Value { - v.map_or_else(Value::Catchable, |v| v.into()) + match v { + Ok(v) => v.into(), + Err(e) => Value::Catchable(Box::new(e)), + } } } @@ -776,8 +781,8 @@ impl Value { b = b.force(&co, span.clone()).await?; } let result = match (a, b) { - (Value::Catchable(c), _) => return Ok(Err(c)), - (_, Value::Catchable(c)) => return Ok(Err(c)), + (Value::Catchable(c), _) => return Ok(Err(*c)), + (_, Value::Catchable(c)) => return Ok(Err(*c)), // same types (Value::Integer(i1), Value::Integer(i2)) => i1.cmp(&i2), (Value::Float(f1), Value::Float(f2)) => f1.total_cmp(&f2), diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs index cffe36e60..fba4e7cd9 100644 --- a/tvix/eval/src/vm/generators.rs +++ b/tvix/eval/src/vm/generators.rs @@ -609,7 +609,7 @@ pub async fn request_string_coerce( match val { Value::String(s) => Ok(*s), _ => match co.yield_(VMRequest::StringCoerce(val, kind)).await { - VMResponse::Value(Value::Catchable(c)) => Err(c), + VMResponse::Value(Value::Catchable(c)) => Err(*c), VMResponse::Value(value) => Ok(value .to_contextful_str() .expect("coerce_to_string always returns a string")), @@ -644,7 +644,7 @@ pub(crate) async fn check_equality( .await { VMResponse::Value(Value::Bool(b)) => Ok(Ok(b)), - VMResponse::Value(Value::Catchable(cek)) => Ok(Err(cek)), + VMResponse::Value(Value::Catchable(cek)) => Ok(Err(*cek)), msg => panic!( "Tvix bug: VM responded with incorrect generator message: {}", msg @@ -776,7 +776,7 @@ pub(crate) async fn request_to_json( ) -> Result { match co.yield_(VMRequest::ToJson(value)).await { VMResponse::Value(Value::Json(json)) => Ok(*json), - VMResponse::Value(Value::Catchable(cek)) => Err(cek), + VMResponse::Value(Value::Catchable(cek)) => Err(*cek), msg => panic!( "Tvix bug: VM responded with incorrect generator message: {}", msg diff --git a/tvix/eval/src/vm/macros.rs b/tvix/eval/src/vm/macros.rs index fdb812961..d8a09706a 100644 --- a/tvix/eval/src/vm/macros.rs +++ b/tvix/eval/src/vm/macros.rs @@ -44,7 +44,7 @@ macro_rules! cmp_op { let span = generators::request_span(&co).await; let ordering = a.nix_cmp_ordering(b, co, span).await?; match ordering { - Err(cek) => Ok(Value::Catchable(cek)), + Err(cek) => Ok(Value::from(cek)), Ok(ordering) => Ok(Value::Bool(cmp_op!(@order $op ordering))), } } diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index 7c2f5b979..0ca72bdf3 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/tvix/eval/src/vm/mod.rs @@ -901,7 +901,7 @@ where OpCode::OpAssertFail => { self.stack - .push(Value::Catchable(CatchableErrorKind::AssertionFailed)); + .push(Value::from(CatchableErrorKind::AssertionFailed)); } // Data-carrying operands should never be executed, @@ -1250,7 +1250,7 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result { path.push(vs.to_os_str()?); crate::value::canon_path(PathBuf::from(path)).into() } - Err(c) => Value::Catchable(c), + Err(c) => Value::Catchable(Box::new(c)), } } (Value::String(s1), Value::String(s2)) => Value::String(Box::new(s1.concat(&s2))), @@ -1288,8 +1288,8 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result { .await; match (r1, r2) { (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)), + (Err(c), _) => return Ok(Value::from(c)), + (_, Err(c)) => return Ok(Value::from(c)), } } }; diff --git a/tvix/glue/src/builtins/derivation.rs b/tvix/glue/src/builtins/derivation.rs index 299d064c4..efcacd831 100644 --- a/tvix/glue/src/builtins/derivation.rs +++ b/tvix/glue/src/builtins/derivation.rs @@ -248,7 +248,7 @@ pub(crate) mod derivation_builtins { "args" => { for arg in value.to_list()? { match strong_importing_coerce_to_string(&co, arg).await { - Err(cek) => return Ok(Value::Catchable(cek)), + Err(cek) => return Ok(Value::from(cek)), Ok(s) => { input_context.mimic(&s); drv.arguments.push(s.to_str()?.to_owned()) @@ -299,7 +299,7 @@ pub(crate) mod derivation_builtins { // handle builder and system. "builder" | "system" => { match strong_importing_coerce_to_string(&co, value).await { - Err(cek) => return Ok(Value::Catchable(cek)), + Err(cek) => return Ok(Value::from(cek)), Ok(val_str) => { input_context.mimic(&val_str); @@ -334,7 +334,7 @@ pub(crate) mod derivation_builtins { // In non-SA case, coerce to string and add to env. if let Some(ref mut structured_attrs) = structured_attrs { let val = generators::request_force(&co, value).await; - if matches!(val, Value::Catchable(_)) { + if val.is_catchable() { return Ok(val); } @@ -343,14 +343,14 @@ pub(crate) mod derivation_builtins { let val_json = match val.into_json(&co).await? { Ok(v) => v, - Err(cek) => return Ok(Value::Catchable(cek)), + Err(cek) => return Ok(Value::from(cek)), }; // No need to check for dups, we only iterate over every attribute name once structured_attrs.insert(arg_name.to_owned(), val_json); } else { match strong_importing_coerce_to_string(&co, value).await { - Err(cek) => return Ok(Value::Catchable(cek)), + Err(cek) => return Ok(Value::from(cek)), Ok(val_str) => { input_context.mimic(&val_str); @@ -384,21 +384,21 @@ pub(crate) mod derivation_builtins { .await .context("evaluating the `outputHash` parameter")? { - Err(cek) => return Ok(Value::Catchable(cek)), + Err(cek) => return Ok(Value::from(cek)), Ok(s) => s, }; let output_hash_algo = match select_string(&co, &input, "outputHashAlgo") .await .context("evaluating the `outputHashAlgo` parameter")? { - Err(cek) => return Ok(Value::Catchable(cek)), + Err(cek) => return Ok(Value::from(cek)), Ok(s) => s, }; let output_hash_mode = match select_string(&co, &input, "outputHashMode") .await .context("evaluating the `outputHashMode` parameter")? { - Err(cek) => return Ok(Value::Catchable(cek)), + Err(cek) => return Ok(Value::from(cek)), Ok(s) => s, };