From 1941082cbb4aa977bc5210516536efdbf96b927c Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Mon, 27 Feb 2023 13:32:03 +0300 Subject: refactor(tvix/eval): simplify NixString representation(s) Instead of the two different representations (which we don't really use much), use a `Box` (which potentially shaves another 8 bytes off `Value`). NixString values themselves are immutable anyways (which was a guarantee we already had with `SmolStr`), so this doesn't change anything else. Change-Id: I1d8454c056c21ecb0aebc473cfb3ae06cd70dbb6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8151 Reviewed-by: raitobezarius Reviewed-by: flokli Tested-by: BuildkiteCI --- tvix/eval/src/value/attrs.rs | 49 +++++++++++++++++--------------------- tvix/eval/src/value/attrs/tests.rs | 4 ++-- tvix/eval/src/value/string.rs | 49 ++++++++++---------------------------- 3 files changed, 37 insertions(+), 65 deletions(-) (limited to 'tvix') diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs index 64a1dc035b54..2b29c8396ff5 100644 --- a/tvix/eval/src/value/attrs.rs +++ b/tvix/eval/src/value/attrs.rs @@ -8,16 +8,23 @@ use std::iter::FromIterator; use imbl::{ordmap, OrdMap}; +use lazy_static::lazy_static; use serde::de::{Deserializer, Error, Visitor}; use serde::ser::SerializeMap; use serde::{Deserialize, Serialize}; -use crate::errors::ErrorKind; - use super::string::NixString; use super::thunk::ThunkSet; use super::TotalDisplay; use super::Value; +use crate::errors::ErrorKind; + +lazy_static! { + static ref NAME_S: NixString = "name".into(); + static ref NAME_REF: &'static NixString = &NAME_S; + static ref VALUE_S: NixString = "value".into(); + static ref VALUE_REF: &'static NixString = &VALUE_S; +} #[cfg(test)] mod tests; @@ -57,8 +64,8 @@ impl AttrsRep { AttrsRep::KV { name, value } => { *self = AttrsRep::Im(ordmap! { - NixString::NAME => name.clone(), - NixString::VALUE => value.clone() + NAME_S.clone() => name.clone(), + VALUE_S.clone() => value.clone() }); self.map_mut() @@ -230,13 +237,13 @@ impl NixAttrs { // Slightly more advanced, but still optimised updates match (self.0, other.0) { (AttrsRep::Im(mut m), AttrsRep::KV { name, value }) => { - m.insert(NixString::NAME, name); - m.insert(NixString::VALUE, value); + m.insert(NAME_S.clone(), name); + m.insert(VALUE_S.clone(), value); NixAttrs(AttrsRep::Im(m)) } (AttrsRep::KV { name, value }, AttrsRep::Im(mut m)) => { - match m.entry(NixString::NAME) { + match m.entry(NAME_S.clone()) { imbl::ordmap::Entry::Vacant(e) => { e.insert(name); } @@ -244,7 +251,7 @@ impl NixAttrs { imbl::ordmap::Entry::Occupied(_) => { /* name from `m` has precedence */ } }; - match m.entry(NixString::VALUE) { + match m.entry(VALUE_S.clone()) { imbl::ordmap::Entry::Vacant(e) => { e.insert(value); } @@ -318,11 +325,7 @@ impl NixAttrs { match self.0 { AttrsRep::Empty => IntoIter(IntoIterRepr::Empty), AttrsRep::KV { name, value } => IntoIter(IntoIterRepr::Finite( - vec![ - (NixString::NAME_REF.clone(), name), - (NixString::VALUE_REF.clone(), value), - ] - .into_iter(), + vec![(NAME_REF.clone(), name), (VALUE_REF.clone(), value)].into_iter(), )), AttrsRep::Im(map) => IntoIter(IntoIterRepr::Im(map.into_iter())), } @@ -411,17 +414,9 @@ impl NixAttrs { fn attempt_optimise_kv(slice: &mut [Value]) -> Option { let (name_idx, value_idx) = { match (&slice[2], &slice[0]) { - (Value::String(s1), Value::String(s2)) - if (*s1 == NixString::NAME && *s2 == NixString::VALUE) => - { - (3, 1) - } + (Value::String(s1), Value::String(s2)) if (*s1 == *NAME_S && *s2 == *VALUE_S) => (3, 1), - (Value::String(s1), Value::String(s2)) - if (*s1 == NixString::VALUE && *s2 == NixString::NAME) => - { - (1, 3) - } + (Value::String(s1), Value::String(s2)) if (*s1 == *VALUE_S && *s2 == *NAME_S) => (1, 3), // Technically this branch lets type errors pass, // but they will be caught during normal attribute @@ -502,12 +497,12 @@ impl<'a> Iterator for Iter> { KeyValue::KV { name, value, at } => match at { IterKV::Name => { at.next(); - Some((NixString::NAME_REF, name)) + Some((&NAME_REF, name)) } IterKV::Value => { at.next(); - Some((NixString::VALUE_REF, value)) + Some((&VALUE_REF, value)) } IterKV::Done => None, @@ -542,11 +537,11 @@ impl<'a> Iterator for Keys<'a> { KeysInner::Empty => None, KeysInner::KV(at @ IterKV::Name) => { at.next(); - Some(NixString::NAME_REF) + Some(&NAME_REF) } KeysInner::KV(at @ IterKV::Value) => { at.next(); - Some(NixString::VALUE_REF) + Some(&VALUE_REF) } KeysInner::KV(IterKV::Done) => None, KeysInner::Im(m) => m.next(), diff --git a/tvix/eval/src/value/attrs/tests.rs b/tvix/eval/src/value/attrs/tests.rs index 39ac55b6799a..473592c519df 100644 --- a/tvix/eval/src/value/attrs/tests.rs +++ b/tvix/eval/src/value/attrs/tests.rs @@ -84,10 +84,10 @@ fn test_kv_attrs_iter() { .into_iter() .map(|(k, v)| (k, v)); let (k, v) = iter.next().unwrap(); - assert!(k == NixString::NAME_REF); + assert!(k == *NAME_REF); assert!(v.to_str().unwrap() == meaning_val.to_str().unwrap()); let (k, v) = iter.next().unwrap(); - assert!(k == NixString::VALUE_REF); + assert!(k == *VALUE_REF); assert!(v.as_int().unwrap() == forty_two_val.as_int().unwrap()); assert!(iter.next().is_none()); } diff --git a/tvix/eval/src/value/string.rs b/tvix/eval/src/value/string.rs index 5962c94ea51f..8bb41a7825dd 100644 --- a/tvix/eval/src/value/string.rs +++ b/tvix/eval/src/value/string.rs @@ -1,5 +1,8 @@ -//! This module implements Nix language strings and their different -//! backing implementations. +//! This module implements Nix language strings. +//! +//! Nix language strings never need to be modified on the language +//! level, allowing us to shave off some memory overhead and only +//! paying the cost when creating new strings. use rnix::ast; use smol_str::SmolStr; use std::ffi::OsStr; @@ -11,16 +14,9 @@ use std::{borrow::Cow, fmt::Display, str::Chars}; use serde::de::{Deserializer, Visitor}; use serde::{Deserialize, Serialize}; -#[derive(Clone, Debug, Serialize)] -#[serde(untagged)] -enum StringRepr { - Smol(SmolStr), - Heap(String), -} - #[repr(transparent)] #[derive(Clone, Debug, Serialize)] -pub struct NixString(StringRepr); +pub struct NixString(Box); impl PartialEq for NixString { fn eq(&self, other: &Self) -> bool { @@ -44,19 +40,19 @@ impl Ord for NixString { impl From<&str> for NixString { fn from(s: &str) -> Self { - NixString(StringRepr::Smol(SmolStr::new(s))) + NixString(Box::from(s)) } } impl From for NixString { fn from(s: String) -> Self { - NixString(StringRepr::Heap(s)) + NixString(s.into_boxed_str()) } } impl From for NixString { fn from(s: SmolStr) -> Self { - NixString(StringRepr::Smol(s)) + NixString(Box::from(s.as_str())) } } @@ -109,7 +105,6 @@ impl<'de> Deserialize<'de> for NixString { mod arbitrary { use super::*; use proptest::prelude::{any_with, Arbitrary}; - use proptest::prop_oneof; use proptest::strategy::{BoxedStrategy, Strategy}; impl Arbitrary for NixString { @@ -118,29 +113,14 @@ mod arbitrary { type Strategy = BoxedStrategy; fn arbitrary_with(args: Self::Parameters) -> Self::Strategy { - prop_oneof![ - // Either generate `StringRepr::Heap`... - any_with::(args).prop_map(Self::from), - // ...or generate `StringRepr::Smol` (which `impl From<&str> for NixString` returns) - any_with::(args).prop_map(|s| Self::from(s.as_str())), - ] - .boxed() + any_with::(args).prop_map(Self::from).boxed() } } } impl NixString { - pub const NAME: Self = NixString(StringRepr::Smol(SmolStr::new_inline("name"))); - pub const NAME_REF: &'static Self = &Self::NAME; - - pub const VALUE: Self = NixString(StringRepr::Smol(SmolStr::new_inline("value"))); - pub const VALUE_REF: &'static Self = &Self::VALUE; - pub fn as_str(&self) -> &str { - match &self.0 { - StringRepr::Smol(s) => s.as_str(), - StringRepr::Heap(s) => s, - } + &self.0 } /// Return a displayable representation of the string as an @@ -172,14 +152,11 @@ impl NixString { pub fn concat(&self, other: &Self) -> Self { let mut s = self.as_str().to_owned(); s.push_str(other.as_str()); - NixString(StringRepr::Heap(s)) + NixString(s.into_boxed_str()) } pub fn chars(&self) -> Chars<'_> { - match &self.0 { - StringRepr::Heap(h) => h.chars(), - StringRepr::Smol(s) => s.chars(), - } + self.0.chars() } } -- cgit 1.4.1