diff --git a/numbat/src/bytecode_interpreter.rs b/numbat/src/bytecode_interpreter.rs index 1a99a2e28..7773582cd 100644 --- a/numbat/src/bytecode_interpreter.rs +++ b/numbat/src/bytecode_interpreter.rs @@ -11,12 +11,11 @@ use crate::interpreter::{ }; use crate::name_resolution::LAST_RESULT_IDENTIFIERS; use crate::prefix::Prefix; -use crate::prefix_parser::AcceptsPrefix; use crate::pretty_print::PrettyPrint; use crate::typed_ast::{ BinaryOperator, DefineVariable, Expression, Statement, StringPart, UnaryOperator, }; -use crate::unit::{CanonicalName, Unit}; +use crate::unit::Unit; use crate::unit_registry::{UnitMetadata, UnitRegistry}; use crate::value::{FunctionReference, Value}; use crate::vm::{Constant, ExecutionContext, Op, Vm}; @@ -51,7 +50,7 @@ impl BytecodeInterpreter { fn compile_expression(&mut self, expr: &Expression) -> Result<()> { match expr { Expression::Scalar(_span, n, _type) => { - let index = self.vm.add_constant(Constant::Scalar(n.to_f64())); + let index = self.vm.add_constant(Constant::scalar_from_f64(n.to_f64())); self.vm.add_op1(Op::LoadConstant, index); } Expression::Identifier(_span, identifier, _type) => { @@ -421,13 +420,7 @@ impl BytecodeInterpreter { .map(|(name, ap)| (name.to_compact_string(), ap)) .collect(); - let constant_idx = self.vm.add_constant(Constant::Unit(Unit::new_base( - CompactString::const_new(""), - CanonicalName { - name: CompactString::const_new(""), - accepts_prefix: AcceptsPrefix::both(), - }, - ))); // TODO: dummy is just a temp. value until the SetUnitConstant op runs + let constant_idx = self.vm.add_dummy_constant(); // TODO: dummy is just a temp. value until the SetUnitConstant op runs let unit_information_idx = self.vm.add_unit_information( unit_name, Some( @@ -530,7 +523,7 @@ impl BytecodeInterpreter { pub fn get_defining_unit(&self, unit_name: &str) -> Option<&Unit> { self.unit_name_to_constant_index .get(unit_name) - .and_then(|idx| self.vm.constants.get(*idx as usize)) + .and_then(|idx| self.vm.constants.get_index(*idx as usize)) .and_then(|constant| match constant { Constant::Unit(u) => Some(u), _ => None, diff --git a/numbat/src/number.rs b/numbat/src/number.rs index 47286396f..d6287ee3b 100644 --- a/numbat/src/number.rs +++ b/numbat/src/number.rs @@ -1,22 +1,46 @@ -use std::fmt::Display; +use std::fmt::{self, Debug, Display}; use compact_str::{format_compact, CompactString, ToCompactString}; use num_traits::{Pow, ToPrimitive}; use pretty_dtoa::FmtFloatConfig; -#[derive(Debug, Clone, Copy, PartialEq, PartialOrd)] // TODO: we probably want to remove 'Copy' once we move to a more sophisticated numerical type -pub struct Number(pub f64); +/// A type that acts like an `f64` +/// +/// To make this type `Hash` and `Eq`, we actually store a `u64`. To convert to and from `f64` +/// (which is the actual value we care about), we use the [`f64::from_bits`] and +/// [`f64::to_bits`] functions. +/// +/// Note that we can't derive PartialEq because some f64 with different bits represent +/// the same value (e.g. `0.0` and `-0.0`). +#[derive(Clone, Copy, Eq)] // TODO: we probably want to remove 'Copy' once we move to a more sophisticated numerical type +pub struct Number(u64); + +impl Debug for Number { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("Number").field(&self.to_f64()).finish() + } +} -impl Eq for Number {} +impl PartialEq for Number { + fn eq(&self, other: &Self) -> bool { + self.to_f64() == other.to_f64() + } +} + +impl PartialOrd for Number { + fn partial_cmp(&self, other: &Self) -> Option { + self.to_f64().partial_cmp(&other.to_f64()) + } +} impl Number { pub fn from_f64(n: f64) -> Self { - Number(n) + Number(n.to_bits()) } pub fn to_f64(self) -> f64 { let Number(n) = self; - n + f64::from_bits(n) } pub fn pow(self, other: &Number) -> Self { @@ -28,7 +52,7 @@ impl Number { } fn is_integer(self) -> bool { - self.0.trunc() == self.0 + self.to_f64().trunc() == self.to_f64() } /// Pretty prints with default options @@ -40,7 +64,7 @@ impl Number { /// If options is None, default options will be used. /// If options is not None, float-based format handling is used and integer-based format handling is skipped. pub fn pretty_print_with_options(self, options: Option) -> CompactString { - let number = self.0; + let number = self.to_f64(); // 64-bit floats can accurately represent integers up to 2^52 [1], // which is approximately 4.5 × 10^15. @@ -48,11 +72,11 @@ impl Number { // [1] https://stackoverflow.com/a/43656339 // // Skip special format handling for integers if options is not None. - if options.is_none() && self.is_integer() && self.0.abs() < 1e15 { + if options.is_none() && self.is_integer() && self.to_f64().abs() < 1e15 { use num_format::{CustomFormat, Grouping, ToFormattedString}; let format = CustomFormat::builder() - .grouping(if self.0.abs() >= 100_000.0 { + .grouping(if self.to_f64().abs() >= 100_000.0 { Grouping::Standard } else { Grouping::Posix @@ -108,7 +132,7 @@ impl Number { impl Display for Number { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.to_f64().fmt(f) + Display::fmt(&self.to_f64(), f) } } @@ -116,7 +140,7 @@ impl std::ops::Add for Number { type Output = Number; fn add(self, rhs: Self) -> Self::Output { - Number(self.0 + rhs.0) + Number::from_f64(self.to_f64() + rhs.to_f64()) } } @@ -124,7 +148,7 @@ impl std::ops::Sub for Number { type Output = Number; fn sub(self, rhs: Self) -> Self::Output { - Number(self.0 - rhs.0) + Number::from_f64(self.to_f64() - rhs.to_f64()) } } @@ -132,7 +156,7 @@ impl std::ops::Mul for Number { type Output = Number; fn mul(self, rhs: Self) -> Self::Output { - Number(self.0 * rhs.0) + Number::from_f64(self.to_f64() * rhs.to_f64()) } } @@ -140,7 +164,7 @@ impl std::ops::Div for Number { type Output = Number; fn div(self, rhs: Self) -> Self::Output { - Number(self.0 / rhs.0) + Number::from_f64(self.to_f64() / rhs.to_f64()) } } @@ -148,7 +172,7 @@ impl std::ops::Neg for Number { type Output = Number; fn neg(self) -> Self::Output { - Number(-self.0) + Number::from_f64(-self.to_f64()) } } diff --git a/numbat/src/prefix_parser.rs b/numbat/src/prefix_parser.rs index 689c185c3..1a3b5f7af 100644 --- a/numbat/src/prefix_parser.rs +++ b/numbat/src/prefix_parser.rs @@ -17,7 +17,7 @@ pub enum PrefixParserResult<'a> { type Result = std::result::Result; -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct AcceptsPrefix { pub short: bool, pub long: bool, diff --git a/numbat/src/product.rs b/numbat/src/product.rs index 7050f3a29..617bef373 100644 --- a/numbat/src/product.rs +++ b/numbat/src/product.rs @@ -1,3 +1,4 @@ +use std::hash::Hash; use std::ops::{Div, Mul}; use crate::arithmetic::{Exponent, Power}; @@ -235,6 +236,14 @@ impl P } } +impl Hash + for Product +{ + fn hash(&self, state: &mut H) { + self.canonicalized().factors.hash(state); + } +} + impl Eq for Product { diff --git a/numbat/src/unit.rs b/numbat/src/unit.rs index e586d8d40..451c7d391 100644 --- a/numbat/src/unit.rs +++ b/numbat/src/unit.rs @@ -10,19 +10,20 @@ use crate::{ prefix::Prefix, prefix_parser::AcceptsPrefix, product::{Canonicalize, Product}, + vm::HashableNumber, }; pub type ConversionFactor = Number; /// A unit can either be a base/fundamental unit or it is derived from another unit. /// In the latter case, a conversion factor to the defining unit has to be specified. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum UnitKind { Base, - Derived(ConversionFactor, Unit), + Derived(HashableNumber, Unit), } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct CanonicalName { pub name: CompactString, pub accepts_prefix: AcceptsPrefix, @@ -37,7 +38,7 @@ impl CanonicalName { } } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct UnitIdentifier { pub name: CompactString, pub canonical_name: CanonicalName, @@ -66,7 +67,7 @@ impl UnitIdentifier { Number::from_f64(1.0), ), UnitKind::Derived(factor, defining_unit) => { - BaseUnitAndFactor(defining_unit.clone(), *factor) + BaseUnitAndFactor(defining_unit.clone(), *factor.number()) } } } @@ -98,7 +99,7 @@ impl UnitIdentifier { ) .product(); - BaseUnitAndFactor(base_unit, *factor * defining_unit_factor) + BaseUnitAndFactor(base_unit, *factor.number() * defining_unit_factor) } } } @@ -166,7 +167,7 @@ impl Ord for UnitIdentifier { } } -#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct UnitFactor { pub unit_id: UnitIdentifier, pub prefix: Prefix, @@ -255,7 +256,7 @@ impl Unit { unit_id: UnitIdentifier { name, canonical_name, - kind: UnitKind::Derived(factor, base_unit), + kind: UnitKind::Derived(factor.into(), base_unit), }, exponent: Rational::from_integer(1), }) diff --git a/numbat/src/value.rs b/numbat/src/value.rs index 6d3b32d7e..f78d5cbad 100644 --- a/numbat/src/value.rs +++ b/numbat/src/value.rs @@ -8,7 +8,7 @@ use crate::{ list::NumbatList, pretty_print::PrettyPrint, quantity::Quantity, typed_ast::StructInfo, }; -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum FunctionReference { Foreign(CompactString), Normal(CompactString), diff --git a/numbat/src/vm.rs b/numbat/src/vm.rs index f0e2a285e..b7dc465ae 100644 --- a/numbat/src/vm.rs +++ b/numbat/src/vm.rs @@ -3,7 +3,7 @@ use std::fmt::Display; use std::sync::Arc; use compact_str::{CompactString, ToCompactString}; -use indexmap::IndexMap; +use indexmap::{IndexMap, IndexSet}; use num_traits::ToPrimitive; use crate::list::NumbatList; @@ -206,25 +206,76 @@ impl Op { } } -#[derive(Clone, Debug)] +/// The value stored in a `Constant::Dummy`. Each dummy needs to have a distinct one of +/// these so that they count as separate keys for a Map. +/// +/// The wrapped value is private (and in particular, we don't just store a `usize` in +/// `Constant::Dummy`) so that it's impossible to construct a dummy ad-hoc; you must +/// instead go through `Vm::add_dummy_constant`, which ensures a unique value is +/// produced. (The implementation currently uses the current length of the vm’s +/// `constants`, which causes each dummy to store its own index in the IndexMap, which +/// will obviously cause them to be distinct.) +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +pub struct ConstantDummyValue(usize); + +/// A Number that is hash-friendly: in addition to all values hashing equal when they +/// compare equal, all NaNs also compare equal to themselves (and hash the same). +#[derive(Clone, Debug, Eq)] +pub struct HashableNumber(Number); + +impl HashableNumber { + pub fn number(&self) -> &Number { + &self.0 + } +} + +impl From for HashableNumber { + fn from(n: Number) -> Self { + HashableNumber(n) + } +} + +impl PartialEq for HashableNumber { + fn eq(&self, other: &Self) -> bool { + let lhs = self.number().to_f64(); + let rhs = other.number().to_f64(); + lhs == rhs || lhs.is_nan() && rhs.is_nan() + } +} + +impl std::hash::Hash for HashableNumber { + fn hash(&self, state: &mut H) { + self.0.to_f64().to_bits().hash(state); + } +} + +#[derive(Clone, Debug, PartialEq, Eq, Hash)] pub enum Constant { - Scalar(f64), + Scalar(HashableNumber), Unit(Unit), Boolean(bool), String(CompactString), FunctionReference(FunctionReference), FormatSpecifiers(Option), + Dummy(ConstantDummyValue), } impl Constant { + pub(crate) fn scalar_from_f64(n: f64) -> Self { + Constant::Scalar(HashableNumber(Number::from_f64(n))) + } + fn to_value(&self) -> Value { match self { - Constant::Scalar(n) => Value::Quantity(Quantity::from_scalar(*n)), + Constant::Scalar(HashableNumber(n)) => { + Value::Quantity(Quantity::from_scalar(n.to_f64())) + } Constant::Unit(u) => Value::Quantity(Quantity::from_unit(u.clone())), Constant::Boolean(b) => Value::Boolean(*b), Constant::String(s) => Value::String(s.clone()), Constant::FunctionReference(inner) => Value::FunctionReference(inner.clone()), Constant::FormatSpecifiers(s) => Value::FormatSpecifiers(s.clone()), + Constant::Dummy(_) => unreachable!("unexpectedly found dummy constant"), } } } @@ -232,12 +283,13 @@ impl Constant { impl Display for Constant { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Constant::Scalar(n) => write!(f, "{n}"), + Constant::Scalar(HashableNumber(n)) => write!(f, "{n}"), Constant::Unit(unit) => write!(f, "{unit}"), Constant::Boolean(val) => write!(f, "{val}"), Constant::String(val) => write!(f, "\"{val}\""), Constant::FunctionReference(inner) => write!(f, "{inner}"), Constant::FormatSpecifiers(_) => write!(f, ""), + Constant::Dummy(ConstantDummyValue(n)) => write!(f, ""), } } } @@ -281,7 +333,7 @@ pub struct Vm { current_chunk_index: usize, /// Constants are numbers like '1.4' or a [Unit] like 'meter'. - pub constants: Vec, + pub constants: IndexSet, /// struct metadata, used so we can display struct fields at runtime struct_infos: IndexMap>, @@ -325,13 +377,13 @@ impl Vm { Self { bytecode: vec![("
".into(), vec![])], current_chunk_index: 0, - constants: vec![], + constants: IndexSet::new(), struct_infos: IndexMap::new(), prefixes: vec![], strings: vec![], unit_information: vec![], last_result: None, - ffi_callables: ffi::procedures().iter().map(|(_, ff)| ff).collect(), + ffi_callables: ffi::procedures().values().collect(), procedure_arg_spans: vec![], frames: vec![CallFrame::root()], stack: vec![], @@ -391,12 +443,27 @@ impl Vm { chunk[offset + 1] = ((arg >> 8) & 0xff) as u8; } - pub fn add_constant(&mut self, constant: Constant) -> u16 { - self.constants.push(constant); + fn add_constant_impl(&mut self, constant: Constant) -> u16 { + self.constants.insert(constant); assert!(self.constants.len() <= u16::MAX as usize); (self.constants.len() - 1) as u16 // TODO: this can overflow, see above } + pub fn add_constant(&mut self, constant: Constant) -> u16 { + if let Some(idx) = self.constants.get_index_of(&constant) { + return idx as u16; + } + + self.add_constant_impl(constant) + } + + pub(crate) fn add_dummy_constant(&mut self) -> u16 { + // uniqueness is guaranteed because each dummy points to its current index in + // the IndexMap + let constant = Constant::Dummy(ConstantDummyValue(self.constants.len())); + self.add_constant_impl(constant) + } + pub fn add_struct_info(&mut self, struct_info: &StructInfo) -> usize { let e = self.struct_infos.entry(struct_info.name.clone()); let idx = e.index(); @@ -664,12 +731,23 @@ impl Vm { ) .map_err(RuntimeError::UnitRegistryError)?; - self.constants[constant_idx as usize] = Constant::Unit(Unit::new_derived( + // 1. swap-remove the dummy value, leaving an arbitrary element in + // its place (which is now at the wrong spot) + // 2. insert the new correct element (which is now at the end of the + // list, also at the wrong spot) + // 3. swap the two + let removed = self.constants.swap_remove_index(constant_idx as usize); + debug_assert!(matches!(removed, Some(Constant::Dummy(_)))); + + // index should be `self.constants.len()-1`, but just in case we try + // to insert the same derived unit twice, this will handle that + let (index, _) = self.constants.insert_full(Constant::Unit(Unit::new_derived( unit_information.0.to_compact_string(), unit_information.2.canonical_name.clone(), *conversion_value.unsafe_value(), defining_unit.clone(), - )); + ))); + self.constants.swap_indices(constant_idx as usize, index); } Op::GetLocal => { let slot_idx = self.read_u16() as usize; @@ -1098,8 +1176,8 @@ impl Vm { #[test] fn vm_basic() { let mut vm = Vm::new(); - vm.add_constant(Constant::Scalar(42.0)); - vm.add_constant(Constant::Scalar(1.0)); + vm.add_constant(Constant::scalar_from_f64(42.0)); + vm.add_constant(Constant::scalar_from_f64(1.0)); vm.add_op1(Op::LoadConstant, 0); vm.add_op1(Op::LoadConstant, 1); @@ -1116,3 +1194,17 @@ fn vm_basic() { InterpreterResult::Value(Value::Quantity(Quantity::from_scalar(42.0 + 1.0))) ); } + +#[test] +fn vm_constant_dedupe_hashability() { + let mut vm = Vm::new(); + + vm.add_constant(Constant::scalar_from_f64(f64::NAN)); + vm.add_constant(Constant::scalar_from_f64(f64::NAN)); + vm.add_constant(Constant::scalar_from_f64(-f64::NAN)); + + vm.add_constant(Constant::scalar_from_f64(0.0)); + vm.add_constant(Constant::scalar_from_f64(-0.0)); + + assert_eq!(vm.constants.len(), 2); +}