Use StringValue in dict key in ParametersSpec::collect

Summary:
Before this diff `kwargs` were collected into `Dict` which is `SmallMap<Hashed<Value>, Value>`.

Now `kwargs` are collected into `SmallMap<Hashed<StringValue>, Value>` and that `SmallMap` is "coerced" into a map with `Value` key.

When inserting keys we lookup previous keys (this should be partially addressed by [this PR in indexmap](indexmap-rs/indexmap#200)), and equality operation on `StringValue` is cheaper that equality on `Value` because there's no dynamic casts. We don't do real equality often (because hash collisions are rare), but having `StringValue` instead of `Value` may generate more efficient machine code.

Also this makes code a little more type-safe.

Reviewed By: ndmitchell

Differential Revision: D30921794

fbshipit-source-id: cf2b4fa72eeef150e6308d2fe2d7c16f59166586

Author: stepancheg

starlark/src/eval/runtime/arguments.rs

@@ -30,7 +30,11 @@ use crate::{
     },
 };
 use either::Either;
-use gazebo::{cell::ARef, coerce::Coerce, prelude::*};
+use gazebo::{
+    cell::ARef,
+    coerce::{coerce, Coerce},
+    prelude::*,
+};
 use itertools::Itertools;
 use std::{cell::Cell, cmp, convert::TryInto, intrinsics::unlikely, iter};
 use thiserror::Error;
@@ -371,18 +375,18 @@ impl<'v, V: ValueLike<'v>> ParametersSpec<V> {
         // Return true if the value is a duplicate
         #[inline(always)]
         fn add_kwargs<'v>(
-            kwargs: &mut Option<Box<Dict<'v>>>,
-            key: Hashed<Value<'v>>,
+            kwargs: &mut Option<Box<SmallMap<StringValue<'v>, Value<'v>>>>,
+            key: Hashed<StringValue<'v>>,
             val: Value<'v>,
         ) -> bool {
             match kwargs {
                 None => {
                     let mut mp = SmallMap::with_capacity_largest_vec();
                     mp.insert_hashed(key, val);
-                    *kwargs = Some(box Dict::new(mp));
+                    *kwargs = Some(box mp);
                     false
                 }
-                Some(mp) => mp.content.insert_hashed(key, val).is_some(),
+                Some(mp) => mp.insert_hashed(key, val).is_some(),
             }
         }
 
@@ -425,7 +429,7 @@ impl<'v, V: ValueLike<'v>> ParametersSpec<V> {
                     None => {
                         add_kwargs(
                             &mut kwargs,
-                            Hashed::new_unchecked(name.small_hash(), name_value.to_value()),
+                            Hashed::new_unchecked(name.small_hash(), *name_value),
                             *v,
                         );
                     }
@@ -466,14 +470,17 @@ impl<'v, V: ValueLike<'v>> ParametersSpec<V> {
             match Dict::from_value(param_kwargs) {
                 Some(y) => {
                     for (k, v) in y.iter_hashed() {
-                        match k.key().unpack_str() {
+                        match StringValue::new(*k.key()) {
                             None => return Err(FunctionError::ArgsValueIsNotString.into()),
                             Some(s) => {
-                                let repeat = match self
-                                    .names
-                                    .get_hashed_str(BorrowHashed::new_unchecked(k.hash(), s))
-                                {
-                                    None => add_kwargs(&mut kwargs, k, v),
+                                let repeat = match self.names.get_hashed_str(
+                                    BorrowHashed::new_unchecked(k.hash(), s.as_str()),
+                                ) {
+                                    None => add_kwargs(
+                                        &mut kwargs,
+                                        Hashed::new_unchecked(k.hash(), s),
+                                        v,
+                                    ),
                                     Some(i) => {
                                         let this_slot = &slots[*i];
                                         let repeat = this_slot.get().is_some();
@@ -483,7 +490,7 @@ impl<'v, V: ValueLike<'v>> ParametersSpec<V> {
                                 };
                                 if unlikely(repeat) {
                                     return Err(FunctionError::RepeatedParameter {
-                                        name: s.to_owned(),
+                                        name: s.as_str().to_owned(),
                                     }
                                     .into());
                                 }
@@ -538,11 +545,14 @@ impl<'v, V: ValueLike<'v>> ParametersSpec<V> {
         }
 
         if let Some(kwargs_pos) = self.kwargs {
-            let kwargs = kwargs.take().unwrap_or_default();
-            slots[kwargs_pos].set(Some(heap.alloc(*kwargs)));
+            let kwargs = match kwargs.take() {
+                Some(kwargs) => Dict::new(coerce(*kwargs)),
+                None => Dict::default(),
+            };
+            slots[kwargs_pos].set(Some(heap.alloc(kwargs)));
         } else if let Some(kwargs) = kwargs {
             return Err(FunctionError::ExtraNamedParameters {
-                names: kwargs.content.keys().map(|x| x.to_str()).collect(),
+                names: kwargs.keys().map(|x| x.as_str().to_owned()).collect(),
                 function: self.signature(),
             }
             .into());

starlark/src/values/layout/constant.rs

@@ -100,6 +100,8 @@ unsafe impl<'v> Coerce<StringValue<'v>> for FrozenStringValue {}
 unsafe impl<'v> CoerceKey<StringValue<'v>> for FrozenStringValue {}
 unsafe impl<'v> Coerce<StringValue<'v>> for StringValue<'v> {}
 unsafe impl<'v> CoerceKey<StringValue<'v>> for StringValue<'v> {}
+unsafe impl<'v> Coerce<Value<'v>> for StringValue<'v> {}
+unsafe impl<'v> CoerceKey<Value<'v>> for StringValue<'v> {}
 
 impl PartialEq for FrozenStringValue {
     fn eq(&self, other: &Self) -> bool {
@@ -138,6 +140,14 @@ impl FrozenStringValue {
     }
 }
 
+impl<'v> PartialEq for StringValue<'v> {
+    fn eq(&self, other: &Self) -> bool {
+        self.0.ptr_eq(other.0) || self.as_str() == other.as_str()
+    }
+}
+
+impl<'v> Eq for StringValue<'v> {}
+
 impl<'v> StringValue<'v> {
     /// Construct without a check that the value contains a string.
     ///
@@ -161,6 +171,10 @@ impl<'v> StringValue<'v> {
         unsafe { &self.0.0.unpack_ptr_no_int_unchecked().as_repr().payload }
     }
 
+    pub(crate) fn as_str(self) -> &'v str {
+        self.unpack_starlark_str().unpack()
+    }
+
     pub(crate) fn to_value(self) -> Value<'v> {
         self.0
     }