Skip to content

Commit a89a6a1

Browse files
committed
refactor: shrinks Value from 96 to 24 bytes
While investigating Sprocket's baseline memory usage, certain workloads—particularly those involving deeply nested compound types like `Map[String, Map[String, ...]]`—scaled poorly. The root cause was the `Value` enum in `wdl-engine`, which occupied 96 bytes per element due to large inline types (`Type` at 56 bytes, `TaskPostEvaluationValue` at 96 bytes) inflating the enum even though the common case (`PrimitiveValue`) is only 16 bytes. This refactor pushes `Arc` wrapping down into the types that inflate `Value`. Each compound value type (`Array`, `Map`, `Pair`, `Struct`, `EnumVariant`) and each large hidden value type (`TaskPreEvaluation`, `TaskPostEvaluation`) becomes a thin wrapper around `Arc<Inner>`, where the inner struct owns all fields directly—eliminating nested `Arc` layering. Two new wrapper types (`NoneValue` and `TypeNameRefValue`) handle variants that previously stored a bare `Type`. `CallValue` wraps its `CallType` field behind `Arc` as well. The tradeoff is an additional pointer indirection when accessing fields of compound and hidden values, since their data now lives behind `Arc` on the heap rather than inline in the enum. In practice, this cost is negligible: these types were already heap-allocated internally (e.g., `Array` stored an `Arc<Vec<Value>>`), so the refactor consolidates rather than adds indirection. Clone remains cheap—a refcount bump on one `Arc` rather than multiple. A const assertion prevents `Value` from exceeding 24 bytes in the future. The `as_map` standard library function was also updated to use the `IndexMap` entry API, eliminating a redundant key clone. See PR for benchmark results. Relates to #541.
1 parent 1079455 commit a89a6a1

10 files changed

Lines changed: 380 additions & 323 deletions

File tree

crates/wdl-engine/src/cache/hash.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ use crate::OutputValue;
2323
use crate::Pair;
2424
use crate::PrimitiveValue;
2525
use crate::Struct;
26+
use crate::NoneValue;
2627
use crate::Value;
2728
use crate::digest::Digest;
2829

@@ -209,7 +210,7 @@ impl Hashable for Option<PrimitiveValue> {
209210
None => {
210211
// A `None` for an optional primitive value (used in map keys) represents a WDL
211212
// `None` value, so hash it as one
212-
Value::None(Type::None).hash(hasher)
213+
Value::None(NoneValue::new(Type::None)).hash(hasher)
213214
}
214215
}
215216
}

crates/wdl-engine/src/eval/v1/expr.rs

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
use std::cmp::Ordering;
44
use std::fmt::Write;
55
use std::iter::once;
6-
use std::sync::Arc;
76

87
use futures::FutureExt;
98
use futures::future::BoxFuture;
@@ -768,8 +767,8 @@ impl<C: EvaluationContext> ExprEvaluator<C> {
768767
}
769768
}
770769

771-
let name = struct_ty.name().clone();
772-
Ok(Struct::new_unchecked(ty, name, Arc::new(members)).into())
770+
let name = struct_ty.name().as_str().to_string();
771+
Ok(Struct::new_unchecked(ty, name, members).into())
773772
}
774773

775774
/// Evaluates a literal hints expression.
@@ -1320,7 +1319,7 @@ impl<C: EvaluationContext> ExprEvaluator<C> {
13201319
// Evaluate the argument expressions
13211320
let mut count = 0;
13221321
let mut types = [const { Type::Union }; MAX_PARAMETERS];
1323-
let mut arguments = [const { CallArgument::none() }; MAX_PARAMETERS];
1322+
let mut arguments: [CallArgument; MAX_PARAMETERS] = std::array::from_fn(|_| CallArgument::none());
13241323
for arg in expr.arguments() {
13251324
if count < MAX_PARAMETERS {
13261325
let v = self.evaluate_expr(&arg).await?;
@@ -1506,16 +1505,17 @@ impl<C: EvaluationContext> ExprEvaluator<C> {
15061505
Some(value) => Ok(value.clone()),
15071506
None => Err(unknown_call_io(call.ty(), &name, Io::Output)),
15081507
},
1509-
Value::TypeNameRef(ty) => {
1510-
if let Some(ty) = ty.as_enum() {
1508+
Value::TypeNameRef(v) => {
1509+
let ty = v.ty();
1510+
if let Some(enum_ty) = ty.as_enum() {
15111511
let value = self
15121512
.context()
1513-
.enum_variant_value(ty.name(), name.text())
1514-
.map_err(|_| unknown_enum_variant_access(ty.name(), &name))?;
1515-
let variant = EnumVariant::new(ty.clone(), name.text(), value);
1513+
.enum_variant_value(enum_ty.name(), name.text())
1514+
.map_err(|_| unknown_enum_variant_access(enum_ty.name(), &name))?;
1515+
let variant = EnumVariant::new(enum_ty.clone(), name.text(), value);
15161516
Ok(Value::Compound(CompoundValue::EnumVariant(variant)))
15171517
} else {
1518-
Err(cannot_access(&ty, target.span()))
1518+
Err(cannot_access(ty, target.span()))
15191519
}
15201520
}
15211521
value => Err(cannot_access(&value.ty(), target.span())),
@@ -1701,6 +1701,7 @@ pub(crate) mod test {
17011701
use std::collections::HashMap;
17021702
use std::fs;
17031703
use std::path::Path;
1704+
use std::sync::Arc;
17041705

17051706
use anyhow::Result;
17061707
use pretty_assertions::assert_eq;
@@ -1716,6 +1717,7 @@ pub(crate) mod test {
17161717

17171718
use super::*;
17181719
use crate::EvaluationPath;
1720+
use crate::TypeNameRefValue;
17191721
use crate::eval::Scope;
17201722
use crate::eval::ScopeRef;
17211723
use crate::http::Location;
@@ -1871,12 +1873,12 @@ pub(crate) mod test {
18711873

18721874
// If the name is a reference to a struct, return it as a [`Type::TypeNameRef`].
18731875
if let Some(ty) = self.env.structs.get(name) {
1874-
return Ok(Value::TypeNameRef(ty.clone()));
1876+
return Ok(Value::TypeNameRef(TypeNameRefValue::new(ty.clone())));
18751877
}
18761878

18771879
// If the name is a reference to an enum, return it as a [`Type::TypeNameRef`].
18781880
if let Some(ty) = self.env.enums.get(name) {
1879-
return Ok(Value::TypeNameRef(ty.clone()));
1881+
return Ok(Value::TypeNameRef(TypeNameRefValue::new(ty.clone())));
18801882
}
18811883

18821884
Err(unknown_name(name, span))

crates/wdl-engine/src/eval/v1/task.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ use crate::TaskInputs;
8080
use crate::TaskPostEvaluationData;
8181
use crate::TaskPostEvaluationValue;
8282
use crate::TaskPreEvaluationValue;
83+
use crate::TypeNameRefValue;
8384
use crate::Value;
8485
use crate::backend::ExecuteTaskRequest;
8586
use crate::backend::TaskExecutionConstraints;
@@ -257,7 +258,7 @@ impl EvaluationContext for TaskEvaluationContext<'_, '_> {
257258
}
258259

259260
if let Some(ty) = self.state.document.get_custom_type(name) {
260-
return Ok(Value::TypeNameRef(ty));
261+
return Ok(Value::TypeNameRef(TypeNameRefValue::new(ty)));
261262
}
262263

263264
Err(unknown_name(name, span))

crates/wdl-engine/src/eval/v1/workflow.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ use crate::EvaluationPath;
6868
use crate::EvaluationResult;
6969
use crate::Inputs;
7070
use crate::Outputs;
71+
use crate::TypeNameRefValue;
7172
use crate::Value;
7273
use crate::WorkflowInputs;
7374
use crate::diagnostics::decl_evaluation_failed;
@@ -164,7 +165,7 @@ impl EvaluationContext for WorkflowEvaluationContext<'_, '_> {
164165
}
165166

166167
if let Some(ty) = self.state.document.get_custom_type(name) {
167-
return Ok(Value::TypeNameRef(ty));
168+
return Ok(Value::TypeNameRef(TypeNameRefValue::new(ty)));
168169
}
169170

170171
Err(unknown_name(name, span))

crates/wdl-engine/src/stdlib.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use crate::EvaluationContext;
2020
use crate::EvaluationPath;
2121
use crate::HostPath;
2222
use crate::PrimitiveValue;
23+
use crate::NoneValue;
2324
use crate::Value;
2425
use crate::diagnostics::function_call_failed;
2526
use crate::http::Location;
@@ -178,9 +179,9 @@ impl CallArgument {
178179
}
179180

180181
/// Constructs a `None` call argument.
181-
pub const fn none() -> Self {
182+
pub fn none() -> Self {
182183
Self {
183-
value: Value::None(Type::None),
184+
value: Value::None(NoneValue::new(Type::None)),
184185
span: Span::new(0, 0),
185186
}
186187
}

crates/wdl-engine/src/stdlib/as_map.rs

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,17 @@ fn as_map(context: CallContext<'_>) -> Result<Value, Diagnostic> {
5959
_ => unreachable!("expected a primitive type for the left value"),
6060
};
6161

62-
if elements.insert(key.clone(), pair.right().clone()).is_some() {
63-
return Err(function_call_failed(
64-
FUNCTION_NAME,
65-
DuplicateKeyError(key),
66-
context.arguments[0].span,
67-
));
62+
match elements.entry(key) {
63+
indexmap::map::Entry::Occupied(e) => {
64+
return Err(function_call_failed(
65+
FUNCTION_NAME,
66+
DuplicateKeyError(e.key().clone()),
67+
context.arguments[0].span,
68+
));
69+
}
70+
indexmap::map::Entry::Vacant(e) => {
71+
e.insert(pair.right().clone());
72+
}
6873
}
6974
}
7075

crates/wdl-engine/src/stdlib/contains_key.rs

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ use super::Function;
1313
use super::Signature;
1414
use crate::CompoundValue;
1515
use crate::PrimitiveValue;
16-
use crate::Struct;
1716
use crate::Value;
1817

1918
/// Given a Map and a key, tests whether the collection contains an entry with
@@ -82,9 +81,7 @@ fn contains_key_recursive(context: CallContext<'_>) -> Result<Value, Diagnostic>
8281
map.get(&PrimitiveValue::String(key.clone())).cloned()
8382
}
8483
Value::Compound(CompoundValue::Object(object)) => object.get(key.as_str()).cloned(),
85-
Value::Compound(CompoundValue::Struct(Struct { members, .. })) => {
86-
members.get(key.as_str()).cloned()
87-
}
84+
Value::Compound(CompoundValue::Struct(s)) => s.get(key.as_str()).cloned(),
8885
_ => None,
8986
}
9087
}

crates/wdl-engine/src/stdlib/find.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ fn find(context: CallContext<'_>) -> Result<Value, Diagnostic> {
3838

3939
match regex.find(input.as_str()) {
4040
Some(m) => Ok(PrimitiveValue::new_string(m.as_str()).into()),
41-
None => Ok(Value::None(Type::from(PrimitiveType::String).optional())),
41+
None => Ok(Value::new_none(Type::from(PrimitiveType::String).optional())),
4242
}
4343
}
4444

crates/wdl-engine/src/stdlib/keys.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ use super::Signature;
99
use crate::Array;
1010
use crate::CompoundValue;
1111
use crate::PrimitiveValue;
12-
use crate::Struct;
1312
use crate::Value;
1413

1514
/// Given a key-value type collection (Map, Struct, or Object), returns an Array
@@ -37,10 +36,9 @@ fn keys(context: CallContext<'_>) -> Result<Value, Diagnostic> {
3736
.keys()
3837
.map(|k| PrimitiveValue::new_string(k).into())
3938
.collect(),
40-
Value::Compound(CompoundValue::Struct(Struct { members, .. })) => members
41-
.keys()
42-
.map(|k| PrimitiveValue::new_string(k).into())
43-
.collect(),
39+
Value::Compound(CompoundValue::Struct(s)) => {
40+
s.keys().map(|k| PrimitiveValue::new_string(k).into()).collect()
41+
}
4442
_ => unreachable!("expected a map, object, or struct"),
4543
};
4644

0 commit comments

Comments
 (0)