From dd261773192d0928571f806892d8065fbba1cf2d Mon Sep 17 00:00:00 2001 From: Aspen Smith Date: Sat, 10 Feb 2024 12:35:44 -0500 Subject: revert(tvix/eval): Don't double-box Path values This reverts commit d3d41552cf1f6485f8ebc597a2128a0d15b030a5. This was well-intentioned, but now the boxed Path values are actually the *largest* Value enum variants, at 16 bytes (because they're fat-pointers, with a len) instead of 8 bytes like all the other values. Having the double reference is a reasonable price to pay (it seems; more benchmarks may end up disagreeing) for a smaller Value repr. Change-Id: I0d3e84f646c8f5ffd0b7259c4e456637eea360f7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10797 Tested-by: BuildkiteCI Autosubmit: aspen Reviewed-by: sterni --- tvix/eval/src/builtins/mod.rs | 8 ++++---- tvix/eval/src/compiler/mod.rs | 9 +++------ tvix/eval/src/value/arbitrary.rs | 4 ++-- tvix/eval/src/value/json.rs | 2 +- tvix/eval/src/value/mod.rs | 14 +++++++------- tvix/eval/src/vm/mod.rs | 6 +++--- 6 files changed, 20 insertions(+), 23 deletions(-) diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index a0b990ec542c..d63b035c053e 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -53,7 +53,7 @@ pub async fn coerce_value_to_path( ) -> Result, ErrorKind> { let value = generators::request_force(co, v).await; if let Value::Path(p) = value { - return Ok(Ok(p.into())); + return Ok(Ok(*p)); } match generators::request_string_coerce( @@ -348,8 +348,8 @@ mod pure_builtins { }) .unwrap_or(b"."); if is_path { - Ok(Value::from(PathBuf::from(OsString::assert_from_raw_vec( - result.to_owned(), + Ok(Value::Path(Box::new(PathBuf::from( + OsString::assert_from_raw_vec(result.to_owned()), )))) } else { Ok(Value::from(NixString::new_inherit_context_from( @@ -1586,7 +1586,7 @@ mod placeholder_builtins { let res = [ ("line", 42.into()), ("col", 42.into()), - ("file", Value::from(PathBuf::from("/deep/thought"))), + ("file", Value::Path(Box::new("/deep/thought".into()))), ]; Ok(Value::attrs(NixAttrs::from_iter(res.into_iter()))) } diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index 4a7b5fd5ba9c..0a9ee2320b1d 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/tvix/eval/src/compiler/mod.rs @@ -390,7 +390,7 @@ impl Compiler<'_> { let home_relative_path = &raw_path[2..(raw_path.len())]; self.emit_constant( - Value::UnresolvedPath(PathBuf::from(home_relative_path).into_boxed_path()), + Value::UnresolvedPath(Box::new(home_relative_path.into())), node, ); self.push_op(OpCode::OpResolveHomePath, node); @@ -408,10 +408,7 @@ impl Compiler<'_> { let path = &raw_path[1..(raw_path.len() - 1)]; // Make a thunk to resolve the path (without using `findFile`, at least for now?) return self.thunk(slot, node, move |c, _| { - c.emit_constant( - Value::UnresolvedPath(PathBuf::from(path).into_boxed_path()), - node, - ); + c.emit_constant(Value::UnresolvedPath(Box::new(path.into())), node); c.push_op(OpCode::OpFindFile, node); }); } else { @@ -422,7 +419,7 @@ impl Compiler<'_> { // TODO: Use https://github.com/rust-lang/rfcs/issues/2208 // once it is available - let value = Value::Path(crate::value::canon_path(path).into_boxed_path()); + let value = Value::Path(Box::new(crate::value::canon_path(path))); self.emit_constant(value, node); } diff --git a/tvix/eval/src/value/arbitrary.rs b/tvix/eval/src/value/arbitrary.rs index a2e8cb899c92..c14fbae6cb71 100644 --- a/tvix/eval/src/value/arbitrary.rs +++ b/tvix/eval/src/value/arbitrary.rs @@ -2,7 +2,7 @@ use imbl::proptest::{ord_map, vector}; use proptest::{prelude::*, strategy::BoxedStrategy}; -use std::{ffi::OsString, path::PathBuf}; +use std::ffi::OsString; use super::{attrs::AttrsRep, NixAttrs, NixList, NixString, Value}; @@ -92,7 +92,7 @@ fn leaf_value() -> impl Strategy { any::().prop_map(Integer), any::().prop_map(Float), any::>().prop_map(String), - any::().prop_map(|s| Path(PathBuf::from(s).into_boxed_path())), + any::().prop_map(|s| Path(Box::new(s.into()))), ] } diff --git a/tvix/eval/src/value/json.rs b/tvix/eval/src/value/json.rs index 5c627540dbe8..efd2763f1c36 100644 --- a/tvix/eval/src/value/json.rs +++ b/tvix/eval/src/value/json.rs @@ -27,7 +27,7 @@ impl Value { Value::String(s) => Json::String(s.to_str()?.to_owned()), Value::Path(p) => { - let imported = generators::request_path_import(co, p.into_path_buf()).await; + let imported = generators::request_path_import(co, *p).await; Json::String(imported.to_string_lossy().to_string()) } diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index 043788da4596..3407dd52029f 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -3,7 +3,7 @@ use std::cmp::Ordering; use std::fmt::Display; use std::num::{NonZeroI32, NonZeroUsize}; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::rc::Rc; use bstr::{BString, ByteVec}; @@ -50,7 +50,7 @@ pub enum Value { String(Box), #[serde(skip)] - Path(Box), + Path(Box), Attrs(Box), List(NixList), @@ -76,7 +76,7 @@ pub enum Value { #[serde(skip)] DeferredUpvalue(StackIdx), #[serde(skip)] - UnresolvedPath(Box), + UnresolvedPath(Box), #[serde(skip)] Json(Box), @@ -351,7 +351,7 @@ impl Value { import_paths: true, .. }, ) => { - let imported = generators::request_path_import(co, p.to_path_buf()).await; + let imported = generators::request_path_import(co, *p).await; // When we import a path from the evaluator, we must attach // its original path as its context. context = context.append(NixContextElement::Plain( @@ -365,7 +365,7 @@ impl Value { import_paths: false, .. }, - ) => Ok((*p).as_os_str().as_encoded_bytes().into()), + ) => Ok(p.into_os_string().into_encoded_bytes().into()), // Attribute sets can be converted to strings if they either have an // `__toString` attribute which holds a function that receives the @@ -707,7 +707,7 @@ impl Value { Value::String(s), (**s).clone() ); - gen_cast!(to_path, Box, "path", Value::Path(p), p.clone()); + gen_cast!(to_path, Box, "path", Value::Path(p), p.clone()); gen_cast!(to_attrs, Box, "set", Value::Attrs(a), a.clone()); gen_cast!(to_list, NixList, "list", Value::List(l), l.clone()); gen_cast!( @@ -1018,7 +1018,7 @@ impl From for Value { impl From for Value { fn from(path: PathBuf) -> Self { - Self::Path(path.into_boxed_path()) + Self::Path(Box::new(path)) } } diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index 6483122f96f6..7c2f5b97960f 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/tvix/eval/src/vm/mod.rs @@ -862,7 +862,7 @@ where Value::UnresolvedPath(path) => { let resolved = self .nix_search_path - .resolve(&self.io_handle, path) + .resolve(&self.io_handle, *path) .with_span(&frame, self)?; self.stack.push(resolved.into()); } @@ -882,7 +882,7 @@ where ); } Some(mut buf) => { - buf.push(path); + buf.push(*path); self.stack.push(buf.into()); } }; @@ -1225,7 +1225,7 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result { // What we try to do is solely determined by the type of the first value! let result = match (a, b) { (Value::Path(p), v) => { - let mut path = p.as_os_str().to_owned(); + let mut path = p.into_os_string(); match generators::request_string_coerce( &co, v, -- cgit 1.4.1