Skip to content

refactor: shrinks Value from 96 to 24 bytes #727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
claymcleod wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: shrinks Value from 96 to 24 bytes #727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions crates/wdl-engine/src/cache/hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use std::path::PathBuf;
use blake3::Hasher;
use num_enum::IntoPrimitive;
use url::Url;
use wdl_analysis::types::Type;

use crate::Array;
use crate::CompoundValue;
Expand All @@ -18,6 +17,7 @@ use crate::HiddenValue;
use crate::HintsValue;
use crate::InputValue;
use crate::Map;
use crate::NoneValue;
use crate::Object;
use crate::OutputValue;
use crate::Pair;
Expand Down Expand Up @@ -209,7 +209,7 @@ impl Hashable for Option<PrimitiveValue> {
None => {
// A `None` for an optional primitive value (used in map keys) represents a WDL
// `None` value, so hash it as one
Value::None(Type::None).hash(hasher)
Value::None(NoneValue::untyped()).hash(hasher)
}
}
}
Expand Down Expand Up @@ -376,6 +376,7 @@ mod test {
use wdl_analysis::types::PairType;
use wdl_analysis::types::PrimitiveType;
use wdl_analysis::types::StructType;
use wdl_analysis::types::Type;

use super::*;

Expand Down
25 changes: 14 additions & 11 deletions crates/wdl-engine/src/eval/v1/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
use std::cmp::Ordering;
use std::fmt::Write;
use std::iter::once;
use std::sync::Arc;

use futures::FutureExt;
use futures::future::BoxFuture;
Expand Down Expand Up @@ -769,7 +768,7 @@ impl<C: EvaluationContext> ExprEvaluator<C> {
}

let name = struct_ty.name().clone();
Ok(Struct::new_unchecked(ty, name, Arc::new(members)).into())
Ok(Struct::new_unchecked(ty, name, members).into())
}

/// Evaluates a literal hints expression.
Expand Down Expand Up @@ -1320,7 +1319,8 @@ impl<C: EvaluationContext> ExprEvaluator<C> {
// Evaluate the argument expressions
let mut count = 0;
let mut types = [const { Type::Union }; MAX_PARAMETERS];
let mut arguments = [const { CallArgument::none() }; MAX_PARAMETERS];
let mut arguments: [CallArgument; MAX_PARAMETERS] =
std::array::from_fn(|_| CallArgument::none());
Comment on lines +1322 to +1323
Copy link
Contributor

@peterhuene peterhuene Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could just be a call to repeat as CallArgument should probably be Clone:

Suggested change
let mut arguments: [CallArgument; MAX_PARAMETERS] =
std::array::from_fn(|_| CallArgument::none());
let mut arguments: [CallArgument; MAX_PARAMETERS] =
std::array::repeat(CallArgument::none()));

for arg in expr.arguments() {
if count < MAX_PARAMETERS {
let v = self.evaluate_expr(&arg).await?;
Expand Down Expand Up @@ -1506,16 +1506,17 @@ impl<C: EvaluationContext> ExprEvaluator<C> {
Some(value) => Ok(value.clone()),
None => Err(unknown_call_io(call.ty(), &name, Io::Output)),
},
Value::TypeNameRef(ty) => {
if let Some(ty) = ty.as_enum() {
Value::TypeNameRef(v) => {
let ty = v.ty();
if let Some(enum_ty) = ty.as_enum() {
let value = self
.context()
.enum_variant_value(ty.name(), name.text())
.map_err(|_| unknown_enum_variant_access(ty.name(), &name))?;
let variant = EnumVariant::new(ty.clone(), name.text(), value);
.enum_variant_value(enum_ty.name(), name.text())
.map_err(|_| unknown_enum_variant_access(enum_ty.name(), &name))?;
let variant = EnumVariant::new(enum_ty.clone(), name.text(), value);
Ok(Value::Compound(CompoundValue::EnumVariant(variant)))
} else {
Err(cannot_access(&ty, target.span()))
Err(cannot_access(ty, target.span()))
}
}
value => Err(cannot_access(&value.ty(), target.span())),
Expand Down Expand Up @@ -1701,6 +1702,7 @@ pub(crate) mod test {
use std::collections::HashMap;
use std::fs;
use std::path::Path;
use std::sync::Arc;

use anyhow::Result;
use pretty_assertions::assert_eq;
Expand All @@ -1716,6 +1718,7 @@ pub(crate) mod test {

use super::*;
use crate::EvaluationPath;
use crate::TypeNameRefValue;
use crate::eval::Scope;
use crate::eval::ScopeRef;
use crate::http::Location;
Expand Down Expand Up @@ -1871,12 +1874,12 @@ pub(crate) mod test {

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

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

Err(unknown_name(name, span))
Expand Down
3 changes: 2 additions & 1 deletion crates/wdl-engine/src/eval/v1/task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ use crate::TaskInputs;
use crate::TaskPostEvaluationData;
use crate::TaskPostEvaluationValue;
use crate::TaskPreEvaluationValue;
use crate::TypeNameRefValue;
use crate::Value;
use crate::backend::ExecuteTaskRequest;
use crate::backend::TaskExecutionConstraints;
Expand Down Expand Up @@ -257,7 +258,7 @@ impl EvaluationContext for TaskEvaluationContext<'_, '_> {
}

if let Some(ty) = self.state.document.get_custom_type(name) {
return Ok(Value::TypeNameRef(ty));
return Ok(Value::TypeNameRef(TypeNameRefValue::new(ty)));
}

Err(unknown_name(name, span))
Expand Down
3 changes: 2 additions & 1 deletion crates/wdl-engine/src/eval/v1/workflow.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ use crate::EvaluationPath;
use crate::EvaluationResult;
use crate::Inputs;
use crate::Outputs;
use crate::TypeNameRefValue;
use crate::Value;
use crate::WorkflowInputs;
use crate::diagnostics::decl_evaluation_failed;
Expand Down Expand Up @@ -164,7 +165,7 @@ impl EvaluationContext for WorkflowEvaluationContext<'_, '_> {
}

if let Some(ty) = self.state.document.get_custom_type(name) {
return Ok(Value::TypeNameRef(ty));
return Ok(Value::TypeNameRef(TypeNameRefValue::new(ty)));
}

Err(unknown_name(name, span))
Expand Down
5 changes: 3 additions & 2 deletions crates/wdl-engine/src/stdlib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::Coercible;
use crate::EvaluationContext;
use crate::EvaluationPath;
use crate::HostPath;
use crate::NoneValue;
use crate::PrimitiveValue;
use crate::Value;
use crate::diagnostics::function_call_failed;
Expand Down Expand Up @@ -178,9 +179,9 @@ impl CallArgument {
}

/// Constructs a `None` call argument.
pub const fn none() -> Self {
pub fn none() -> Self {
Self {
value: Value::None(Type::None),
value: Value::None(NoneValue::untyped()),
span: Span::new(0, 0),
}
}
Expand Down
17 changes: 11 additions & 6 deletions crates/wdl-engine/src/stdlib/as_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,17 @@ fn as_map(context: CallContext<'_>) -> Result<Value, Diagnostic> {
_ => unreachable!("expected a primitive type for the left value"),
};

if elements.insert(key.clone(), pair.right().clone()).is_some() {
return Err(function_call_failed(
FUNCTION_NAME,
DuplicateKeyError(key),
context.arguments[0].span,
));
match elements.entry(key) {
indexmap::map::Entry::Occupied(e) => {
return Err(function_call_failed(
FUNCTION_NAME,
DuplicateKeyError(e.key().clone()),
context.arguments[0].span,
));
}
indexmap::map::Entry::Vacant(e) => {
Copy link
Member Author

@claymcleod claymcleod Mar 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a correctness sidequest rather than a performance improvement. In the case where the key is not in the map, we can avoid the preemptive clone of the key.

e.insert(pair.right().clone());
}
}
}

Expand Down
5 changes: 1 addition & 4 deletions crates/wdl-engine/src/stdlib/contains_key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use super::Function;
use super::Signature;
use crate::CompoundValue;
use crate::PrimitiveValue;
use crate::Struct;
use crate::Value;

/// Given a Map and a key, tests whether the collection contains an entry with
Expand Down Expand Up @@ -82,9 +81,7 @@ fn contains_key_recursive(context: CallContext<'_>) -> Result<Value, Diagnostic>
map.get(&PrimitiveValue::String(key.clone())).cloned()
}
Value::Compound(CompoundValue::Object(object)) => object.get(key.as_str()).cloned(),
Value::Compound(CompoundValue::Struct(Struct { members, .. })) => {
members.get(key.as_str()).cloned()
}
Value::Compound(CompoundValue::Struct(s)) => s.get(key.as_str()).cloned(),
_ => None,
}
}
Expand Down
4 changes: 3 additions & 1 deletion crates/wdl-engine/src/stdlib/find.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ fn find(context: CallContext<'_>) -> Result<Value, Diagnostic> {

match regex.find(input.as_str()) {
Some(m) => Ok(PrimitiveValue::new_string(m.as_str()).into()),
None => Ok(Value::None(Type::from(PrimitiveType::String).optional())),
None => Ok(Value::new_none(
Type::from(PrimitiveType::String).optional(),
)),
}
}

Expand Down
3 changes: 1 addition & 2 deletions crates/wdl-engine/src/stdlib/keys.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use super::Signature;
use crate::Array;
use crate::CompoundValue;
use crate::PrimitiveValue;
use crate::Struct;
use crate::Value;

/// Given a key-value type collection (Map, Struct, or Object), returns an Array
Expand Down Expand Up @@ -37,7 +36,7 @@ fn keys(context: CallContext<'_>) -> Result<Value, Diagnostic> {
.keys()
.map(|k| PrimitiveValue::new_string(k).into())
.collect(),
Value::Compound(CompoundValue::Struct(Struct { members, .. })) => members
Value::Compound(CompoundValue::Struct(s)) => s
.keys()
.map(|k| PrimitiveValue::new_string(k).into())
.collect(),
Expand Down
Loading
Loading