From f86deb99519b7d9b3b522bf45340cef2a65331cd Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Mon, 19 Jun 2023 14:33:50 -0700 Subject: [PATCH 1/2] Use Arc instead of String for OptionMap value First in a series of commits to make this kind of change for values that are part of `State` and will benefit greatly from faster cloning and lower memory footprint. Cloning an Arc is an O(1) operation and the solver has to make many clones of State as it progresses. There is currently a lot of `.to_string()` used on these values but this is expected to change into Arc::clone as other things get similarly converted. Signed-off-by: J Robert Ray --- crates/spk-build/src/build/binary.rs | 8 +++- crates/spk-build/src/build/binary_test.rs | 5 +-- .../src/cmd_build_test/variant_filter.rs | 32 ++++++++-------- crates/spk-cli/common/src/flags.rs | 4 +- crates/spk-cli/common/src/flags/variant.rs | 7 ++-- crates/spk-cli/common/src/flags_test.rs | 2 +- .../spk-schema/crates/foundation/Cargo.toml | 2 +- .../crates/foundation/src/option_map/mod.rs | 26 ++++++------- .../src/option_map/option_map_test.rs | 20 ++-------- crates/spk-schema/src/build_spec.rs | 4 +- crates/spk-schema/src/option.rs | 37 +++++++++++++++---- crates/spk-schema/src/option_test.rs | 2 +- crates/spk-schema/src/package_test.rs | 6 +-- crates/spk-schema/src/requirements_list.rs | 2 +- crates/spk-schema/src/spec_test.rs | 6 +-- crates/spk-schema/src/v0/spec.rs | 33 +++++++++-------- crates/spk-schema/src/v0/spec_test.rs | 6 +-- crates/spk-schema/src/v0/variant.rs | 4 +- crates/spk-solve/crates/graph/src/graph.rs | 31 +++++++++------- .../spk-solve/crates/graph/src/graph_test.rs | 4 +- .../crates/package-iterator/src/build_key.rs | 2 +- .../package-iterator/src/build_key_test.rs | 14 +++---- .../package-iterator/src/package_iterator.rs | 4 +- .../src/package_iterator_test.rs | 2 +- .../spk-solve/crates/solution/src/solution.rs | 14 +++++-- .../crates/validation/src/validation_test.rs | 4 +- .../src/validators/var_requirements.rs | 2 +- crates/spk-solve/src/solver.rs | 8 +--- crates/spk-solve/src/solver_test.rs | 14 ++----- 29 files changed, 160 insertions(+), 145 deletions(-) diff --git a/crates/spk-build/src/build/binary.rs b/crates/spk-build/src/build/binary.rs index c3d2c80a3d..d56c85563d 100644 --- a/crates/spk-build/src/build/binary.rs +++ b/crates/spk-build/src/build/binary.rs @@ -703,7 +703,13 @@ where let mut cmd = cmd.into_std(); cmd.envs(self.environment.drain()); - cmd.envs(options.as_ref().to_environment()); + cmd.envs( + options + .as_ref() + .to_environment() + .iter() + .map(|(k, v)| (&**k, &**v)), + ); cmd.envs(get_package_build_env(package)); cmd.env("PREFIX", &self.prefix); // force the base environment to be setup using bash, so that the diff --git a/crates/spk-build/src/build/binary_test.rs b/crates/spk-build/src/build/binary_test.rs index 89806a56ed..6103f42a69 100644 --- a/crates/spk-build/src/build/binary_test.rs +++ b/crates/spk-build/src/build/binary_test.rs @@ -188,10 +188,7 @@ async fn test_build_package_options() { .await .unwrap() .option_values(); - assert_eq!( - build_options.get(opt_name!("dep")), - Some(&String::from("~1.0.0")) - ); + assert_eq!(build_options.get(opt_name!("dep")), Some(&"~1.0.0".into())); } #[rstest] diff --git a/crates/spk-cli/cmd-build/src/cmd_build_test/variant_filter.rs b/crates/spk-cli/cmd-build/src/cmd_build_test/variant_filter.rs index 0e766a95c3..0666c3ff06 100644 --- a/crates/spk-cli/cmd-build/src/cmd_build_test/variant_filter.rs +++ b/crates/spk-cli/cmd-build/src/cmd_build_test/variant_filter.rs @@ -69,7 +69,7 @@ build: assert!( matches!( &result.created_builds.artifacts[0].1, - BuildArtifact::Binary(_, VariantLocation::Index(index), options) if *index == 1 && matches!(options.get(opt_name_color), Some(color) if color == "green") + BuildArtifact::Binary(_, VariantLocation::Index(index), options) if *index == 1 && matches!(options.get(opt_name_color), Some(color) if &**color == "green") ), "Expected the second variant to be built, and color=green" ); @@ -121,7 +121,7 @@ build: assert!( matches!( &result.created_builds.artifacts[0].1, - BuildArtifact::Binary(_, VariantLocation::Index(index), options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if color == "green") + BuildArtifact::Binary(_, VariantLocation::Index(index), options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if &**color == "green") ), "Expected the first variant to be built, and color=green" ); @@ -201,7 +201,7 @@ build: assert!( matches!( &result.created_builds.artifacts[0].1, - BuildArtifact::Binary(_, VariantLocation::Index(index), options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if color == "blue") + BuildArtifact::Binary(_, VariantLocation::Index(index), options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if &**color == "blue") ), "Expected the first variant to be built, and color=blue" ); @@ -275,7 +275,7 @@ build: assert!( matches!( &result.created_builds.artifacts[0].1, - BuildArtifact::Binary(_, VariantLocation::Bespoke(index), options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if color == "green") + BuildArtifact::Binary(_, VariantLocation::Bespoke(index), options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if &**color == "green") ), "Expected the first extra-variant to be built, and color=green" ); @@ -331,8 +331,8 @@ build: &result.created_builds.artifacts[0].1, BuildArtifact::Binary(_, VariantLocation::Index(index), options) if *index == 1 && - matches!(options.get(opt_name_color), Some(color) if color == "green") - && matches!(options.get(opt_name_fruit), Some(fruit) if fruit == "apple") + matches!(options.get(opt_name_color), Some(color) if &**color == "green") + && matches!(options.get(opt_name_fruit), Some(fruit) if &**fruit == "apple") ), "Expected the second variant to be built, with color=green and fruit=apple" ); @@ -424,8 +424,8 @@ build: &result.created_builds.artifacts[0].1, BuildArtifact::Binary(_, VariantLocation::Index(index), options) if *index == 1 && - matches!(options.get(opt_name_color), Some(color) if color == "green") - && matches!(options.get(opt_name_fruit), Some(fruit) if fruit == "apple") + matches!(options.get(opt_name_color), Some(color) if &**color == "green") + && matches!(options.get(opt_name_fruit), Some(fruit) if &**fruit == "apple") ), "Expected the second variant to be built, with color=green and fruit=apple" ); @@ -479,7 +479,7 @@ build: result.created_builds.artifacts.iter().all(|(_, artifact)| { matches!( artifact, - BuildArtifact::Binary(_, _, options) if matches!(options.get(opt_name_fruit), Some(fruit) if fruit == "apple") + BuildArtifact::Binary(_, _, options) if matches!(options.get(opt_name_fruit), Some(fruit) if &**fruit == "apple") ) }), "Expected all variants to be built with fruit=apple" @@ -539,8 +539,8 @@ build: &result.created_builds.artifacts[0].1, BuildArtifact::Binary(_, VariantLocation::Index(index), options) if *index == 0 && - matches!(options.get(opt_name_color), Some(color) if color == "green") - && matches!(options.get(opt_name_fruit), Some(fruit) if fruit == "banana") + matches!(options.get(opt_name_color), Some(color) if &**color == "green") + && matches!(options.get(opt_name_fruit), Some(fruit) if &**fruit == "banana") ), "Expected the first variant to be built, with color=green and fruit=banana" ); @@ -596,8 +596,8 @@ build: BuildArtifact::Binary( _, VariantLocation::Bespoke(index), - options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if color == "brown") - && matches!(options.get(opt_name_fruit), Some(fruit) if fruit == "kiwi") + options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if &**color == "brown") + && matches!(options.get(opt_name_fruit), Some(fruit) if &**fruit == "kiwi") ), "Expected the first extra-variant to be built, with color=brown and fruit=kiwi" ); @@ -656,8 +656,8 @@ build: BuildArtifact::Binary( _, VariantLocation::Bespoke(index), - options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if color == "green") - && matches!(options.get(opt_name_fruit), Some(fruit) if fruit == "kiwi") + options) if *index == 0 && matches!(options.get(opt_name_color), Some(color) if &**color == "green") + && matches!(options.get(opt_name_fruit), Some(fruit) if &**fruit == "kiwi") ), "Expected the first extra-variant to be built, with color=green and fruit=kiwi" ); @@ -710,7 +710,7 @@ async fn test_build_filters_variants_based_on_host_opts(tmpdir: tempfile::TempDi result.created_builds.artifacts.iter().all(|(_, artifact)| { matches!( artifact, - BuildArtifact::Binary(_, _, options) if matches!(options.get(opt_name_distro), Some(distro) if distro == "centos") + BuildArtifact::Binary(_, _, options) if matches!(options.get(opt_name_distro), Some(distro) if &**distro == "centos") ) }), "Expected all variants to be built with distro=centos" diff --git a/crates/spk-cli/common/src/flags.rs b/crates/spk-cli/common/src/flags.rs index b4e92929d1..e16c188641 100644 --- a/crates/spk-cli/common/src/flags.rs +++ b/crates/spk-cli/common/src/flags.rs @@ -320,7 +320,7 @@ impl Options { }) .and_then(|(name, value)| Ok((OptName::new(name)?, value)))?; - opts.insert(name.to_owned(), value.to_string()); + opts.insert(name.to_owned(), value.into()); } Ok(opts) @@ -331,7 +331,7 @@ impl Options { .get_options()? .into_iter() .filter(|(_name, value)| !value.is_empty()) - .map(|(name, value)| VarRequest::new_with_value(name, value)) + .map(|(name, value)| VarRequest::new_with_value(name, &*value)) .collect()) } } diff --git a/crates/spk-cli/common/src/flags/variant.rs b/crates/spk-cli/common/src/flags/variant.rs index f274d68f92..9d61e7c729 100644 --- a/crates/spk-cli/common/src/flags/variant.rs +++ b/crates/spk-cli/common/src/flags/variant.rs @@ -5,6 +5,7 @@ use std::borrow::Cow; use std::collections::{HashMap, HashSet}; use std::str::FromStr; +use std::sync::Arc; use clap::Args; use miette::{miette, Result}; @@ -49,7 +50,7 @@ impl FromStr for VariantSpec { .and_then(|(name, value)| { Ok((OptNameBuf::try_from(name)?, value.to_string())) })?; - Ok((name, value)) + Ok((name, value.into())) }) .collect::>() .map(VariantSpec::Filter) @@ -77,8 +78,8 @@ impl std::fmt::Display for VariantLocation { /// A mismatch between the expected and actual values of a variant option. pub struct VariantOptionMismatch { - pub expected: String, - pub actual: Option, + pub expected: Arc, + pub actual: Option>, } /// Details on if a variant is selected for build or why it is not. diff --git a/crates/spk-cli/common/src/flags_test.rs b/crates/spk-cli/common/src/flags_test.rs index 3a6a7cad26..ac873c54d2 100644 --- a/crates/spk-cli/common/src/flags_test.rs +++ b/crates/spk-cli/common/src/flags_test.rs @@ -30,7 +30,7 @@ fn test_option_flags_parsing(#[case] args: &[&str], #[case] expected: &[(&str, & let actual = options.get_options().unwrap(); let expected: OptionMap = expected .iter() - .map(|(k, v)| (OptName::new(k).unwrap().to_owned(), v.to_string())) + .map(|(k, v)| (OptName::new(k).unwrap().to_owned(), (*v).into())) .collect(); assert_eq!(actual, expected); } diff --git a/crates/spk-schema/crates/foundation/Cargo.toml b/crates/spk-schema/crates/foundation/Cargo.toml index ece50b2e99..ab8e7aeca0 100644 --- a/crates/spk-schema/crates/foundation/Cargo.toml +++ b/crates/spk-schema/crates/foundation/Cargo.toml @@ -33,7 +33,7 @@ rand = { workspace = true } relative-path = "1.3" ring = { workspace = true } rstest = { workspace = true } -serde = { workspace = true, features = ["derive"] } +serde = { workspace = true, features = ["derive", "rc"] } serde_yaml = { workspace = true } spfs = { workspace = true } sys-info = "0.9.0" diff --git a/crates/spk-schema/crates/foundation/src/option_map/mod.rs b/crates/spk-schema/crates/foundation/src/option_map/mod.rs index a90673477f..968a052992 100644 --- a/crates/spk-schema/crates/foundation/src/option_map/mod.rs +++ b/crates/spk-schema/crates/foundation/src/option_map/mod.rs @@ -79,11 +79,11 @@ impl HostOptions { }; if let Some(id) = info.id { - opts.insert(OptName::distro().to_owned(), id.clone()); + opts.insert(OptName::distro().to_owned(), id.clone().into()); match OptNameBuf::try_from(id) { Ok(id) => { if let Some(version_id) = info.version_id { - opts.insert(id, version_id); + opts.insert(id, version_id.into()); } } Err(err) => { @@ -126,11 +126,11 @@ pub static HOST_OPTIONS: HostOptions = HostOptions(Lazy::new(|| { #[derive(Default, Clone, Hash, PartialEq, Eq, Serialize, Ord, PartialOrd)] #[serde(transparent)] pub struct OptionMap { - options: BTreeMap, + options: BTreeMap>, } impl std::ops::Deref for OptionMap { - type Target = BTreeMap; + type Target = BTreeMap>; fn deref(&self) -> &Self::Target { &self.options @@ -143,16 +143,16 @@ impl std::ops::DerefMut for OptionMap { } } -impl From<&Arc>> for OptionMap { - fn from(hm: &Arc>) -> Self { +impl From<&Arc>>> for OptionMap { + fn from(hm: &Arc>>) -> Self { Self { options: (**hm).clone(), } } } -impl FromIterator<(OptNameBuf, String)> for OptionMap { - fn from_iter>(iter: T) -> Self { +impl FromIterator<(OptNameBuf, Arc)> for OptionMap { + fn from_iter)>>(iter: T) -> Self { Self { options: BTreeMap::from_iter(iter), } @@ -173,8 +173,8 @@ impl std::fmt::Display for OptionMap { } impl IntoIterator for OptionMap { - type IntoIter = std::collections::btree_map::IntoIter; - type Item = (OptNameBuf, String); + type IntoIter = std::collections::btree_map::IntoIter>; + type Item = (OptNameBuf, Arc); fn into_iter(self) -> Self::IntoIter { self.options.into_iter() @@ -183,11 +183,11 @@ impl IntoIterator for OptionMap { impl OptionMap { /// Return the data of these options as environment variables. - pub fn to_environment(&self) -> HashMap { + pub fn to_environment(&self) -> HashMap, Arc> { let mut out = HashMap::default(); for (name, value) in self.iter() { let var_name = format!("SPK_OPT_{name}"); - out.insert(var_name, value.into()); + out.insert(var_name.into(), Arc::clone(value)); } out } @@ -284,7 +284,7 @@ impl<'de> Deserialize<'de> for OptionMap { { let mut options = OptionMap::default(); while let Some((name, value)) = map.next_entry::()? { - options.insert(name, value.0); + options.insert(name, value.0.into()); } Ok(options) } diff --git a/crates/spk-schema/crates/foundation/src/option_map/option_map_test.rs b/crates/spk-schema/crates/foundation/src/option_map/option_map_test.rs index f670bc925b..527d3dcfe9 100644 --- a/crates/spk-schema/crates/foundation/src/option_map/option_map_test.rs +++ b/crates/spk-schema/crates/foundation/src/option_map/option_map_test.rs @@ -29,22 +29,10 @@ fn test_package_options() { fn test_option_map_deserialize_scalar() { let opts: OptionMap = serde_yaml::from_str("{one: one, two: 2, three: false, four: 4.4}").unwrap(); - assert_eq!( - opts.options.get(opt_name!("one")).map(String::to_owned), - Some("one".to_string()) - ); - assert_eq!( - opts.options.get(opt_name!("two")).map(String::to_owned), - Some("2".to_string()) - ); - assert_eq!( - opts.options.get(opt_name!("three")).map(String::to_owned), - Some("false".to_string()) - ); - assert_eq!( - opts.options.get(opt_name!("four")).map(String::to_owned), - Some("4.4".to_string()) - ); + assert_eq!(opts.options.get(opt_name!("one")), Some(&"one".into())); + assert_eq!(opts.options.get(opt_name!("two")), Some(&"2".into())); + assert_eq!(opts.options.get(opt_name!("three")), Some(&"false".into())); + assert_eq!(opts.options.get(opt_name!("four")), Some(&"4.4".into())); } #[rstest] diff --git a/crates/spk-schema/src/build_spec.rs b/crates/spk-schema/src/build_spec.rs index 7a4806f824..0f22659586 100644 --- a/crates/spk-schema/src/build_spec.rs +++ b/crates/spk-schema/src/build_spec.rs @@ -249,12 +249,12 @@ impl BuildSpec { .get(&opt.full_name().with_namespace(pkg_name)) .or_else(|| given.get(opt.full_name())), }; - let value = opt.get_value(given_value.map(String::as_ref)); + let value = opt.get_value(given_value); let compat = opt.validate(Some(&value)); if !compat.is_ok() { return Err(Error::String(compat.to_string())); } - resolved.insert(opt.full_name().to_owned(), value); + resolved.insert(opt.full_name().to_owned(), value.into()); } Ok((resolved, opts)) diff --git a/crates/spk-schema/src/option.rs b/crates/spk-schema/src/option.rs index 2cb8ee8bad..6d94e4e490 100644 --- a/crates/spk-schema/src/option.rs +++ b/crates/spk-schema/src/option.rs @@ -98,7 +98,10 @@ impl Opt { } } - pub fn validate(&self, value: Option<&str>) -> Compatibility { + pub fn validate(&self, value: Option) -> Compatibility + where + S: AsRef, + { match self { Self::Pkg(opt) => opt.validate(value), Self::Var(opt) => opt.validate(value), @@ -118,7 +121,10 @@ impl Opt { /// Return the current value of this option, if set. /// /// Given is only returned if the option is not currently set to something else. - pub fn get_value(&self, given: Option<&str>) -> String { + pub fn get_value(&self, given: Option) -> String + where + S: Copy + ToString, + { let value = match self { Self::Pkg(opt) => opt.get_value(given), Self::Var(opt) => opt.get_value(given), @@ -388,7 +394,10 @@ impl VarOpt { }) } - pub fn get_value(&self, given: Option<&str>) -> Option { + pub fn get_value(&self, given: Option) -> Option + where + S: ToString, + { if let Some(v) = &self.value { if !v.is_empty() { return Some(v.clone()); @@ -414,12 +423,15 @@ impl VarOpt { Ok(()) } - pub fn validate(&self, value: Option<&str>) -> Compatibility { + pub fn validate(&self, value: Option) -> Compatibility + where + S: AsRef, + { if value.is_none() && self.value.is_some() { return self.validate(self.value.as_deref()); } let assigned = self.value.as_deref(); - match (value, assigned) { + match (value.as_ref().map(AsRef::::as_ref), assigned) { (None, Some(_)) => Compatibility::Compatible, (Some(value), Some(assigned)) => { if value == assigned { @@ -509,7 +521,10 @@ impl PkgOpt { }) } - pub fn get_value(&self, given: Option<&str>) -> Option { + pub fn get_value(&self, given: Option) -> Option + where + S: ToString, + { if let Some(v) = &self.value { Some(v.clone()) } else if let Some(v) = given { @@ -546,8 +561,14 @@ impl PkgOpt { Ok(()) } - pub fn validate(&self, value: Option<&str>) -> Compatibility { - let value = value.unwrap_or_default(); + pub fn validate(&self, value: Option) -> Compatibility + where + S: AsRef, + { + let value = value + .as_ref() + .map(AsRef::::as_ref) + .unwrap_or_else(|| ""); // skip any default that might exist since // that does not represent a definitive range diff --git a/crates/spk-schema/src/option_test.rs b/crates/spk-schema/src/option_test.rs index c76b693f06..52d44ed776 100644 --- a/crates/spk-schema/src/option_test.rs +++ b/crates/spk-schema/src/option_test.rs @@ -45,7 +45,7 @@ fn test_var_opt_validation(#[case] spec: &str, #[case] value: &str, #[case] expe #[case("{static: static, var: my-var/default}", Some("static"))] // static supersedes default fn test_var_opt_parse_value(#[case] spec: &str, #[case] expected: Option<&str>) { let opt = Opt::from_yaml(spec).unwrap().into_var().unwrap(); - let actual = opt.get_value(None); + let actual = opt.get_value::<&str>(None); assert_eq!(actual.as_deref(), expected); } diff --git a/crates/spk-schema/src/package_test.rs b/crates/spk-schema/src/package_test.rs index 365c91d2d3..f6efef569a 100644 --- a/crates/spk-schema/src/package_test.rs +++ b/crates/spk-schema/src/package_test.rs @@ -34,17 +34,17 @@ fn test_resolve_options_package_option() { let resolved = recipe.resolve_options(&options).unwrap(); assert_eq!( resolved.get(opt_name!("my-opt")), - Some(&"override".to_string()), + Some(&"override".into()), "namespaced option should take precedence" ); assert_eq!( resolved.get(opt_name!("debug")), - Some(&"on".to_string()), + Some(&"on".into()), "global opt should resolve if given" ); assert_eq!( resolved.get(opt_name!("python.abi")), - Some(&"cp27mu".to_string()), + Some(&"cp27mu".into()), "opt for other package should exist" ); } diff --git a/crates/spk-schema/src/requirements_list.rs b/crates/spk-schema/src/requirements_list.rs index 7e18f8d0dc..14c274a4a2 100644 --- a/crates/spk-schema/src/requirements_list.rs +++ b/crates/spk-schema/src/requirements_list.rs @@ -178,7 +178,7 @@ impl RequirementsList { ))) } Some(opt) => { - Some(var_request.render_pin(opt.as_str()).map_err(Into::into).map(Request::Var)) + Some(var_request.render_pin(&**opt).map_err(Into::into).map(Request::Var)) } } } diff --git a/crates/spk-schema/src/spec_test.rs b/crates/spk-schema/src/spec_test.rs index fe99f75f01..458f839059 100644 --- a/crates/spk-schema/src/spec_test.rs +++ b/crates/spk-schema/src/spec_test.rs @@ -60,7 +60,7 @@ fn test_resolve_options_variant_adds_new_var_option( assert_eq!(resolved_options.len(), 1); let (k, v) = resolved_options.into_iter().next().unwrap(); assert_eq!(k.as_str(), opt_name); - assert_eq!(v, default_value); + assert_eq!(&*v, default_value); // Now do the same thing but add-in an override for the option. @@ -79,14 +79,14 @@ fn test_resolve_options_variant_adds_new_var_option( // The override should have won. let (k, v) = resolved_options.into_iter().next().unwrap(); assert_eq!(k.as_str(), opt_name); - assert_eq!(v, override_value); + assert_eq!(&*v, override_value); } macro_rules! assert_option_map_contains { ( $option_map:expr, $expected_key:expr, $expected_value:expr ) => {{ match $option_map.get($crate::opt_name!($expected_key)) { None => panic!("option map did not contain expected key {}", $expected_key), - Some(v) => assert_eq!(v, $expected_value), + Some(v) => assert_eq!(v.as_ref(), $expected_value), } }}; } diff --git a/crates/spk-schema/src/v0/spec.rs b/crates/spk-schema/src/v0/spec.rs index 810448db28..2db5d77849 100644 --- a/crates/spk-schema/src/v0/spec.rs +++ b/crates/spk-schema/src/v0/spec.rs @@ -168,7 +168,7 @@ impl Spec { var, // we are assuming that the var here will have a value because // this is a built binary package - value: o.get_value(None).unwrap_or_default().into(), + value: o.get_value::<&str>(None).unwrap_or_default().into(), description: o.description.clone(), } }) @@ -227,7 +227,10 @@ impl Package for Spec { for opt in self.build.options.iter() { // we are assuming that this spec has been updated to represent // a build and had all of the options pinned/resolved. - opts.insert(opt.full_name().to_owned(), opt.get_value(None)); + opts.insert( + opt.full_name().to_owned(), + opt.get_value::<&str>(None).into(), + ); } opts } @@ -285,7 +288,7 @@ impl Package for Spec { // If no value was specified in the spec, there's // no need to turn that into a requirement to // find a var with an empty value. - if let Some(value) = opt.get_value(None) { + if let Some(value) = opt.get_value::<&str>(None) { if !value.is_empty() { requests .insert_or_merge(opt.to_request(Some(value.as_str())).into())?; @@ -327,9 +330,7 @@ impl Package for Spec { let mut must_exist = given_options.package_options_without_global(self.name()); let given_options = given_options.package_options(self.name()); for option in self.build.options.iter() { - let value = given_options - .get(option.full_name().without_namespace()) - .map(String::as_str); + let value = given_options.get(option.full_name().without_namespace()); let compat = option.validate(value); if !compat.is_ok() { return Compatibility::incompatible(format!( @@ -406,7 +407,7 @@ impl Recipe for Spec { for opt in opts { match opt { Opt::Pkg(opt) => { - let given_value = options.get(opt.pkg.as_opt_name()).map(String::to_owned); + let given_value = options.get(opt.pkg.as_opt_name()).map(ToString::to_string); let mut req = opt.to_request( given_value, RequestedBy::BinaryBuild(self.ident().to_build(build_digest.clone())), @@ -454,8 +455,9 @@ impl Recipe for Spec { match key { VariantSpecEntryKey::PkgOrOpt(pkg) => { // First the version asked for must match. - if options.get(pkg.0.name.as_opt_name()) != Some(value) { - return false; + match options.get(pkg.0.name.as_opt_name()) { + Some(v) if &**v == value.as_str() => {} + _ => return false, } // Then the components asked for must be a // subset of what is present. @@ -483,11 +485,10 @@ impl Recipe for Spec { return false; } } - VariantSpecEntryKey::Opt(opt) => { - if options.get(opt) != Some(value) { - return false; - } - } + VariantSpecEntryKey::Opt(opt) => match options.get(opt) { + Some(v) if &**v == value.as_str() => {} + _ => return false, + }, } } } @@ -533,8 +534,8 @@ impl Recipe for Spec { build_options .get(&opt.var) .or_else(|| build_options.get(opt.var.without_namespace())) - .map(String::to_owned) - .or_else(|| opt.get_value(None)) + .map(ToString::to_string) + .or_else(|| opt.get_value::<&str>(None)) .unwrap_or_default(), )?; continue; diff --git a/crates/spk-schema/src/v0/spec_test.rs b/crates/spk-schema/src/v0/spec_test.rs index 54e3e87c3d..98e8249526 100644 --- a/crates/spk-schema/src/v0/spec_test.rs +++ b/crates/spk-schema/src/v0/spec_test.rs @@ -207,7 +207,7 @@ fn test_strong_inheritance_injection() { built_package.build.options.iter().any(|opt| match opt { Opt::Pkg(_) => false, Opt::Var(var) => - var.var == "base.inherit-me" && var.get_value(None) == Some("1.2.3".into()), + var.var == "base.inherit-me" && var.get_value::<&str>(None) == Some("1.2.3".into()), }), "didn't find inherited build option" ); @@ -275,7 +275,7 @@ fn test_strong_inheritance_injection_transitivity() { built_package.build.options.iter().any(|opt| match opt { Opt::Pkg(_) => false, Opt::Var(var) => - var.var == "base.inherit-me" && var.get_value(None) == Some("1.2.3".into()), + var.var == "base.inherit-me" && var.get_value::<&str>(None) == Some("1.2.3".into()), }), "didn't find inherited build option" ); @@ -315,7 +315,7 @@ fn test_variants_can_introduce_components() { let mut found_pkg = false; for (opt_name, value) in variant.options().iter() { - if opt_name == "dep-pkg" && value == "1.2.3" { + if opt_name == "dep-pkg" && &**value == "1.2.3" { found_opt = true; break; } diff --git a/crates/spk-schema/src/v0/variant.rs b/crates/spk-schema/src/v0/variant.rs index acc550f873..87635de4b3 100644 --- a/crates/spk-schema/src/v0/variant.rs +++ b/crates/spk-schema/src/v0/variant.rs @@ -47,7 +47,7 @@ impl Variant { options .get(&o.var) .cloned() - .unwrap_or_else(|| o.default.clone()), + .unwrap_or_else(|| o.default.clone().into()), ) }) }) @@ -72,7 +72,7 @@ impl Variant { VariantSpecEntryKey::Opt(opt) => opt.to_owned(), }; let value = value.as_str(); - options.insert(name.clone(), value.to_owned()); + options.insert(name.clone(), value.into()); // if it was parsed as something with components, then it is a pkg // request. diff --git a/crates/spk-solve/crates/graph/src/graph.rs b/crates/spk-solve/crates/graph/src/graph.rs index bb691c9c69..178ecc2cb1 100644 --- a/crates/spk-solve/crates/graph/src/graph.rs +++ b/crates/spk-solve/crates/graph/src/graph.rs @@ -462,7 +462,7 @@ impl<'state, 'cmpt> DecisionBuilder<'state, 'cmpt> { let mut opts = OptionMap::default(); opts.insert( spec.name().as_opt_name().to_owned(), - spec.compat().render(spec.version()), + spec.compat().render(spec.version()).into(), ); for (name, value) in spec.option_values() { if !value.is_empty() { @@ -945,24 +945,27 @@ impl SetOptions { /// Compute the new options list for a state, preserving the insertion /// order based on option key. - pub fn compute_new_options<'i, I>( + pub fn compute_new_options<'i, I, V>( base: &State, new_options: I, update_existing_option_with_empty_value: bool, - ) -> BTreeMap + ) -> BTreeMap> where - I: Iterator, + I: Iterator, + V: AsRef + Into> + Clone + 'i, { let mut options = (*base.options).clone(); // Update base options with request options... for (k, v) in new_options { match options.get_mut(k) { // Unless already present and request option value is empty. - Some(_) if v.is_empty() && !update_existing_option_with_empty_value => continue, + Some(_) if v.as_ref().is_empty() && !update_existing_option_with_empty_value => { + continue + } // If option already existed, change the value - Some(value) => *value = v.to_owned(), + Some(value) => *value = (*v).clone().into(), None => { - options.insert(k.to_owned(), v.to_owned()); + options.insert(k.to_owned(), (*v).clone().into()); } }; } @@ -1048,7 +1051,7 @@ impl StateId { } } - fn options_hash(options: &BTreeMap) -> u64 { + fn options_hash(options: &BTreeMap>) -> u64 { let mut hasher = DefaultHasher::new(); options.hash(&mut hasher); hasher.finish() @@ -1084,7 +1087,7 @@ impl StateId { (global_hasher.finish(), var_requests_membership) } - fn with_options(&self, options: &BTreeMap) -> Self { + fn with_options(&self, options: &BTreeMap>) -> Self { Self::new( self.pkg_requests_hash, self.var_requests_hash, @@ -1117,7 +1120,7 @@ impl StateId { fn with_var_requests_and_options( &self, var_requests: &BTreeSet, - options: &BTreeMap, + options: &BTreeMap>, ) -> Self { let (var_requests_hash, var_requests_membership) = StateId::var_requests_hash(var_requests); Self::new( @@ -1188,7 +1191,7 @@ pub struct State { // efficient for processing. This field does not contribute to the // state id. It is used to track the resolve order for a solution. packages_in_solve_order: Arc>>, - options: Arc>, + options: Arc>>, state_id: StateId, cached_option_map: Arc>, // How deep is this state? @@ -1201,7 +1204,7 @@ impl State { pkg_requests: Vec, var_requests: Vec, packages: Vec<(Arc, PackageSource)>, - options: Vec<(OptNameBuf, String)>, + options: Vec<(OptNameBuf, Arc)>, ) -> Arc { // TODO: This pre-calculates the hash but there // may be states constructed where the id is @@ -1397,7 +1400,7 @@ impl State { self.state_id.packages_hash } - fn with_options(&self, parent: &Self, options: BTreeMap) -> Self { + fn with_options(&self, parent: &Self, options: BTreeMap>) -> Self { let state_id = self.state_id.with_options(&options); Self { pkg_requests: Arc::clone(&self.pkg_requests), @@ -1468,7 +1471,7 @@ impl State { &self, parent: &Self, var_requests: Arc>, - options: BTreeMap, + options: BTreeMap>, ) -> Self { let state_id = self .state_id diff --git a/crates/spk-solve/crates/graph/src/graph_test.rs b/crates/spk-solve/crates/graph/src/graph_test.rs index f31d478f87..948610916f 100644 --- a/crates/spk-solve/crates/graph/src/graph_test.rs +++ b/crates/spk-solve/crates/graph/src/graph_test.rs @@ -75,7 +75,7 @@ fn test_empty_options_do_not_unset() { let opts = new_state.get_option_map(); assert_eq!( opts.get(opt_name!("something")), - Some(String::new()).as_ref(), + Some(&"".into()), "should assign empty option of no current value" ); @@ -85,7 +85,7 @@ fn test_empty_options_do_not_unset() { let opts = new_state.get_option_map(); assert_eq!( opts.get(opt_name!("something")), - Some(String::from("value")).as_ref(), + Some(&"value".into()), "should not unset value when one exists" ); } diff --git a/crates/spk-solve/crates/package-iterator/src/build_key.rs b/crates/spk-solve/crates/package-iterator/src/build_key.rs index 3355909909..430457c2a5 100644 --- a/crates/spk-solve/crates/package-iterator/src/build_key.rs +++ b/crates/spk-solve/crates/package-iterator/src/build_key.rs @@ -179,7 +179,7 @@ impl BuildKey { // options, could use that information here to // determine the kind of value instead of // relying on parsing errors. - BuildKeyEntry::Text(value.clone()) + BuildKeyEntry::Text(value.to_string()) } } } diff --git a/crates/spk-solve/crates/package-iterator/src/build_key_test.rs b/crates/spk-solve/crates/package-iterator/src/build_key_test.rs index 2be66d052d..ea5a729ffd 100644 --- a/crates/spk-solve/crates/package-iterator/src/build_key_test.rs +++ b/crates/spk-solve/crates/package-iterator/src/build_key_test.rs @@ -248,11 +248,11 @@ fn test_generating_build_key() { let value5 = "4.1.0/DIGEST".to_string(); let mut resolved_options: OptionMap = OptionMap::default(); - resolved_options.insert(name1.clone(), value1); + resolved_options.insert(name1.clone(), value1.into()); // value3 is left out deliberately to exercise unset value processing - resolved_options.insert(name2.clone(), value2.clone()); - resolved_options.insert(name4.clone(), value4); - resolved_options.insert(name5.clone(), value5); + resolved_options.insert(name2.clone(), value2.clone().into()); + resolved_options.insert(name4.clone(), value4.into()); + resolved_options.insert(name5.clone(), value5.into()); // Generate the build's key based on the ordering of option names let ordering = vec![name1, name2, name3, name4, name5]; @@ -336,10 +336,10 @@ fn test_generating_build_key_src_build() { let value4 = "1.0.0,<1.5".to_string(); let mut resolved_options: OptionMap = OptionMap::default(); - resolved_options.insert(name1.clone(), value1); + resolved_options.insert(name1.clone(), value1.into()); // value3 is left out deliberately to exercise unset value processing - resolved_options.insert(name2.clone(), value2); - resolved_options.insert(name4.clone(), value4); + resolved_options.insert(name2.clone(), value2.into()); + resolved_options.insert(name4.clone(), value4.into()); // Generate the build's key based on the ordering of option // names. Note: because this is a source build it won't use any of diff --git a/crates/spk-solve/crates/package-iterator/src/package_iterator.rs b/crates/spk-solve/crates/package-iterator/src/package_iterator.rs index 7637b3c841..30d0534bf2 100644 --- a/crates/spk-solve/crates/package-iterator/src/package_iterator.rs +++ b/crates/spk-solve/crates/package-iterator/src/package_iterator.rs @@ -561,7 +561,7 @@ impl SortedBuildIterator { // before. The count is used later to check if the // name is used by all, or only some, builds. let counter = changes.entry(name.clone()).or_insert(ChangeCounter { - last: value.clone(), + last: value.to_string(), count: 0, use_it: false, }); @@ -570,7 +570,7 @@ impl SortedBuildIterator { // Is this name marked as don't use yet, and is this // value different from the last one seen for this // name? - if !counter.use_it && counter.last != *value { + if !counter.use_it && counter.last.as_str() != &**value { // The values differ, mark this name as one to use counter.use_it = true; } diff --git a/crates/spk-solve/crates/package-iterator/src/package_iterator_test.rs b/crates/spk-solve/crates/package-iterator/src/package_iterator_test.rs index e30da57c46..3e9158f0eb 100644 --- a/crates/spk-solve/crates/package-iterator/src/package_iterator_test.rs +++ b/crates/spk-solve/crates/package-iterator/src/package_iterator_test.rs @@ -215,7 +215,7 @@ async fn test_solver_sorted_build_iterator_sort_by_option_values() { }; // Is the value what it should be for this option of this build in the order - assert_eq!(v, expected_v); + assert_eq!(&**v, expected_v); } } } diff --git a/crates/spk-solve/crates/solution/src/solution.rs b/crates/spk-solve/crates/solution/src/solution.rs index 37a165835d..199471249b 100644 --- a/crates/spk-solve/crates/solution/src/solution.rs +++ b/crates/spk-solve/crates/solution/src/solution.rs @@ -354,12 +354,15 @@ impl Solution { /// Return the data of this solution as environment variables. /// /// If base is given, also clean any existing, conflicting values. - pub fn to_environment(&self, base: Option) -> HashMap + pub fn to_environment(&self, base: Option) -> HashMap where - V: IntoIterator, + I: IntoIterator, + K: ToString, + V: ToString, { let mut out = base .map(IntoIterator::into_iter) + .map(|i| i.into_iter().map(|(k, v)| (k.to_string(), v.to_string()))) .map(HashMap::from_iter) .unwrap_or_default(); @@ -403,7 +406,12 @@ impl Solution { ); } - out.extend(self.options.to_environment()); + out.extend( + self.options + .to_environment() + .into_iter() + .map(|(k, v)| (k.to_string(), v.to_string())), + ); out } diff --git a/crates/spk-solve/crates/validation/src/validation_test.rs b/crates/spk-solve/crates/validation/src/validation_test.rs index ddc4ddbc4b..0987d285e4 100644 --- a/crates/spk-solve/crates/validation/src/validation_test.rs +++ b/crates/spk-solve/crates/validation/src/validation_test.rs @@ -52,7 +52,7 @@ fn test_src_package_install_requests_are_not_considered() { .collect(), vec![], vec![], - vec![(opt_name!("debug").to_owned(), "off".to_string())], + vec![(opt_name!("debug").to_owned(), "off".into())], ); for validator in validators { @@ -73,7 +73,7 @@ fn test_empty_options_can_match_anything() { vec![], // this option is requested to be a specific value in the installed // spec file, but is empty so should not cause a conflict - vec![(opt_name!("python.abi").to_owned(), "".to_string())], + vec![(opt_name!("python.abi").to_owned(), "".into())], ); let spec = Arc::new(spec!( diff --git a/crates/spk-solve/crates/validation/src/validators/var_requirements.rs b/crates/spk-solve/crates/validation/src/validators/var_requirements.rs index 4843e54fc3..52665181be 100644 --- a/crates/spk-solve/crates/validation/src/validators/var_requirements.rs +++ b/crates/spk-solve/crates/validation/src/validators/var_requirements.rs @@ -30,7 +30,7 @@ impl ValidatorT for VarRequirementsValidator { continue; } let requested = request.value.as_pinned().unwrap_or_default(); - if requested != value.as_str() { + if requested != &**value { return Ok(Compatibility::incompatible(format!( "package wants {}={requested}, resolve has {name}={value}", request.var diff --git a/crates/spk-solve/src/solver.rs b/crates/spk-solve/src/solver.rs index b07560a51b..ebfcdaa423 100644 --- a/crates/spk-solve/src/solver.rs +++ b/crates/spk-solve/src/solver.rs @@ -355,7 +355,7 @@ impl Solver { if !opts.contains_key(pkg_request.pkg.name.as_opt_name()) { opts.insert( pkg_request.pkg.name.as_opt_name().to_owned(), - pkg_request.pkg.version.to_string(), + pkg_request.pkg.version.to_string().into(), ); } } @@ -363,11 +363,7 @@ impl Solver { if !opts.contains_key(&var_request.var) { opts.insert( var_request.var.clone(), - var_request - .value - .as_pinned() - .unwrap_or_default() - .to_string(), + var_request.value.as_pinned().unwrap_or_default().into(), ); } } diff --git a/crates/spk-solve/src/solver_test.rs b/crates/spk-solve/src/solver_test.rs index ccc368c1da..c9e0935602 100644 --- a/crates/spk-solve/src/solver_test.rs +++ b/crates/spk-solve/src/solver_test.rs @@ -769,16 +769,10 @@ async fn test_solver_option_injection(mut solver: Solver) { let solution = run_and_print_resolve_for_tests(&solver).await.unwrap(); let mut opts = solution.options().clone(); - assert_eq!(opts.remove(opt_name!("vnp3")), Some("~2.0.0".to_string())); - assert_eq!( - opts.remove(opt_name!("vnp3.python")), - Some("~2.7.5".to_string()) - ); - assert_eq!(opts.remove(opt_name!("vnp3.debug")), Some("on".to_string())); - assert_eq!( - opts.remove(opt_name!("python.abi")), - Some("cp27mu".to_string()) - ); + assert_eq!(opts.remove(opt_name!("vnp3")), Some("~2.0.0".into())); + assert_eq!(opts.remove(opt_name!("vnp3.python")), Some("~2.7.5".into())); + assert_eq!(opts.remove(opt_name!("vnp3.debug")), Some("on".into())); + assert_eq!(opts.remove(opt_name!("python.abi")), Some("cp27mu".into())); assert!( !opts.contains_key(opt_name!("vnp3.special")), "should not define empty values" From 97d8075cb1970887948581995df48cc21f7c709c Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Mon, 19 Jun 2023 19:49:05 -0700 Subject: [PATCH 2/2] Use string interning for PkgNameBuf, etc. Use the lasso crate to intern package name strings, so that any instances of the same name share the same backing memory. These are also very cheap to clone because it only clones the "Spur" (a u32). The tradeoff is that there is some contention from using `ThreadedRodeo` since we need to support accessing the backing store from multiple threads. Signed-off-by: J Robert Ray --- Cargo.lock | 16 ++++ Cargo.toml | 1 + crates/parsedbuf/Cargo.toml | 2 + crates/parsedbuf/src/lib.rs | 94 ++++++++++++++----- .../spk-schema/crates/foundation/Cargo.toml | 1 + .../crates/foundation/src/name/error.rs | 2 + .../crates/foundation/src/name/mod.rs | 22 +++-- .../crates/ident/src/parsing/parsing_test.rs | 4 +- 8 files changed, 108 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 38087cc729..32bea75894 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1353,6 +1353,9 @@ name = "hashbrown" version = "0.13.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "43a3c133739dddd0d2990f9a4bdf8eb4b21ef50e4851ca85ab661199821d510e" +dependencies = [ + "ahash", +] [[package]] name = "hashbrown" @@ -1705,6 +1708,16 @@ dependencies = [ "winapi-build", ] +[[package]] +name = "lasso" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4644821e1c3d7a560fe13d842d13f587c07348a1a05d3a797152d41c90c56df2" +dependencies = [ + "dashmap", + "hashbrown 0.13.2", +] + [[package]] name = "lazy_static" version = "1.4.0" @@ -2148,6 +2161,8 @@ dependencies = [ name = "parsedbuf" version = "0.1.0" dependencies = [ + "lasso", + "once_cell", "paste", ] @@ -4104,6 +4119,7 @@ dependencies = [ "format_serde_error", "ignore", "itertools 0.12.0", + "lasso", "miette", "nom", "nom-supreme", diff --git a/Cargo.toml b/Cargo.toml index eadcd8b093..ab836d31e5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,6 +52,7 @@ futures = "0.3.28" futures-core = "0.3.28" glob = "0.3" indicatif = "0.17.5" +lasso = { version = "0.7", features = ["multi-threaded"] } indexmap = "2.2" itertools = "0.12" libc = "0.2.80" diff --git a/crates/parsedbuf/Cargo.toml b/crates/parsedbuf/Cargo.toml index 3464e3da4e..fe44addae3 100644 --- a/crates/parsedbuf/Cargo.toml +++ b/crates/parsedbuf/Cargo.toml @@ -8,4 +8,6 @@ version = "0.1.0" workspace = true [dependencies] +once_cell = { workspace = true } paste = "1.0" +lasso = { workspace = true } diff --git a/crates/parsedbuf/src/lib.rs b/crates/parsedbuf/src/lib.rs index 03143582ee..47b0d1b993 100644 --- a/crates/parsedbuf/src/lib.rs +++ b/crates/parsedbuf/src/lib.rs @@ -2,8 +2,14 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/imageworks/spk +use std::sync::Arc; + pub use paste; +/// Global string interner for common strings. +pub static RODEO: once_cell::sync::Lazy> = + once_cell::sync::Lazy::new(|| Arc::new(lasso::ThreadedRodeo::default())); + /// Generate a pair of types to represent a parsed string type. /// /// A `$type_name::validate()` method must be manually implemented which @@ -39,9 +45,34 @@ macro_rules! parsed { } $crate::paste::paste! { - #[derive(Debug, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)] + #[derive(Debug, Clone, Eq, PartialEq)] #[doc = "An owned, mutable, and validated " $what " string"] - pub struct $owned_type_name(String); + pub struct $owned_type_name(lasso::Spur); + } + + impl std::hash::Hash for $owned_type_name { + #[inline] + fn hash(&self, state: &mut H) { + // Hash the interned string, not the Spur, for consistency + // with hash() on the borrowed type. + $crate::RODEO.resolve(&self.0).hash(state) + } + } + + impl Ord for $owned_type_name { + #[inline] + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + // Order on the interned string, not the Spur, for consistency + // with Ord on the borrowed type (as required by BTreeMap). + self.as_str().cmp(other.as_str()) + } + } + + impl PartialOrd for $owned_type_name { + #[inline] + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } } #[cfg(feature = "parsedbuf-serde")] @@ -74,6 +105,7 @@ macro_rules! parsed { }) } + #[inline] pub fn as_str(&self) -> &str { &self.0 } @@ -90,10 +122,12 @@ macro_rules! parsed { } } + #[inline] pub fn is_empty(&self) -> bool { self.0.is_empty() } + #[inline] pub fn len(&self) -> usize { self.0.len() } @@ -109,27 +143,23 @@ macro_rules! parsed { #[doc = ""] #[doc = "No validation is performed on `name`."] pub unsafe fn from_string(name: String) -> Self { - Self(name) - } - } - - $crate::paste::paste! { - #[doc = "Consume the [`" $owned_type_name "`], returning the inner [`String`]."] - pub fn into_inner(self) -> String { - self.0 + let key = $crate::RODEO.try_get_or_intern(name).expect("won't run out of intern slots"); + Self(key) } } } impl std::borrow::Borrow<$type_name> for $owned_type_name { + #[inline] fn borrow(&self) -> &$type_name { self.as_ref() } } - impl std::borrow::Borrow for $owned_type_name { - fn borrow(&self) -> &String { - &self.0 + impl std::borrow::Borrow for $owned_type_name { + #[inline] + fn borrow(&self) -> &str { + $crate::RODEO.resolve(&self.0) } } @@ -137,99 +167,115 @@ macro_rules! parsed { type Owned = $owned_type_name; fn to_owned(&self) -> Self::Owned { - $owned_type_name(self.0.to_owned()) + let key = $crate::RODEO.try_get_or_intern(&self.0).expect("won't run out of intern slots"); + $owned_type_name(key) } } impl std::cmp::PartialEq<$type_name> for $owned_type_name { + #[inline] fn eq(&self, other: &$type_name) -> bool { - &**self == other + self.as_str() == &other.0 } } impl std::cmp::PartialEq<$owned_type_name> for $type_name { + #[inline] fn eq(&self, other: &$owned_type_name) -> bool { &self.0 == other.as_str() } } impl std::cmp::PartialEq<$owned_type_name> for &$type_name { + #[inline] fn eq(&self, other: &$owned_type_name) -> bool { &self.0 == other.as_str() } } impl std::cmp::PartialEq for $type_name { + #[inline] fn eq(&self, other: &str) -> bool { - self.as_str() == other + &self.0 == other } } impl std::cmp::PartialEq for $owned_type_name { + #[inline] fn eq(&self, other: &str) -> bool { - &**self == other + self.as_str() == other } } impl std::convert::AsRef<$type_name> for $type_name { + #[inline] fn as_ref(&self) -> &$type_name { self } } impl std::convert::AsRef<$type_name> for $owned_type_name { + #[inline] fn as_ref(&self) -> &$type_name { // Safety: from_str bypasses validation but the contents // of owned instance must already be valid - unsafe { $type_name::from_str(&self.0) } + unsafe { $type_name::from_str($crate::RODEO.resolve(&self.0)) } } } impl std::convert::AsRef for $type_name { + #[inline] fn as_ref(&self) -> &std::ffi::OsStr { std::ffi::OsStr::new(&self.0) } } impl std::convert::AsRef for $type_name { + #[inline] fn as_ref(&self) -> &std::path::Path { std::path::Path::new(&self.0) } } impl std::convert::AsRef for $owned_type_name { + #[inline] fn as_ref(&self) -> &std::path::Path { - std::path::Path::new(&self.0) + std::path::Path::new($crate::RODEO.resolve(&self.0)) } } impl std::convert::AsRef for $owned_type_name { + #[inline] fn as_ref(&self) -> &str { - &self.0 + $crate::RODEO.resolve(&self.0) } } impl std::cmp::PartialEq<&str> for $type_name { + #[inline] fn eq(&self, other: &&str) -> bool { - self.as_str() == *other + &self.0 == *other } } impl std::cmp::PartialEq<&str> for $owned_type_name { + #[inline] fn eq(&self, other: &&str) -> bool { - &**self == other + self.as_str() == *other } } impl std::convert::From<&$type_name> for $owned_type_name { + #[inline] fn from(name: &$type_name) -> Self { name.to_owned() } } impl std::convert::From<$owned_type_name> for String { + #[inline] fn from(val: $owned_type_name) -> Self { - val.0 + $crate::RODEO.resolve(&val.0).to_string() } } @@ -273,7 +319,7 @@ macro_rules! parsed { impl std::fmt::Display for $owned_type_name { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.0.fmt(f) + $crate::RODEO.resolve(&self.0).fmt(f) } } diff --git a/crates/spk-schema/crates/foundation/Cargo.toml b/crates/spk-schema/crates/foundation/Cargo.toml index ab8e7aeca0..2ef6d6a28d 100644 --- a/crates/spk-schema/crates/foundation/Cargo.toml +++ b/crates/spk-schema/crates/foundation/Cargo.toml @@ -24,6 +24,7 @@ format_serde_error = { version = "0.3", default_features = false, features = [ ] } ignore = "0.4.18" itertools = { workspace = true } +lasso = { workspace = true } nom = { workspace = true } nom-supreme = { workspace = true } once_cell = { workspace = true } diff --git a/crates/spk-schema/crates/foundation/src/name/error.rs b/crates/spk-schema/crates/foundation/src/name/error.rs index d9c0c3e45b..adfb4a3644 100644 --- a/crates/spk-schema/crates/foundation/src/name/error.rs +++ b/crates/spk-schema/crates/foundation/src/name/error.rs @@ -18,4 +18,6 @@ pub enum Error { #[error(transparent)] #[diagnostic(forward(0))] InvalidNameError(#[from] super::InvalidNameError), + #[error(transparent)] + LassoError(#[from] lasso::LassoError), } diff --git a/crates/spk-schema/crates/foundation/src/name/mod.rs b/crates/spk-schema/crates/foundation/src/name/mod.rs index 453ca2b023..6941a137a3 100644 --- a/crates/spk-schema/crates/foundation/src/name/mod.rs +++ b/crates/spk-schema/crates/foundation/src/name/mod.rs @@ -215,17 +215,23 @@ impl OptName { /// Return a copy of this option, adding the provided namespace if there isn't already one set pub fn with_default_namespace>(&self, ns: N) -> OptNameBuf { - OptNameBuf(format!( - "{}{}{}", - self.namespace().unwrap_or_else(|| ns.as_ref()), - Self::SEP, - self.base_name() - )) + // Safety: the individual components are already validated. + unsafe { + OptNameBuf::from_string(format!( + "{}{}{}", + self.namespace().unwrap_or_else(|| ns.as_ref()), + Self::SEP, + self.base_name() + )) + } } /// Return a copy of this option, replacing any namespace with the provided one pub fn with_namespace>(&self, ns: N) -> OptNameBuf { - OptNameBuf(format!("{}{}{}", ns.as_ref(), Self::SEP, self.base_name())) + // Safety: the individual components are already validated. + unsafe { + OptNameBuf::from_string(format!("{}{}{}", ns.as_ref(), Self::SEP, self.base_name())) + } } /// Return an option with the same name but no associated namespace @@ -371,6 +377,6 @@ impl RepositoryName { impl RepositoryNameBuf { /// Return if this RepositoryName names the "local" repository pub fn is_local(&self) -> bool { - self.0 == "local" + self.as_str() == "local" } } diff --git a/crates/spk-schema/crates/ident/src/parsing/parsing_test.rs b/crates/spk-schema/crates/ident/src/parsing/parsing_test.rs index 81da5941ac..54a7abb06f 100644 --- a/crates/spk-schema/crates/ident/src/parsing/parsing_test.rs +++ b/crates/spk-schema/crates/ident/src/parsing/parsing_test.rs @@ -72,7 +72,7 @@ prop_compose! { "run[a-z]+".prop_map(Component::Named), "build[a-z]+".prop_map(Component::Named), "src[a-z]+".prop_map(Component::Named), - arb_pkg_legal_name().prop_filter("name can't be a reserved name", |name| !(name == "all" || name == "run" || name == "build" || name == "src")).prop_map(|name| Component::Named(name.into_inner())), + arb_pkg_legal_name().prop_filter("name can't be a reserved name", |name| !(name == "all" || name == "run" || name == "build" || name == "src")).prop_map(|name| Component::Named(name.to_string())), ]) -> Component { component } @@ -159,7 +159,7 @@ fn arb_opt_version_filter() -> impl Strategy> { } prop_compose! { - fn arb_repo()(name in weighted(0.9, prop_oneof!["local", "origin", arb_pkg_legal_name().prop_map(|name| name.into_inner())])) -> Option { + fn arb_repo()(name in weighted(0.9, prop_oneof!["local", "origin", arb_pkg_legal_name().prop_map(|name| name.to_string())])) -> Option { // Safety: We only generate legal repository names. name.map(|n| unsafe { RepositoryNameBuf::from_string(n) }) }