revert(tvix/eval): Don't double-box Path values
This reverts commit d3d41552cf
.
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 <root@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
This commit is contained in:
parent
e3c92ac3b4
commit
dd26177319
6 changed files with 20 additions and 23 deletions
|
@ -53,7 +53,7 @@ pub async fn coerce_value_to_path(
|
||||||
) -> Result<Result<PathBuf, CatchableErrorKind>, ErrorKind> {
|
) -> Result<Result<PathBuf, CatchableErrorKind>, ErrorKind> {
|
||||||
let value = generators::request_force(co, v).await;
|
let value = generators::request_force(co, v).await;
|
||||||
if let Value::Path(p) = value {
|
if let Value::Path(p) = value {
|
||||||
return Ok(Ok(p.into()));
|
return Ok(Ok(*p));
|
||||||
}
|
}
|
||||||
|
|
||||||
match generators::request_string_coerce(
|
match generators::request_string_coerce(
|
||||||
|
@ -348,8 +348,8 @@ mod pure_builtins {
|
||||||
})
|
})
|
||||||
.unwrap_or(b".");
|
.unwrap_or(b".");
|
||||||
if is_path {
|
if is_path {
|
||||||
Ok(Value::from(PathBuf::from(OsString::assert_from_raw_vec(
|
Ok(Value::Path(Box::new(PathBuf::from(
|
||||||
result.to_owned(),
|
OsString::assert_from_raw_vec(result.to_owned()),
|
||||||
))))
|
))))
|
||||||
} else {
|
} else {
|
||||||
Ok(Value::from(NixString::new_inherit_context_from(
|
Ok(Value::from(NixString::new_inherit_context_from(
|
||||||
|
@ -1586,7 +1586,7 @@ mod placeholder_builtins {
|
||||||
let res = [
|
let res = [
|
||||||
("line", 42.into()),
|
("line", 42.into()),
|
||||||
("col", 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())))
|
Ok(Value::attrs(NixAttrs::from_iter(res.into_iter())))
|
||||||
}
|
}
|
||||||
|
|
|
@ -390,7 +390,7 @@ impl Compiler<'_> {
|
||||||
|
|
||||||
let home_relative_path = &raw_path[2..(raw_path.len())];
|
let home_relative_path = &raw_path[2..(raw_path.len())];
|
||||||
self.emit_constant(
|
self.emit_constant(
|
||||||
Value::UnresolvedPath(PathBuf::from(home_relative_path).into_boxed_path()),
|
Value::UnresolvedPath(Box::new(home_relative_path.into())),
|
||||||
node,
|
node,
|
||||||
);
|
);
|
||||||
self.push_op(OpCode::OpResolveHomePath, node);
|
self.push_op(OpCode::OpResolveHomePath, node);
|
||||||
|
@ -408,10 +408,7 @@ impl Compiler<'_> {
|
||||||
let path = &raw_path[1..(raw_path.len() - 1)];
|
let path = &raw_path[1..(raw_path.len() - 1)];
|
||||||
// Make a thunk to resolve the path (without using `findFile`, at least for now?)
|
// Make a thunk to resolve the path (without using `findFile`, at least for now?)
|
||||||
return self.thunk(slot, node, move |c, _| {
|
return self.thunk(slot, node, move |c, _| {
|
||||||
c.emit_constant(
|
c.emit_constant(Value::UnresolvedPath(Box::new(path.into())), node);
|
||||||
Value::UnresolvedPath(PathBuf::from(path).into_boxed_path()),
|
|
||||||
node,
|
|
||||||
);
|
|
||||||
c.push_op(OpCode::OpFindFile, node);
|
c.push_op(OpCode::OpFindFile, node);
|
||||||
});
|
});
|
||||||
} else {
|
} else {
|
||||||
|
@ -422,7 +419,7 @@ impl Compiler<'_> {
|
||||||
|
|
||||||
// TODO: Use https://github.com/rust-lang/rfcs/issues/2208
|
// TODO: Use https://github.com/rust-lang/rfcs/issues/2208
|
||||||
// once it is available
|
// 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);
|
self.emit_constant(value, node);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -2,7 +2,7 @@
|
||||||
|
|
||||||
use imbl::proptest::{ord_map, vector};
|
use imbl::proptest::{ord_map, vector};
|
||||||
use proptest::{prelude::*, strategy::BoxedStrategy};
|
use proptest::{prelude::*, strategy::BoxedStrategy};
|
||||||
use std::{ffi::OsString, path::PathBuf};
|
use std::ffi::OsString;
|
||||||
|
|
||||||
use super::{attrs::AttrsRep, NixAttrs, NixList, NixString, Value};
|
use super::{attrs::AttrsRep, NixAttrs, NixList, NixString, Value};
|
||||||
|
|
||||||
|
@ -92,7 +92,7 @@ fn leaf_value() -> impl Strategy<Value = Value> {
|
||||||
any::<i64>().prop_map(Integer),
|
any::<i64>().prop_map(Integer),
|
||||||
any::<f64>().prop_map(Float),
|
any::<f64>().prop_map(Float),
|
||||||
any::<Box<NixString>>().prop_map(String),
|
any::<Box<NixString>>().prop_map(String),
|
||||||
any::<OsString>().prop_map(|s| Path(PathBuf::from(s).into_boxed_path())),
|
any::<OsString>().prop_map(|s| Path(Box::new(s.into()))),
|
||||||
]
|
]
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -27,7 +27,7 @@ impl Value {
|
||||||
Value::String(s) => Json::String(s.to_str()?.to_owned()),
|
Value::String(s) => Json::String(s.to_str()?.to_owned()),
|
||||||
|
|
||||||
Value::Path(p) => {
|
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())
|
Json::String(imported.to_string_lossy().to_string())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -3,7 +3,7 @@
|
||||||
use std::cmp::Ordering;
|
use std::cmp::Ordering;
|
||||||
use std::fmt::Display;
|
use std::fmt::Display;
|
||||||
use std::num::{NonZeroI32, NonZeroUsize};
|
use std::num::{NonZeroI32, NonZeroUsize};
|
||||||
use std::path::{Path, PathBuf};
|
use std::path::PathBuf;
|
||||||
use std::rc::Rc;
|
use std::rc::Rc;
|
||||||
|
|
||||||
use bstr::{BString, ByteVec};
|
use bstr::{BString, ByteVec};
|
||||||
|
@ -50,7 +50,7 @@ pub enum Value {
|
||||||
String(Box<NixString>),
|
String(Box<NixString>),
|
||||||
|
|
||||||
#[serde(skip)]
|
#[serde(skip)]
|
||||||
Path(Box<Path>),
|
Path(Box<PathBuf>),
|
||||||
Attrs(Box<NixAttrs>),
|
Attrs(Box<NixAttrs>),
|
||||||
List(NixList),
|
List(NixList),
|
||||||
|
|
||||||
|
@ -76,7 +76,7 @@ pub enum Value {
|
||||||
#[serde(skip)]
|
#[serde(skip)]
|
||||||
DeferredUpvalue(StackIdx),
|
DeferredUpvalue(StackIdx),
|
||||||
#[serde(skip)]
|
#[serde(skip)]
|
||||||
UnresolvedPath(Box<Path>),
|
UnresolvedPath(Box<PathBuf>),
|
||||||
#[serde(skip)]
|
#[serde(skip)]
|
||||||
Json(Box<serde_json::Value>),
|
Json(Box<serde_json::Value>),
|
||||||
|
|
||||||
|
@ -351,7 +351,7 @@ impl Value {
|
||||||
import_paths: true, ..
|
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
|
// When we import a path from the evaluator, we must attach
|
||||||
// its original path as its context.
|
// its original path as its context.
|
||||||
context = context.append(NixContextElement::Plain(
|
context = context.append(NixContextElement::Plain(
|
||||||
|
@ -365,7 +365,7 @@ impl Value {
|
||||||
import_paths: false,
|
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
|
// Attribute sets can be converted to strings if they either have an
|
||||||
// `__toString` attribute which holds a function that receives the
|
// `__toString` attribute which holds a function that receives the
|
||||||
|
@ -707,7 +707,7 @@ impl Value {
|
||||||
Value::String(s),
|
Value::String(s),
|
||||||
(**s).clone()
|
(**s).clone()
|
||||||
);
|
);
|
||||||
gen_cast!(to_path, Box<Path>, "path", Value::Path(p), p.clone());
|
gen_cast!(to_path, Box<PathBuf>, "path", Value::Path(p), p.clone());
|
||||||
gen_cast!(to_attrs, Box<NixAttrs>, "set", Value::Attrs(a), a.clone());
|
gen_cast!(to_attrs, Box<NixAttrs>, "set", Value::Attrs(a), a.clone());
|
||||||
gen_cast!(to_list, NixList, "list", Value::List(l), l.clone());
|
gen_cast!(to_list, NixList, "list", Value::List(l), l.clone());
|
||||||
gen_cast!(
|
gen_cast!(
|
||||||
|
@ -1018,7 +1018,7 @@ impl From<f64> for Value {
|
||||||
|
|
||||||
impl From<PathBuf> for Value {
|
impl From<PathBuf> for Value {
|
||||||
fn from(path: PathBuf) -> Self {
|
fn from(path: PathBuf) -> Self {
|
||||||
Self::Path(path.into_boxed_path())
|
Self::Path(Box::new(path))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -862,7 +862,7 @@ where
|
||||||
Value::UnresolvedPath(path) => {
|
Value::UnresolvedPath(path) => {
|
||||||
let resolved = self
|
let resolved = self
|
||||||
.nix_search_path
|
.nix_search_path
|
||||||
.resolve(&self.io_handle, path)
|
.resolve(&self.io_handle, *path)
|
||||||
.with_span(&frame, self)?;
|
.with_span(&frame, self)?;
|
||||||
self.stack.push(resolved.into());
|
self.stack.push(resolved.into());
|
||||||
}
|
}
|
||||||
|
@ -882,7 +882,7 @@ where
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
Some(mut buf) => {
|
Some(mut buf) => {
|
||||||
buf.push(path);
|
buf.push(*path);
|
||||||
self.stack.push(buf.into());
|
self.stack.push(buf.into());
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
@ -1225,7 +1225,7 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result<Value, ErrorKind> {
|
||||||
// What we try to do is solely determined by the type of the first value!
|
// What we try to do is solely determined by the type of the first value!
|
||||||
let result = match (a, b) {
|
let result = match (a, b) {
|
||||||
(Value::Path(p), v) => {
|
(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(
|
match generators::request_string_coerce(
|
||||||
&co,
|
&co,
|
||||||
v,
|
v,
|
||||||
|
|
Loading…
Reference in a new issue