From 610c44ec1ec2eaf58e5c36d7007d6c1922e49804 Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Thu, 29 Dec 2022 15:50:19 +0300 Subject: refactor(tvix/eval): use im::Vector directly where possible The conversion from im::Vector -> Vec is cheaper for NixList construction (of course), so where possible we should make use of that. This updates most builtins dealing with lists to use Vector directly, and marks the function constructing NixList from Vec as deprecated so that we get appropriate warnings in places where it's still in use. These places are currently inside of JSON serialisation logic which is in flux right now, so lets leave them as-is until it's stabilised. Change-Id: I037f12a2800f2576db4d9526bd935efd079163f0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7671 Reviewed-by: sterni Tested-by: BuildkiteCI --- tvix/eval/src/builtins/mod.rs | 33 ++++++++++++++++++--------------- tvix/eval/src/value/list.rs | 19 +++++++++---------- tvix/eval/src/value/mod.rs | 7 ++++--- 3 files changed, 31 insertions(+), 28 deletions(-) diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index 709d2918f3c0..3708ed04d8ba 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -187,14 +187,14 @@ mod pure_builtins { .collect::, ErrorKind>>()?; Ok(Value::List(NixList::from( - lists.into_iter().flatten().collect::>(), + lists.into_iter().flatten().collect::>(), ))) } #[builtin("concatMap")] fn builtin_concat_map(vm: &mut VM, f: Value, list: Value) -> Result { let list = list.to_list()?; - let mut res = Vec::new(); + let mut res = im::Vector::new(); for val in list { res.extend(vm.call_with(&f, [val])?.force(vm)?.to_list()?); } @@ -295,7 +295,7 @@ mod pure_builtins { result }) - .collect::, _>>() + .collect::, _>>() .map(|list| Value::List(NixList::from(list))) .map_err(Into::into) } @@ -356,7 +356,7 @@ mod pure_builtins { let operator = attrs.select_required("operator")?; - let mut res: Vec = vec![]; + let mut res = im::Vector::new(); let mut done_keys: Vec = vec![]; let mut insert_key = |k: Value, vm: &mut VM| -> Result { @@ -377,7 +377,7 @@ mod pure_builtins { continue; } - res.push(val.clone()); + res.push_back(val.clone()); let op_result = vm.call_with(operator, Some(val))?.force(vm)?.to_list()?; work_set.extend(op_result.into_iter()); @@ -391,7 +391,7 @@ mod pure_builtins { let len = length.as_int()?; (0..len) .map(|i| vm.call_with(&generator, [i.into()])) - .collect::, _>>() + .collect::, _>>() .map(|list| Value::List(NixList::from(list))) .map_err(Into::into) } @@ -411,10 +411,12 @@ mod pure_builtins { #[builtin("groupBy")] fn builtin_group_by(vm: &mut VM, f: Value, list: Value) -> Result { - let mut res: BTreeMap> = BTreeMap::new(); + let mut res: BTreeMap> = BTreeMap::new(); for val in list.to_list()? { let key = vm.call_with(&f, [val.clone()])?.force(vm)?.to_str()?; - res.entry(key).or_insert_with(std::vec::Vec::new).push(val); + res.entry(key) + .or_insert_with(im::Vector::new) + .push_back(val); } Ok(Value::attrs(NixAttrs::from_iter( res.into_iter() @@ -548,7 +550,7 @@ mod pure_builtins { list.into_iter() .map(|val| vm.call_with(&f, [val])) - .collect::, _>>() + .collect::, _>>() .map(|list| Value::List(NixList::from(list))) .map_err(Into::into) } @@ -742,30 +744,30 @@ mod pure_builtins { let re: Regex = Regex::new(re.as_str()).unwrap(); let mut capture_locations = re.capture_locations(); let num_captures = capture_locations.len(); - let mut ret: Vec = vec![]; + let mut ret = im::Vector::new(); let mut pos = 0; while let Some(thematch) = re.captures_read_at(&mut capture_locations, text, pos) { // push the unmatched characters preceding the match - ret.push(Value::from(&text[pos..thematch.start()])); + ret.push_back(Value::from(&text[pos..thematch.start()])); // Push a list with one element for each capture // group in the regex, containing the characters // matched by that capture group, or null if no match. // We skip capture 0; it represents the whole match. - let v: Vec = (1..num_captures) + let v: im::Vector = (1..num_captures) .map(|i| capture_locations.get(i)) .map(|o| { o.map(|(start, end)| Value::from(&text[start..end])) .unwrap_or(Value::Null) }) .collect(); - ret.push(Value::List(NixList::from(v))); + ret.push_back(Value::List(NixList::from(v))); pos = thematch.end(); } // push the unmatched characters following the last match - ret.push(Value::from(&text[pos..])); + ret.push_back(Value::from(&text[pos..])); Ok(Value::List(NixList::from(ret))) } @@ -808,7 +810,8 @@ mod pure_builtins { }); match error { - None => Ok(Value::List(NixList::from(list))), + #[allow(deprecated)] // im::Vector usage prevented by its API + None => Ok(Value::List(NixList::from_vec(list))), Some(e) => Err(e), } } diff --git a/tvix/eval/src/value/list.rs b/tvix/eval/src/value/list.rs index 0a7ff8a58a79..0ab180a341e9 100644 --- a/tvix/eval/src/value/list.rs +++ b/tvix/eval/src/value/list.rs @@ -27,14 +27,6 @@ impl TotalDisplay for NixList { } } -// TODO(tazjin): uses of this instance are likely inefficient and can be optimised. -// Eventually this instance should be removed. -impl From> for NixList { - fn from(vs: Vec) -> Self { - Self(Vector::from_iter(vs.into_iter())) - } -} - impl From> for NixList { fn from(vs: Vector) -> Self { Self(vs) @@ -57,7 +49,9 @@ mod arbitrary { type Strategy = BoxedStrategy; fn arbitrary_with(args: Self::Parameters) -> Self::Strategy { - any_with::>(args).prop_map(|v| v.into()).boxed() + any_with::>(args) + .prop_map(NixList::from_vec) + .boxed() } } } @@ -79,7 +73,7 @@ impl NixList { stack_slice.len(), ); - stack_slice.into() + NixList(Vector::from_iter(stack_slice.into_iter())) } pub fn iter(&self) -> vector::Iter { @@ -116,6 +110,11 @@ impl NixList { pub fn into_inner(self) -> Vector { self.0 } + + #[deprecated(note = "callers should avoid constructing from Vec")] + pub fn from_vec(vs: Vec) -> Self { + Self(Vector::from_iter(vs.into_iter())) + } } impl IntoIterator for NixList { diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index 34dbf06231d4..1c6d104bd864 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -544,7 +544,7 @@ impl From for Value { impl From> for Value { fn from(val: Vec) -> Self { - Self::List(NixList::from(val)) + Self::List(NixList::from_vec(val)) } } @@ -601,6 +601,7 @@ fn type_error(expected: &'static str, actual: &Value) -> ErrorKind { #[cfg(test)] mod tests { use super::*; + use im::vector; mod nix_eq { use crate::observer::NoOpObserver; @@ -643,8 +644,8 @@ mod tests { let mut observer = NoOpObserver {}; let mut vm = VM::new(Default::default(), Box::new(crate::DummyIO), &mut observer); - let v1 = Value::List(NixList::from(vec![Value::Integer(1)])); - let v2 = Value::List(NixList::from(vec![Value::Float(1.0)])); + let v1 = Value::List(NixList::from(vector![Value::Integer(1)])); + let v2 = Value::List(NixList::from(vector![Value::Float(1.0)])); assert!(v1.nix_eq(&v2, &mut vm).unwrap()) } -- cgit 1.4.1