From 86361f0f4a754370b42ae3568ece4bc43850f36b Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Thu, 29 Dec 2022 17:34:08 +0300 Subject: refactor(tvix/eval): remove extra Rc<..> around Value::Attrs The `im::OrdMap` is already small and cheap to copy while sharing memory, so this is not required anymore. Only the `KV` variant may have slightly larger content, but in practice this doesn't seem to make a difference when comparing the two variants and this one is less complicated. Change-Id: I64a563b209a2444125653777551373cb2989ca7d Reviewed-on: https://cl.tvl.fyi/c/depot/+/7677 Reviewed-by: sterni Tested-by: BuildkiteCI --- tvix/eval/src/builtins/mod.rs | 8 ++++---- tvix/eval/src/lib.rs | 7 ------- tvix/eval/src/value/attrs.rs | 11 +++++++++++ tvix/eval/src/value/mod.rs | 6 +++--- tvix/eval/src/vm.rs | 13 ++++++------- 5 files changed, 24 insertions(+), 21 deletions(-) diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs index 9f03cd85b8e4..2e043a1b104f 100644 --- a/tvix/eval/src/builtins/mod.rs +++ b/tvix/eval/src/builtins/mod.rs @@ -21,7 +21,7 @@ use crate::{ vm::VM, }; -use crate::{arithmetic_op, unwrap_or_clone_rc}; +use crate::arithmetic_op; use self::versions::{VersionPart, VersionPartsIter}; @@ -1022,8 +1022,8 @@ fn placeholders() -> Vec { // values on the fields that a real derivation would contain. // // Crucially this means we do not yet *validate* the values either. - let attrs = unwrap_or_clone_rc(args[0].to_attrs()?); - let attrs = attrs.update(NixAttrs::from_iter( + let input = args[0].to_attrs()?; + let attrs = input.update(NixAttrs::from_iter( [ ( "outPath", @@ -1038,7 +1038,7 @@ fn placeholders() -> Vec { .into_iter(), )); - Ok(Value::Attrs(Rc::new(attrs))) + Ok(Value::Attrs(Box::new(attrs))) }, ), ] diff --git a/tvix/eval/src/lib.rs b/tvix/eval/src/lib.rs index 32624b2918a2..a467a1884c41 100644 --- a/tvix/eval/src/lib.rs +++ b/tvix/eval/src/lib.rs @@ -37,7 +37,6 @@ mod test_utils; mod tests; use std::path::PathBuf; -use std::rc::Rc; use std::str::FromStr; use std::sync::Arc; @@ -63,12 +62,6 @@ pub mod internal { pub use crate::vm::VM; } -// TODO: use Rc::unwrap_or_clone once it is stabilised. -// https://doc.rust-lang.org/std/rc/struct.Rc.html#method.unwrap_or_clone -pub(crate) fn unwrap_or_clone_rc(rc: Rc) -> T { - Rc::try_unwrap(rc).unwrap_or_else(|rc| (*rc).clone()) -} - /// An `Evaluation` represents how a piece of Nix code is evaluated. It can be /// instantiated and configured directly, or it can be accessed through the /// various simplified helper methods available below. diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs index adf56567fb0d..a41e7ce58a12 100644 --- a/tvix/eval/src/value/attrs.rs +++ b/tvix/eval/src/value/attrs.rs @@ -173,6 +173,17 @@ impl NixAttrs { Self(AttrsRep::Empty) } + /// Compare two attribute sets by pointer equality. Only makes + /// sense for some attribute set reprsentations, i.e. returning + /// `false` does not mean that the two attribute sets do not have + /// equal *content*. + pub fn ptr_eq(&self, other: &Self) -> bool { + match (&self.0, &other.0) { + (AttrsRep::Im(lhs), AttrsRep::Im(rhs)) => lhs.ptr_eq(rhs), + _ => false, + } + } + /// Return an attribute set containing the merge of the two /// provided sets. Keys from the `other` set have precedence. pub fn update(self, other: Self) -> Self { diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs index 7791a4a8c33d..b5585923a301 100644 --- a/tvix/eval/src/value/mod.rs +++ b/tvix/eval/src/value/mod.rs @@ -39,7 +39,7 @@ pub enum Value { Float(f64), String(NixString), Path(PathBuf), - Attrs(Rc), + Attrs(Box), List(NixList), Closure(Rc), // must use Rc here in order to get proper pointer equality Builtin(Builtin), @@ -154,7 +154,7 @@ where impl Value { /// Construct a [`Value::Attrs`] from a [`NixAttrs`]. pub fn attrs(attrs: NixAttrs) -> Self { - Self::Attrs(Rc::new(attrs)) + Self::Attrs(Box::new(attrs)) } } @@ -303,7 +303,7 @@ impl Value { gen_cast!(as_int, i64, "int", Value::Integer(x), *x); gen_cast!(as_float, f64, "float", Value::Float(x), *x); gen_cast!(to_str, NixString, "string", Value::String(s), s.clone()); - gen_cast!(to_attrs, Rc, "set", Value::Attrs(a), a.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!( as_closure, diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs index 5bc0f7736ae4..107aadb6e068 100644 --- a/tvix/eval/src/vm.rs +++ b/tvix/eval/src/vm.rs @@ -12,7 +12,6 @@ use crate::{ observer::RuntimeObserver, opcode::{CodeIdx, Count, JumpOffset, OpCode, StackIdx, UpvalueIdx}, spans::LightSpan, - unwrap_or_clone_rc, upvalues::Upvalues, value::{Builtin, Closure, CoercionKind, Lambda, NixAttrs, NixList, Thunk, Value}, warnings::{EvalWarning, WarningKind}, @@ -584,7 +583,7 @@ impl<'o> VM<'o> { (Value::List(_), _) => break false, (Value::Attrs(a1), Value::Attrs(a2)) => { - if allow_pointer_equality_on_functions_and_thunks && Rc::ptr_eq(&a1, &a2) { + if allow_pointer_equality_on_functions_and_thunks && a1.ptr_eq(&a2) { continue; } allow_pointer_equality_on_functions_and_thunks = true; @@ -620,8 +619,8 @@ impl<'o> VM<'o> { } _ => {} } - let iter1 = unwrap_or_clone_rc(a1).into_iter_sorted(); - let iter2 = unwrap_or_clone_rc(a2).into_iter_sorted(); + let iter1 = a1.into_iter_sorted(); + let iter2 = a2.into_iter_sorted(); if iter1.len() != iter2.len() { break false; } @@ -745,10 +744,10 @@ impl<'o> VM<'o> { OpCode::OpAttrs(Count(count)) => self.run_attrset(count)?, OpCode::OpAttrsUpdate => { - let rhs = unwrap_or_clone_rc(fallible!(self, self.pop().to_attrs())); - let lhs = unwrap_or_clone_rc(fallible!(self, self.pop().to_attrs())); + let rhs = fallible!(self, self.pop().to_attrs()); + let lhs = fallible!(self, self.pop().to_attrs()); - self.push(Value::attrs(lhs.update(rhs))) + self.push(Value::attrs(lhs.update(*rhs))) } OpCode::OpAttrsSelect => { -- cgit 1.4.1