From 016fd7c67380e77ecbca53c2c6a7408772ea7fe1 Mon Sep 17 00:00:00 2001 From: Stiopa Koltsov Date: Tue, 14 Sep 2021 18:50:09 -0700 Subject: [PATCH] Use StringValue in dict key in ParametersSpec::collect Summary: Before this diff `kwargs` were collected into `Dict` which is `SmallMap, Value>`. Now `kwargs` are collected into `SmallMap, 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](https://github.com/bluss/indexmap/pull/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 --- starlark/src/eval/runtime/arguments.rs | 42 ++++++++++++++++---------- starlark/src/values/layout/constant.rs | 14 +++++++++ 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/starlark/src/eval/runtime/arguments.rs b/starlark/src/eval/runtime/arguments.rs index 2601d41cb..8ae9733e6 100644 --- a/starlark/src/eval/runtime/arguments.rs +++ b/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 { // Return true if the value is a duplicate #[inline(always)] fn add_kwargs<'v>( - kwargs: &mut Option>>, - key: Hashed>, + kwargs: &mut Option, Value<'v>>>>, + key: Hashed>, 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 { 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 { 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 { }; 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 { } 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()); diff --git a/starlark/src/values/layout/constant.rs b/starlark/src/values/layout/constant.rs index d61a4451b..59bdd444f 100644 --- a/starlark/src/values/layout/constant.rs +++ b/starlark/src/values/layout/constant.rs @@ -100,6 +100,8 @@ unsafe impl<'v> Coerce> for FrozenStringValue {} unsafe impl<'v> CoerceKey> for FrozenStringValue {} unsafe impl<'v> Coerce> for StringValue<'v> {} unsafe impl<'v> CoerceKey> for StringValue<'v> {} +unsafe impl<'v> Coerce> for StringValue<'v> {} +unsafe impl<'v> CoerceKey> 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 }