Use Arc<str> 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 <jrray@jrray.org>

Author: jrray

crates/spk-build/src/build/binary.rs

@@ -665,7 +665,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

crates/spk-build/src/build/binary_test.rs

@@ -193,10 +193,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]

crates/spk-cli/common/src/flags.rs

@@ -271,7 +271,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)
@@ -282,7 +282,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())
     }
 }

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);
 }

crates/spk-schema/crates/foundation/Cargo.toml

@@ -25,7 +25,7 @@ parsedbuf = { path = "../../../parsedbuf" }
 relative-path = "1.3"
 ring = "0.16.15"
 rstest = { workspace = true }
-serde = { workspace = true, features = ["derive"] }
+serde = { workspace = true, features = ["derive", "rc"] }
 serde_yaml = "0.8.17"
 spfs = { version = '0.34.6', path = "../../../spfs" }
 sys-info = "0.9.0"

crates/spk-schema/crates/foundation/src/option_map/mod.rs

@@ -70,11 +70,11 @@ pub fn host_options() -> Result<OptionMap> {
     };
 
     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) => {
@@ -90,11 +90,11 @@ pub fn host_options() -> Result<OptionMap> {
 #[derive(Default, Clone, Hash, PartialEq, Eq, Serialize, Ord, PartialOrd)]
 #[serde(transparent)]
 pub struct OptionMap {
-    options: BTreeMap<OptNameBuf, String>,
+    options: BTreeMap<OptNameBuf, Arc<str>>,
 }
 
 impl std::ops::Deref for OptionMap {
-    type Target = BTreeMap<OptNameBuf, String>;
+    type Target = BTreeMap<OptNameBuf, Arc<str>>;
 
     fn deref(&self) -> &Self::Target {
         &self.options
@@ -107,16 +107,16 @@ impl std::ops::DerefMut for OptionMap {
     }
 }
 
-impl From<&Arc<BTreeMap<OptNameBuf, String>>> for OptionMap {
-    fn from(hm: &Arc<BTreeMap<OptNameBuf, String>>) -> Self {
+impl From<&Arc<BTreeMap<OptNameBuf, Arc<str>>>> for OptionMap {
+    fn from(hm: &Arc<BTreeMap<OptNameBuf, Arc<str>>>) -> Self {
         Self {
             options: (**hm).clone(),
         }
     }
 }
 
-impl FromIterator<(OptNameBuf, String)> for OptionMap {
-    fn from_iter<T: IntoIterator<Item = (OptNameBuf, String)>>(iter: T) -> Self {
+impl FromIterator<(OptNameBuf, Arc<str>)> for OptionMap {
+    fn from_iter<T: IntoIterator<Item = (OptNameBuf, Arc<str>)>>(iter: T) -> Self {
         Self {
             options: BTreeMap::from_iter(iter),
         }
@@ -137,8 +137,8 @@ impl std::fmt::Display for OptionMap {
 }
 
 impl IntoIterator for OptionMap {
-    type IntoIter = std::collections::btree_map::IntoIter<OptNameBuf, String>;
-    type Item = (OptNameBuf, String);
+    type IntoIter = std::collections::btree_map::IntoIter<OptNameBuf, Arc<str>>;
+    type Item = (OptNameBuf, Arc<str>);
 
     fn into_iter(self) -> Self::IntoIter {
         self.options.into_iter()
@@ -147,16 +147,16 @@ impl IntoIterator for OptionMap {
 
 impl OptionMap {
     /// Return the data of these options as environment variables.
-    pub fn to_environment(&self) -> HashMap<String, String> {
+    pub fn to_environment(&self) -> HashMap<Arc<str>, Arc<str>> {
         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
     }
 
-    fn items(&self) -> Vec<(OptNameBuf, String)> {
+    fn items(&self) -> Vec<(OptNameBuf, Arc<str>)> {
         self.options
             .iter()
             .map(|(k, v)| (k.to_owned(), v.to_owned()))
@@ -278,7 +278,7 @@ impl<'de> Deserialize<'de> for OptionMap {
             {
                 let mut options = OptionMap::default();
                 while let Some((name, value)) = map.next_entry::<OptNameBuf, Stringified>()? {
-                    options.insert(name, value.0);
+                    options.insert(name, value.0.into());
                 }
                 Ok(options)
             }

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]

crates/spk-schema/src/option.rs

@@ -98,7 +98,10 @@ impl Opt {
         }
     }
 
-    pub fn validate(&self, value: Option<&str>) -> Compatibility {
+    pub fn validate<S>(&self, value: Option<S>) -> Compatibility
+    where
+        S: AsRef<str>,
+    {
         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<S>(&self, given: Option<S>) -> String
+    where
+        S: Copy + ToString,
+    {
         let value = match self {
             Self::Pkg(opt) => opt.get_value(given),
             Self::Var(opt) => opt.get_value(given),
@@ -378,7 +384,10 @@ impl VarOpt {
         })
     }
 
-    pub fn get_value(&self, given: Option<&str>) -> Option<String> {
+    pub fn get_value<S>(&self, given: Option<S>) -> Option<String>
+    where
+        S: ToString,
+    {
         if let Some(v) = &self.value {
             if !v.is_empty() {
                 return Some(v.clone());
@@ -404,12 +413,15 @@ impl VarOpt {
         Ok(())
     }
 
-    pub fn validate(&self, value: Option<&str>) -> Compatibility {
+    pub fn validate<S>(&self, value: Option<S>) -> Compatibility
+    where
+        S: AsRef<str>,
+    {
         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::<str>::as_ref), assigned) {
             (None, Some(_)) => Compatibility::Compatible,
             (Some(value), Some(assigned)) => {
                 if value == assigned {
@@ -496,7 +508,10 @@ impl PkgOpt {
         })
     }
 
-    pub fn get_value(&self, given: Option<&str>) -> Option<String> {
+    pub fn get_value<S>(&self, given: Option<S>) -> Option<String>
+    where
+        S: ToString,
+    {
         if let Some(v) = &self.value {
             Some(v.clone())
         } else if let Some(v) = given {
@@ -533,8 +548,14 @@ impl PkgOpt {
         Ok(())
     }
 
-    pub fn validate(&self, value: Option<&str>) -> Compatibility {
-        let value = value.unwrap_or_default();
+    pub fn validate<S>(&self, value: Option<S>) -> Compatibility
+    where
+        S: AsRef<str>,
+    {
+        let value = value
+            .as_ref()
+            .map(AsRef::<str>::as_ref)
+            .unwrap_or_else(|| "");
 
         // skip any default that might exist since
         // that does not represent a definitive range

crates/spk-schema/src/option_test.rs

@@ -44,7 +44,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);
 }
 

crates/spk-schema/src/package.rs

@@ -69,9 +69,7 @@ pub trait Package:
         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.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!(