From 72ece2e5184df7cb2099e54ace01154a4043d289 Mon Sep 17 00:00:00 2001 From: Adam Joseph Date: Sat, 25 Nov 2023 01:38:00 -0800 Subject: [PATCH] feat(tvix/eval): nonrecursive nix_eq() This commit rewrites Value::nix_eq() into an equivalent. Except for calls to Thunk::force(), the new form no longer uses generators, and is async only because of the fact that it calls Thunk::force(). I believed that the nonrecursive form would be faster. It is, in fact, slightly slower. I believe this is due to the vec![] allocation; I am investigating. Prev-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"459068","system-seconds":"0.71","user-seconds":"5.39"} This-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"460048","system-seconds":"0.68","user-seconds":"5.73"} Change-Id: I10f4868891e4b7475df13f0cbc41ec78dd985dd8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10118 Reviewed-by: tazjin Tested-by: BuildkiteCI Autosubmit: Adam Joseph --- tvix/eval/src/value/mod.rs | 289 +++++++++++++++++---------------- tvix/eval/src/value/thunk.rs | 11 +- tvix/eval/src/vm/generators.rs | 12 +- tvix/eval/src/vm/mod.rs | 4 +- 4 files changed, 169 insertions(+), 147 deletions(-) diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index 2fb27d8dc..f4ed7733e 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -375,7 +375,6 @@ impl Value { } } - // TODO(amjoseph): de-asyncify this (when called directly by the VM) /// Compare two Nix values for equality, forcing nested parts of the structure /// as needed. /// @@ -391,156 +390,163 @@ impl Value { other: Value, co: GenCo, ptr_eq: PointerEquality, + span: LightSpan, ) -> Result { - let a = match self { - Value::Thunk(ref thunk) => { - // If both values are thunks, and thunk comparisons are allowed by - // pointer, do that and move on. - if ptr_eq == PointerEquality::AllowAll { - if let Value::Thunk(t1) = &other { - if t1.ptr_eq(thunk) { - return Ok(Value::Bool(true)); - } - } - }; + // this is a stack of ((v1,v2),peq) triples to be compared; + // after each triple is popped off of the stack, v1 is + // compared to v2 using peq-mode PointerEquality + let mut vals = vec![((self, other), ptr_eq)]; - generators::request_force(&co, self).await - } - - _ => self, - }; - - let b = match other { - Value::Thunk(_) => generators::request_force(&co, other).await, - _ => other, - }; - - debug_assert!(!matches!(a, Value::Thunk(_))); - debug_assert!(!matches!(b, Value::Thunk(_))); - - let result = match (a, b) { - // Trivial comparisons - (c @ Value::Catchable(_), _) => return Ok(c), - (_, c @ Value::Catchable(_)) => return Ok(c), - (Value::Null, Value::Null) => true, - (Value::Bool(b1), Value::Bool(b2)) => b1 == b2, - (Value::String(s1), Value::String(s2)) => s1 == s2, - (Value::Path(p1), Value::Path(p2)) => p1 == p2, - - // Numerical comparisons (they work between float & int) - (Value::Integer(i1), Value::Integer(i2)) => i1 == i2, - (Value::Integer(i), Value::Float(f)) => i as f64 == f, - (Value::Float(f1), Value::Float(f2)) => f1 == f2, - (Value::Float(f), Value::Integer(i)) => i as f64 == f, - - // List comparisons - (Value::List(l1), Value::List(l2)) => { - if ptr_eq >= PointerEquality::AllowNested && l1.ptr_eq(&l2) { - return Ok(Value::Bool(true)); - } - - if l1.len() != l2.len() { - return Ok(Value::Bool(false)); - } - - for (vi1, vi2) in l1.into_iter().zip(l2.into_iter()) { - if !generators::check_equality( - &co, - vi1, - vi2, - std::cmp::max(ptr_eq, PointerEquality::AllowNested), - ) - .await? - { - return Ok(Value::Bool(false)); - } - } - - true - } - - (_, Value::List(_)) | (Value::List(_), _) => false, - - // Attribute set comparisons - (Value::Attrs(a1), Value::Attrs(a2)) => { - if ptr_eq >= PointerEquality::AllowNested && a1.ptr_eq(&a2) { - return Ok(Value::Bool(true)); - } - - // Special-case for derivation comparisons: If both attribute sets - // have `type = derivation`, compare them by `outPath`. - #[allow(clippy::single_match)] // might need more match arms later - match (a1.select("type"), a2.select("type")) { - (Some(v1), Some(v2)) => { - let s1 = generators::request_force(&co, v1.clone()).await.to_str(); - let s2 = generators::request_force(&co, v2.clone()).await.to_str(); - - if let (Ok(s1), Ok(s2)) = (s1, s2) { - if s1.as_str() == "derivation" && s2.as_str() == "derivation" { - // TODO(tazjin): are the outPaths really required, - // or should it fall through? - let out1 = a1 - .select_required("outPath") - .context("comparing derivations")? - .clone(); - - let out2 = a2 - .select_required("outPath") - .context("comparing derivations")? - .clone(); - - let result = generators::request_force(&co, out1.clone()) - .await - .to_str()? - == generators::request_force(&co, out2.clone()) - .await - .to_str()?; - return Ok(Value::Bool(result)); + loop { + let ((a, b), ptr_eq) = if let Some(abp) = vals.pop() { + abp + } else { + // stack is empty, so comparison has succeeded + return Ok(Value::Bool(true)); + }; + let a = match a { + Value::Thunk(thunk) => { + // If both values are thunks, and thunk comparisons are allowed by + // pointer, do that and move on. + if ptr_eq == PointerEquality::AllowAll { + if let Value::Thunk(t1) = &b { + if t1.ptr_eq(&thunk) { + continue; } } - } - _ => {} - }; + }; - if a1.len() != a2.len() { - return Ok(Value::Bool(false)); + Thunk::force_(thunk, &co, span.clone()).await? } - let iter1 = a1.iter_sorted(); - let iter2 = a2.iter_sorted(); + _ => a, + }; - for ((k1, v1), (k2, v2)) in iter1.zip(iter2) { - if k1 != k2 { + let b = b.force(&co, span.clone()).await?; + + debug_assert!(!matches!(a, Value::Thunk(_))); + debug_assert!(!matches!(b, Value::Thunk(_))); + + let result = match (a, b) { + // Trivial comparisons + (c @ Value::Catchable(_), _) => return Ok(c), + (_, c @ Value::Catchable(_)) => return Ok(c), + (Value::Null, Value::Null) => true, + (Value::Bool(b1), Value::Bool(b2)) => b1 == b2, + (Value::String(s1), Value::String(s2)) => s1 == s2, + (Value::Path(p1), Value::Path(p2)) => p1 == p2, + + // Numerical comparisons (they work between float & int) + (Value::Integer(i1), Value::Integer(i2)) => i1 == i2, + (Value::Integer(i), Value::Float(f)) => i as f64 == f, + (Value::Float(f1), Value::Float(f2)) => f1 == f2, + (Value::Float(f), Value::Integer(i)) => i as f64 == f, + + // List comparisons + (Value::List(l1), Value::List(l2)) => { + if ptr_eq >= PointerEquality::AllowNested && l1.ptr_eq(&l2) { + continue; + } + + if l1.len() != l2.len() { return Ok(Value::Bool(false)); } - if !generators::check_equality( - &co, - v1.clone(), - v2.clone(), - std::cmp::max(ptr_eq, PointerEquality::AllowNested), - ) - .await? - { + vals.extend(l1.into_iter().rev().zip(l2.into_iter().rev()).zip( + std::iter::repeat(std::cmp::max(ptr_eq, PointerEquality::AllowNested)), + )); + continue; + } + + (_, Value::List(_)) | (Value::List(_), _) => return Ok(Value::Bool(false)), + + // Attribute set comparisons + (Value::Attrs(a1), Value::Attrs(a2)) => { + if ptr_eq >= PointerEquality::AllowNested && a1.ptr_eq(&a2) { + continue; + } + + // Special-case for derivation comparisons: If both attribute sets + // have `type = derivation`, compare them by `outPath`. + #[allow(clippy::single_match)] // might need more match arms later + match (a1.select("type"), a2.select("type")) { + (Some(v1), Some(v2)) => { + let s1 = v1.clone().force(&co, span.clone()).await?.to_str(); + let s2 = v2.clone().force(&co, span.clone()).await?.to_str(); + + if let (Ok(s1), Ok(s2)) = (s1, s2) { + if s1.as_str() == "derivation" && s2.as_str() == "derivation" { + // TODO(tazjin): are the outPaths really required, + // or should it fall through? + let out1 = a1 + .select_required("outPath") + .context("comparing derivations")? + .clone(); + + let out2 = a2 + .select_required("outPath") + .context("comparing derivations")? + .clone(); + + let result = out1 + .clone() + .force(&co, span.clone()) + .await? + .to_str()? + == out2.clone().force(&co, span.clone()).await?.to_str()?; + if !result { + return Ok(Value::Bool(false)); + } else { + continue; + } + } + } + } + _ => {} + }; + + if a1.len() != a2.len() { + return Ok(Value::Bool(false)); + } + + // note that it is important to be careful here with the + // order we push the keys and values in order to properly + // compare attrsets containing `throw` elements. + let iter1 = a1.into_iter_sorted().rev(); + let iter2 = a2.into_iter_sorted().rev(); + for ((k1, v1), (k2, v2)) in iter1.zip(iter2) { + vals.push(( + (v1, v2), + std::cmp::max(ptr_eq, PointerEquality::AllowNested), + )); + vals.push(( + (k1.into(), k2.into()), + std::cmp::max(ptr_eq, PointerEquality::AllowNested), + )); + } + continue; + } + + (Value::Attrs(_), _) | (_, Value::Attrs(_)) => return Ok(Value::Bool(false)), + + (Value::Closure(c1), Value::Closure(c2)) + if ptr_eq >= PointerEquality::AllowNested => + { + if Rc::ptr_eq(&c1, &c2) { + continue; + } else { return Ok(Value::Bool(false)); } } - true + // Everything else is either incomparable (e.g. internal types) or + // false. + _ => return Ok(Value::Bool(false)), + }; + if !result { + return Ok(Value::Bool(false)); } - - (Value::Attrs(_), _) | (_, Value::Attrs(_)) => false, - - (Value::Closure(c1), Value::Closure(c2)) if ptr_eq >= PointerEquality::AllowNested => { - Rc::ptr_eq(&c1, &c2) - } - - // Everything else is either incomparable (e.g. internal types) or - // false. - _ => false, - }; - - Ok(Value::Bool(result)) + } } pub fn type_of(&self) -> &'static str { @@ -660,10 +666,19 @@ impl Value { } // TODO(amjoseph): de-asyncify this (when called directly by the VM) - pub async fn force(self, co: GenCo, span: LightSpan) -> Result { + pub async fn force(self, co: &GenCo, span: LightSpan) -> Result { if let Value::Thunk(thunk) = self { // TODO(amjoseph): use #[tailcall::mutual] - return Thunk::force(thunk, co, span).await; + return Thunk::force_(thunk, co, span).await; + } + Ok(self) + } + + // need two flavors, because async + pub async fn force_owned_genco(self, co: GenCo, span: LightSpan) -> Result { + if let Value::Thunk(thunk) = self { + // TODO(amjoseph): use #[tailcall::mutual] + return Thunk::force_(thunk, &co, span).await; } Ok(self) } diff --git a/tvix/eval/src/value/thunk.rs b/tvix/eval/src/value/thunk.rs index 3ff3bde65..345903237 100644 --- a/tvix/eval/src/value/thunk.rs +++ b/tvix/eval/src/value/thunk.rs @@ -205,7 +205,14 @@ impl Thunk { } } - pub async fn force(mut myself: Thunk, co: GenCo, span: LightSpan) -> Result { + pub async fn force(myself: Thunk, co: GenCo, span: LightSpan) -> Result { + Self::force_(myself, &co, span).await + } + pub async fn force_( + mut myself: Thunk, + co: &GenCo, + span: LightSpan, + ) -> Result { // This vector of "thunks which point to the thunk-being-forced", to // be updated along with it, is necessary in order to write this // function in iterative (and later, mutual-tail-call) form. @@ -261,7 +268,7 @@ impl Thunk { // be turned into a tailcall to vm::execute_bytecode() by // passing `also_update` to it. let value = - generators::request_enter_lambda(&co, lambda, upvalues, light_span).await; + generators::request_enter_lambda(co, lambda, upvalues, light_span).await; myself.0.replace(ThunkRepr::Evaluated(value)); continue; } diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs index 1b6029e80..3a256ec55 100644 --- a/tvix/eval/src/vm/generators.rs +++ b/tvix/eval/src/vm/generators.rs @@ -288,7 +288,7 @@ impl<'o> VM<'o> { VMRequest::ForceValue(value) => { self.reenqueue_generator(name, span.clone(), generator); self.enqueue_generator("force", span.clone(), |co| { - value.force(co, span) + value.force_owned_genco(co, span) }); return Ok(false); } @@ -310,7 +310,7 @@ impl<'o> VM<'o> { let value = self.stack[self.with_stack[idx]].clone(); self.enqueue_generator("force", span.clone(), |co| { - value.force(co, span) + value.force_owned_genco(co, span) }); return Ok(false); @@ -327,7 +327,7 @@ impl<'o> VM<'o> { let value = call_frame.upvalues.with_stack().unwrap()[idx].clone(); self.enqueue_generator("force", span.clone(), |co| { - value.force(co, span) + value.force_owned_genco(co, span) }); return Ok(false); @@ -336,8 +336,8 @@ impl<'o> VM<'o> { VMRequest::NixEquality(values, ptr_eq) => { let values = *values; self.reenqueue_generator(name, span.clone(), generator); - self.enqueue_generator("nix_eq", span, |co| { - values.0.nix_eq(values.1, co, ptr_eq) + self.enqueue_generator("nix_eq", span.clone(), |co| { + values.0.nix_eq(values.1, co, ptr_eq, span) }); return Ok(false); } @@ -465,7 +465,7 @@ impl<'o> VM<'o> { ); self.enqueue_generator("force", span.clone(), |co| { - value.force(co, span) + value.force_owned_genco(co, span) }); return Ok(false); } diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs index 6aad8719e..615d77c0e 100644 --- a/tvix/eval/src/vm/mod.rs +++ b/tvix/eval/src/vm/mod.rs @@ -615,8 +615,8 @@ impl<'o> VM<'o> { let a = self.stack_pop(); let gen_span = frame.current_light_span(); self.push_call_frame(span, frame); - self.enqueue_generator("nix_eq", gen_span, |co| { - a.nix_eq(b, co, PointerEquality::ForbidAll) + self.enqueue_generator("nix_eq", gen_span.clone(), |co| { + a.nix_eq(b, co, PointerEquality::ForbidAll, gen_span) }); return Ok(false); }