Shrink Value by boxing Range/Closure (#12784)

# Description
On 64-bit platforms the current size of `Value` is 56 bytes. The
limiting variants were `Closure` and `Range`. Boxing the two reduces the
size of Value to 48 bytes. This is the minimal size possible with our
current 16-byte `Span` and any 24-byte `Vec` container which we use in
several variants. (Note the extra full 8-bytes necessary for the
discriminant or other smaller values due to the 8-byte alignment of
`usize`)

This is leads to a size reduction of ~15% for `Value` and should overall
be beneficial as both `Range` and `Closure` are rarely used compared to
the primitive types or even our general container types.

# User-Facing Changes
Less memory used, potential runtime benefits.

(Too late in the evening to run the benchmarks myself right now)
This commit is contained in:
Stefan Holderbach 2024-05-09 02:10:58 +02:00 committed by GitHub
parent 92831d7efc
commit ba6f38510c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
13 changed files with 39 additions and 36 deletions

View file

@ -65,7 +65,7 @@ fn get_prompt_string(
.get_env_var(engine_state, prompt) .get_env_var(engine_state, prompt)
.and_then(|v| match v { .and_then(|v| match v {
Value::Closure { val, .. } => { Value::Closure { val, .. } => {
let result = ClosureEvalOnce::new(engine_state, stack, val) let result = ClosureEvalOnce::new(engine_state, stack, *val)
.run_with_input(PipelineData::Empty); .run_with_input(PipelineData::Empty);
trace!( trace!(

View file

@ -223,7 +223,7 @@ impl<'a> std::fmt::Debug for DebuggableValue<'a> {
Value::Date { val, .. } => { Value::Date { val, .. } => {
write!(f, "Date({:?})", val) write!(f, "Date({:?})", val)
} }
Value::Range { val, .. } => match val { Value::Range { val, .. } => match **val {
Range::IntRange(range) => match range.end() { Range::IntRange(range) => match range.end() {
Bound::Included(end) => write!( Bound::Included(end) => write!(
f, f,

View file

@ -146,7 +146,10 @@ impl<'a> StyleComputer<'a> {
let span = value.span(); let span = value.span();
match value { match value {
Value::Closure { val, .. } => { Value::Closure { val, .. } => {
map.insert(key.to_string(), ComputableStyle::Closure(val.clone(), span)); map.insert(
key.to_string(),
ComputableStyle::Closure(*val.clone(), span),
);
} }
Value::Record { .. } => { Value::Record { .. } => {
map.insert( map.insert(

View file

@ -139,7 +139,7 @@ pub fn group_by(
match grouper { match grouper {
Value::CellPath { val, .. } => group_cell_path(val, values)?, Value::CellPath { val, .. } => group_cell_path(val, values)?,
Value::Closure { val, .. } => { Value::Closure { val, .. } => {
group_closure(values, span, val, engine_state, stack)? group_closure(values, span, *val, engine_state, stack)?
} }
_ => { _ => {
return Err(ShellError::TypeMismatch { return Err(ShellError::TypeMismatch {

View file

@ -133,7 +133,7 @@ fn insert(
if let Value::Closure { val, .. } = replacement { if let Value::Closure { val, .. } = replacement {
match (cell_path.members.first(), &mut value) { match (cell_path.members.first(), &mut value) {
(Some(PathMember::String { .. }), Value::List { vals, .. }) => { (Some(PathMember::String { .. }), Value::List { vals, .. }) => {
let mut closure = ClosureEval::new(engine_state, stack, val); let mut closure = ClosureEval::new(engine_state, stack, *val);
for val in vals { for val in vals {
insert_value_by_closure( insert_value_by_closure(
val, val,
@ -147,7 +147,7 @@ fn insert(
(first, _) => { (first, _) => {
insert_single_value_by_closure( insert_single_value_by_closure(
&mut value, &mut value,
ClosureEvalOnce::new(engine_state, stack, val), ClosureEvalOnce::new(engine_state, stack, *val),
head, head,
&cell_path.members, &cell_path.members,
matches!(first, Some(PathMember::Int { .. })), matches!(first, Some(PathMember::Int { .. })),
@ -188,7 +188,7 @@ fn insert(
let value = stream.next(); let value = stream.next();
let end_of_stream = value.is_none(); let end_of_stream = value.is_none();
let value = value.unwrap_or(Value::nothing(head)); let value = value.unwrap_or(Value::nothing(head));
let new_value = ClosureEvalOnce::new(engine_state, stack, val) let new_value = ClosureEvalOnce::new(engine_state, stack, *val)
.run_with_value(value.clone())? .run_with_value(value.clone())?
.into_value(head); .into_value(head);
@ -203,7 +203,7 @@ fn insert(
if let Value::Closure { val, .. } = replacement { if let Value::Closure { val, .. } = replacement {
insert_single_value_by_closure( insert_single_value_by_closure(
&mut value, &mut value,
ClosureEvalOnce::new(engine_state, stack, val), ClosureEvalOnce::new(engine_state, stack, *val),
head, head,
path, path,
true, true,
@ -224,7 +224,7 @@ fn insert(
.chain(stream) .chain(stream)
.into_pipeline_data_with_metadata(head, engine_state.ctrlc.clone(), metadata)) .into_pipeline_data_with_metadata(head, engine_state.ctrlc.clone(), metadata))
} else if let Value::Closure { val, .. } = replacement { } else if let Value::Closure { val, .. } = replacement {
let mut closure = ClosureEval::new(engine_state, stack, val); let mut closure = ClosureEval::new(engine_state, stack, *val);
let stream = stream.map(move |mut value| { let stream = stream.map(move |mut value| {
let err = insert_value_by_closure( let err = insert_value_by_closure(
&mut value, &mut value,

View file

@ -117,7 +117,7 @@ fn update(
if let Value::Closure { val, .. } = replacement { if let Value::Closure { val, .. } = replacement {
match (cell_path.members.first(), &mut value) { match (cell_path.members.first(), &mut value) {
(Some(PathMember::String { .. }), Value::List { vals, .. }) => { (Some(PathMember::String { .. }), Value::List { vals, .. }) => {
let mut closure = ClosureEval::new(engine_state, stack, val); let mut closure = ClosureEval::new(engine_state, stack, *val);
for val in vals { for val in vals {
update_value_by_closure( update_value_by_closure(
val, val,
@ -131,7 +131,7 @@ fn update(
(first, _) => { (first, _) => {
update_single_value_by_closure( update_single_value_by_closure(
&mut value, &mut value,
ClosureEvalOnce::new(engine_state, stack, val), ClosureEvalOnce::new(engine_state, stack, *val),
head, head,
&cell_path.members, &cell_path.members,
matches!(first, Some(PathMember::Int { .. })), matches!(first, Some(PathMember::Int { .. })),
@ -175,7 +175,7 @@ fn update(
if let Value::Closure { val, .. } = replacement { if let Value::Closure { val, .. } = replacement {
update_single_value_by_closure( update_single_value_by_closure(
value, value,
ClosureEvalOnce::new(engine_state, stack, val), ClosureEvalOnce::new(engine_state, stack, *val),
head, head,
path, path,
true, true,
@ -189,7 +189,7 @@ fn update(
.chain(stream) .chain(stream)
.into_pipeline_data_with_metadata(head, engine_state.ctrlc.clone(), metadata)) .into_pipeline_data_with_metadata(head, engine_state.ctrlc.clone(), metadata))
} else if let Value::Closure { val, .. } = replacement { } else if let Value::Closure { val, .. } = replacement {
let mut closure = ClosureEval::new(engine_state, stack, val); let mut closure = ClosureEval::new(engine_state, stack, *val);
let stream = stream.map(move |mut value| { let stream = stream.map(move |mut value| {
let err = update_value_by_closure( let err = update_value_by_closure(
&mut value, &mut value,

View file

@ -163,7 +163,7 @@ fn upsert(
if let Value::Closure { val, .. } = replacement { if let Value::Closure { val, .. } = replacement {
match (cell_path.members.first(), &mut value) { match (cell_path.members.first(), &mut value) {
(Some(PathMember::String { .. }), Value::List { vals, .. }) => { (Some(PathMember::String { .. }), Value::List { vals, .. }) => {
let mut closure = ClosureEval::new(engine_state, stack, val); let mut closure = ClosureEval::new(engine_state, stack, *val);
for val in vals { for val in vals {
upsert_value_by_closure( upsert_value_by_closure(
val, val,
@ -177,7 +177,7 @@ fn upsert(
(first, _) => { (first, _) => {
upsert_single_value_by_closure( upsert_single_value_by_closure(
&mut value, &mut value,
ClosureEvalOnce::new(engine_state, stack, val), ClosureEvalOnce::new(engine_state, stack, *val),
head, head,
&cell_path.members, &cell_path.members,
matches!(first, Some(PathMember::Int { .. })), matches!(first, Some(PathMember::Int { .. })),
@ -216,7 +216,7 @@ fn upsert(
let value = if path.is_empty() { let value = if path.is_empty() {
let value = stream.next().unwrap_or(Value::nothing(head)); let value = stream.next().unwrap_or(Value::nothing(head));
if let Value::Closure { val, .. } = replacement { if let Value::Closure { val, .. } = replacement {
ClosureEvalOnce::new(engine_state, stack, val) ClosureEvalOnce::new(engine_state, stack, *val)
.run_with_value(value)? .run_with_value(value)?
.into_value(head) .into_value(head)
} else { } else {
@ -226,7 +226,7 @@ fn upsert(
if let Value::Closure { val, .. } = replacement { if let Value::Closure { val, .. } = replacement {
upsert_single_value_by_closure( upsert_single_value_by_closure(
&mut value, &mut value,
ClosureEvalOnce::new(engine_state, stack, val), ClosureEvalOnce::new(engine_state, stack, *val),
head, head,
path, path,
true, true,
@ -249,7 +249,7 @@ fn upsert(
.chain(stream) .chain(stream)
.into_pipeline_data_with_metadata(head, engine_state.ctrlc.clone(), metadata)) .into_pipeline_data_with_metadata(head, engine_state.ctrlc.clone(), metadata))
} else if let Value::Closure { val, .. } = replacement { } else if let Value::Closure { val, .. } = replacement {
let mut closure = ClosureEval::new(engine_state, stack, val); let mut closure = ClosureEval::new(engine_state, stack, *val);
let stream = stream.map(move |mut value| { let stream = stream.map(move |mut value| {
let err = upsert_value_by_closure( let err = upsert_value_by_closure(
&mut value, &mut value,

View file

@ -103,7 +103,7 @@ impl Command for Zip {
let metadata = input.metadata(); let metadata = input.metadata();
let other = if let Value::Closure { val, .. } = other { let other = if let Value::Closure { val, .. } = other {
// If a closure was provided, evaluate it and consume its stream output // If a closure was provided, evaluate it and consume its stream output
ClosureEvalOnce::new(engine_state, stack, val).run_with_input(PipelineData::Empty)? ClosureEvalOnce::new(engine_state, stack, *val).run_with_input(PipelineData::Empty)?
} else { } else {
other.into_pipeline_data() other.into_pipeline_data()
}; };

View file

@ -106,7 +106,7 @@ impl<'a> PluginExecutionContext for PluginExecutionCommandContext<'a> {
let span = value.span(); let span = value.span();
match value { match value {
Value::Closure { val, .. } => { Value::Closure { val, .. } => {
ClosureEvalOnce::new(&self.engine_state, &self.stack, val) ClosureEvalOnce::new(&self.engine_state, &self.stack, *val)
.run_with_input(PipelineData::Empty) .run_with_input(PipelineData::Empty)
.map(|data| data.into_value(span)) .map(|data| data.into_value(span))
.unwrap_or_else(|err| Value::error(err, self.call.head)) .unwrap_or_else(|err| Value::error(err, self.call.head))

View file

@ -421,7 +421,7 @@ impl PipelineData {
) )
.into_iter(), .into_iter(),
), ),
Value::Range { val, .. } => PipelineIteratorInner::ListStream( Value::Range { ref val, .. } => PipelineIteratorInner::ListStream(
ListStream::new(val.into_range_iter(value.span(), None), val_span, None) ListStream::new(val.into_range_iter(value.span(), None), val_span, None)
.into_iter(), .into_iter(),
), ),
@ -801,7 +801,7 @@ impl PipelineData {
let span = v.span(); let span = v.span();
match v { match v {
Value::Range { val, .. } => { Value::Range { val, .. } => {
match val { match *val {
Range::IntRange(range) => { Range::IntRange(range) => {
if range.is_unbounded() { if range.is_unbounded() {
return Err(ShellError::GenericError { return Err(ShellError::GenericError {

View file

@ -442,7 +442,7 @@ impl FromValue for Spanned<DateTime<FixedOffset>> {
impl FromValue for Range { impl FromValue for Range {
fn from_value(v: Value) -> Result<Self, ShellError> { fn from_value(v: Value) -> Result<Self, ShellError> {
match v { match v {
Value::Range { val, .. } => Ok(val), Value::Range { val, .. } => Ok(*val),
v => Err(ShellError::CantConvert { v => Err(ShellError::CantConvert {
to_type: "range".into(), to_type: "range".into(),
from_type: v.get_type().to_string(), from_type: v.get_type().to_string(),
@ -457,7 +457,7 @@ impl FromValue for Spanned<Range> {
fn from_value(v: Value) -> Result<Self, ShellError> { fn from_value(v: Value) -> Result<Self, ShellError> {
let span = v.span(); let span = v.span();
match v { match v {
Value::Range { val, .. } => Ok(Spanned { item: val, span }), Value::Range { val, .. } => Ok(Spanned { item: *val, span }),
v => Err(ShellError::CantConvert { v => Err(ShellError::CantConvert {
to_type: "range".into(), to_type: "range".into(),
from_type: v.get_type().to_string(), from_type: v.get_type().to_string(),
@ -552,7 +552,7 @@ impl FromValue for Record {
impl FromValue for Closure { impl FromValue for Closure {
fn from_value(v: Value) -> Result<Self, ShellError> { fn from_value(v: Value) -> Result<Self, ShellError> {
match v { match v {
Value::Closure { val, .. } => Ok(val), Value::Closure { val, .. } => Ok(*val),
v => Err(ShellError::CantConvert { v => Err(ShellError::CantConvert {
to_type: "Closure".into(), to_type: "Closure".into(),
from_type: v.get_type().to_string(), from_type: v.get_type().to_string(),
@ -567,7 +567,7 @@ impl FromValue for Spanned<Closure> {
fn from_value(v: Value) -> Result<Self, ShellError> { fn from_value(v: Value) -> Result<Self, ShellError> {
let span = v.span(); let span = v.span();
match v { match v {
Value::Closure { val, .. } => Ok(Spanned { item: val, span }), Value::Closure { val, .. } => Ok(Spanned { item: *val, span }),
v => Err(ShellError::CantConvert { v => Err(ShellError::CantConvert {
to_type: "Closure".into(), to_type: "Closure".into(),
from_type: v.get_type().to_string(), from_type: v.get_type().to_string(),

View file

@ -86,7 +86,7 @@ pub enum Value {
internal_span: Span, internal_span: Span,
}, },
Range { Range {
val: Range, val: Box<Range>,
// note: spans are being refactored out of Value // note: spans are being refactored out of Value
// please use .span() instead of matching this span value // please use .span() instead of matching this span value
#[serde(rename = "span")] #[serde(rename = "span")]
@ -122,7 +122,7 @@ pub enum Value {
internal_span: Span, internal_span: Span,
}, },
Closure { Closure {
val: Closure, val: Box<Closure>,
// note: spans are being refactored out of Value // note: spans are being refactored out of Value
// please use .span() instead of matching this span value // please use .span() instead of matching this span value
#[serde(rename = "span")] #[serde(rename = "span")]
@ -182,7 +182,7 @@ impl Clone for Value {
internal_span: *internal_span, internal_span: *internal_span,
}, },
Value::Range { val, internal_span } => Value::Range { Value::Range { val, internal_span } => Value::Range {
val: *val, val: val.clone(),
internal_span: *internal_span, internal_span: *internal_span,
}, },
Value::Float { val, internal_span } => Value::float(*val, *internal_span), Value::Float { val, internal_span } => Value::float(*val, *internal_span),
@ -327,7 +327,7 @@ impl Value {
/// Returns a reference to the inner [`Range`] value or an error if this `Value` is not a range /// Returns a reference to the inner [`Range`] value or an error if this `Value` is not a range
pub fn as_range(&self) -> Result<Range, ShellError> { pub fn as_range(&self) -> Result<Range, ShellError> {
if let Value::Range { val, .. } = self { if let Value::Range { val, .. } = self {
Ok(*val) Ok(**val)
} else { } else {
self.cant_convert_to("range") self.cant_convert_to("range")
} }
@ -336,7 +336,7 @@ impl Value {
/// Unwraps the inner [`Range`] value or returns an error if this `Value` is not a range /// Unwraps the inner [`Range`] value or returns an error if this `Value` is not a range
pub fn into_range(self) -> Result<Range, ShellError> { pub fn into_range(self) -> Result<Range, ShellError> {
if let Value::Range { val, .. } = self { if let Value::Range { val, .. } = self {
Ok(val) Ok(*val)
} else { } else {
self.cant_convert_to("range") self.cant_convert_to("range")
} }
@ -553,7 +553,7 @@ impl Value {
/// Unwraps the inner [`Closure`] value or returns an error if this `Value` is not a closure /// Unwraps the inner [`Closure`] value or returns an error if this `Value` is not a closure
pub fn into_closure(self) -> Result<Closure, ShellError> { pub fn into_closure(self) -> Result<Closure, ShellError> {
if let Value::Closure { val, .. } = self { if let Value::Closure { val, .. } = self {
Ok(val) Ok(*val)
} else { } else {
self.cant_convert_to("closure") self.cant_convert_to("closure")
} }
@ -1012,7 +1012,7 @@ impl Value {
}); });
} }
} }
Value::Range { val, .. } => { Value::Range { ref val, .. } => {
if let Some(item) = if let Some(item) =
val.into_range_iter(current.span(), None).nth(*count) val.into_range_iter(current.span(), None).nth(*count)
{ {
@ -1826,7 +1826,7 @@ impl Value {
pub fn range(val: Range, span: Span) -> Value { pub fn range(val: Range, span: Span) -> Value {
Value::Range { Value::Range {
val, val: val.into(),
internal_span: span, internal_span: span,
} }
} }
@ -1862,7 +1862,7 @@ impl Value {
pub fn closure(val: Closure, span: Span) -> Value { pub fn closure(val: Closure, span: Span) -> Value {
Value::Closure { Value::Closure {
val, val: val.into(),
internal_span: span, internal_span: span,
} }
} }

View file

@ -176,7 +176,7 @@ fn value_to_string(
} }
} }
Value::Nothing { .. } => Ok("null".to_string()), Value::Nothing { .. } => Ok("null".to_string()),
Value::Range { val, .. } => match val { Value::Range { val, .. } => match **val {
Range::IntRange(range) => Ok(range.to_string()), Range::IntRange(range) => Ok(range.to_string()),
Range::FloatRange(range) => { Range::FloatRange(range) => {
let start = let start =