From a7389c79b4697988f95be503a8a59b309d659292 Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Tue, 26 Aug 2025 14:48:44 -0700 Subject: [PATCH] Replace the String producing Display logic to using a Formatter This code heavily relied on allocating strings and vectors of strings for each object properties. This new logic eliminates the allocations entirely, saving both memory and improving performance. It also makes the logic easier to follow, which might be help with simplification later. Tests should be unchanged. --- Cargo.lock | 16 +- core/engine/src/value/display.rs | 485 +++++++++++++++---------------- 2 files changed, 240 insertions(+), 261 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c1cae5627e3..8d76c82e3db 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3044,9 +3044,9 @@ dependencies = [ [[package]] name = "phf" -version = "0.12.1" +version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "913273894cec178f401a31ec4b656318d95473527be05c0752cc41cdc32be8b7" +checksum = "c1562dc717473dbaa4c1f85a36410e03c047b2e7df7f45ee938fbef64ae7fadf" dependencies = [ "phf_macros", "phf_shared", @@ -3054,9 +3054,9 @@ dependencies = [ [[package]] name = "phf_generator" -version = "0.12.1" +version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2cbb1126afed61dd6368748dae63b1ee7dc480191c6262a3b4ff1e29d86a6c5b" +checksum = "135ace3a761e564ec88c03a77317a7c6b80bb7f7135ef2544dbe054243b89737" dependencies = [ "fastrand 2.3.0", "phf_shared", @@ -3064,9 +3064,9 @@ dependencies = [ [[package]] name = "phf_macros" -version = "0.12.1" +version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d713258393a82f091ead52047ca779d37e5766226d009de21696c4e667044368" +checksum = "812f032b54b1e759ccd5f8b6677695d5268c588701effba24601f6932f8269ef" dependencies = [ "phf_generator", "phf_shared", @@ -3077,9 +3077,9 @@ dependencies = [ [[package]] name = "phf_shared" -version = "0.12.1" +version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "06005508882fb681fd97892ecff4b7fd0fee13ef1aa569f8695dae7ab9099981" +checksum = "e57fef6bc5981e38c2ce2d63bfa546861309f875b8a75f092d1d54ae2d64f266" dependencies = [ "siphasher", ] diff --git a/core/engine/src/value/display.rs b/core/engine/src/value/display.rs index 694db619257..6a33df760e2 100644 --- a/core/engine/src/value/display.rs +++ b/core/engine/src/value/display.rs @@ -8,7 +8,8 @@ use crate::{ js_string, property::{PropertyDescriptor, PropertyKey}, }; -use std::{borrow::Cow, fmt::Write}; +use std::borrow::Cow; +use std::fmt::Write; /// This object is used for displaying a `Value`. #[derive(Debug, Clone, Copy)] @@ -29,66 +30,36 @@ impl ValueDisplay<'_> { } } -/// A helper function for printing objects -/// Can be used to print both properties and internal slots -/// All of the overloads take: -/// - The object to be printed -/// - The function with which to print -/// - The indentation for the current level (for nested objects) -/// - A `HashSet` with the addresses of the already printed objects for the current branch -/// (used to avoid infinite loops when there are cyclic deps) -fn print_obj_value_all( - obj: &JsObject, - display_fn: fn(&JsValue, &mut HashSet, usize, bool) -> String, - indent: usize, - encounters: &mut HashSet, -) -> Vec { - let mut internals = print_obj_value_internals(obj, display_fn, indent, encounters); - let mut props = print_obj_value_props(obj, display_fn, indent, encounters, true); - - props.reserve(internals.len()); - props.append(&mut internals); - props -} - fn print_obj_value_internals( + f: &mut fmt::Formatter<'_>, obj: &JsObject, - display_fn: fn(&JsValue, &mut HashSet, usize, bool) -> String, indent: usize, encounters: &mut HashSet, -) -> Vec { +) -> fmt::Result { let object = obj.borrow(); + write!(f, "{:>indent$}__proto__: ", "")?; if let Some(object) = object.prototype() { - vec![format!( - "{:>width$}{}: {}", - "", - "__proto__", - display_fn( - &object.clone().into(), - encounters, - indent.wrapping_add(4), - true - ), - width = indent, - )] + log_object_to_internal( + f, + &object.clone().into(), + encounters, + indent.wrapping_add(4), + true, + )?; } else { - vec![format!( - "{:>width$}{}: {}", - "", - "__proto__", - JsValue::null().display(), - width = indent, - )] + write!(f, "{}", JsValue::null().display())?; } + f.write_char(',')?; + f.write_char('\n') } fn print_obj_value_props( + f: &mut fmt::Formatter<'_>, obj: &JsObject, - display_fn: fn(&JsValue, &mut HashSet, usize, bool) -> String, indent: usize, encounters: &mut HashSet, print_internals: bool, -) -> Vec { +) -> fmt::Result { let mut keys: Vec<_> = obj .borrow() .properties() @@ -96,22 +67,25 @@ fn print_obj_value_props( .map(PropertyKey::from) .collect(); keys.extend(obj.borrow().properties().shape.keys()); - let mut result = Vec::default(); + + let mut first = true; for key in keys { + if first { + first = false; + } else { + f.write_char(',')?; + f.write_char('\n')?; + } let val = obj .borrow() .properties() .get(&key) .expect("There should be a value"); + + write!(f, "{:>width$}{}: ", "", key, width = indent)?; if val.is_data_descriptor() { - let v = &val.expect_value(); - result.push(format!( - "{:>width$}{}: {}", - "", - key, - display_fn(v, encounters, indent.wrapping_add(4), print_internals), - width = indent, - )); + let v = val.expect_value(); + log_object_to_internal(f, v, encounters, indent.wrapping_add(4), print_internals)?; } else { let display = match (val.set().is_some(), val.get().is_some()) { (true, true) => "Getter & Setter", @@ -119,106 +93,130 @@ fn print_obj_value_props( (false, true) => "Getter", _ => "No Getter/Setter", }; - result.push(format!("{key:>indent$}: {display}")); + write!(f, "{key}: {display}")?; } } - result + f.write_char('\n')?; + Ok(()) } -pub(crate) fn log_string_from(x: &JsValue, print_internals: bool, print_children: bool) -> String { +fn log_array_to( + f: &mut fmt::Formatter<'_>, + x: &JsObject, + print_internals: bool, + print_children: bool, +) -> fmt::Result { + let len = x + .borrow() + .properties() + .get(&js_string!("length").into()) + .expect("array object must have 'length' property") + // FIXME: handle accessor descriptors + .expect_value() + .as_number() + .map(|n| n as i32) + .unwrap_or_default(); + + if print_children { + if len == 0 { + return f.write_str("[]"); + } + + f.write_str("[ ")?; + let mut first = true; + for i in 0..len { + if first { + first = false; + } else { + f.write_str(", ")?; + } + + // Introduce recursive call to stringify any objects + // which are part of the Array + + // FIXME: handle accessor descriptors + if let Some(value) = x + .borrow() + .properties() + .get(&i.into()) + .and_then(|x| x.value().cloned()) + { + log_value_to(f, &value, print_internals, false)?; + } else { + f.write_str("")?; + } + } + write!(f, " ]") + } else { + write!(f, "Array({len})") + } +} + +pub(crate) fn log_value_to( + f: &mut fmt::Formatter<'_>, + x: &JsValue, + print_internals: bool, + print_children: bool, +) -> fmt::Result { match x.variant() { // We don't want to print private (compiler) or prototype properties JsVariant::Object(v) => { // Can use the private "type" field of an Object to match on // which type of Object it represents for special printing if let Some(s) = v.downcast_ref::() { - format!("String {{ \"{}\" }}", s.to_std_string_escaped()) + write!(f, "String {{ {:?} }}", s.to_std_string_escaped()) } else if let Some(b) = v.downcast_ref::() { - format!("Boolean {{ {b} }}") + write!(f, "Boolean {{ {b} }}") } else if let Some(r) = v.downcast_ref::() { - if r.is_sign_negative() && *r == 0.0 { - "Number { -0 }".to_string() - } else { - let mut buffer = ryu_js::Buffer::new(); - format!("Number {{ {} }}", buffer.format(*r)) - } + f.write_str("Number { ")?; + format_rational(*r, f)?; + f.write_str(" }") } else if v.is::() { - let len = v - .borrow() - .properties() - .get(&js_string!("length").into()) - .expect("array object must have 'length' property") - // FIXME: handle accessor descriptors - .expect_value() - .as_number() - .map(|n| n as i32) - .unwrap_or_default(); - - if print_children { - if len == 0 { - return String::from("[]"); - } - - let arr = (0..len) - .map(|i| { - // Introduce recursive call to stringify any objects - // which are part of the Array - - // FIXME: handle accessor descriptors - if let Some(value) = v - .borrow() - .properties() - .get(&i.into()) - .and_then(|x| x.value().cloned()) - { - log_string_from(&value, print_internals, false) - } else { - String::from("") - } - }) - .collect::>() - .join(", "); - - format!("[ {arr} ]") - } else { - format!("Array({len})") - } + log_array_to(f, &v, print_internals, print_children) } else if let Some(map) = v.downcast_ref::>() { let size = map.len(); if size == 0 { - return String::from("Map(0)"); + return f.write_str("Map(0)"); } if print_children { - let mappings = map - .iter() - .map(|(key, value)| { - let key = log_string_from(key, print_internals, false); - let value = log_string_from(value, print_internals, false); - format!("{key} → {value}") - }) - .collect::>() - .join(", "); - format!("Map {{ {mappings} }}") + f.write_str("Map { ")?; + let mut first = true; + for (key, value) in map.iter() { + if first { + first = false; + } else { + f.write_str(", ")?; + } + log_value_to(f, key, print_internals, false)?; + f.write_str(" → ")?; + log_value_to(f, value, print_internals, false)?; + } + f.write_str(" }") } else { - format!("Map({size})") + write!(f, "Map({size})") } } else if let Some(set) = v.downcast_ref::() { let size = set.len(); if size == 0 { - return String::from("Set(0)"); + return f.write_str("Set(0)"); } if print_children { - let entries = set - .iter() - .map(|value| log_string_from(value, print_internals, false)) - .collect::>() - .join(", "); - format!("Set {{ {entries} }}") + f.write_str("Set { ")?; + let mut first = true; + for value in set.iter() { + if first { + first = false; + } else { + f.write_str(", ")?; + } + log_value_to(f, value, print_internals, false)?; + } + f.write_str(" }") } else { - format!("Set({size})") + write!(f, "Set({size})") } } else if v.is::() { let name: Cow<'static, str> = v @@ -248,190 +246,171 @@ pub(crate) fn log_string_from(x: &JsValue, print_internals: bool, print_children ) }) .unwrap_or_default(); - let mut result = if name.is_empty() { - message + if name.is_empty() { + f.write_str(&message)?; } else if message.is_empty() { - name.to_string() + f.write_str(name.as_ref())?; } else { - format!("{name}: {message}") - }; + write!(f, "{name}: {message}")?; + } let data = v .downcast_ref::() .expect("already checked object type"); if let Some(position) = &data.position.0 { - write!(&mut result, "{position}").expect("should not fail"); + write!(f, "{position}")?; } - result + Ok(()) } else if let Some(promise) = v.downcast_ref::() { - format!( - "Promise {{ {} }}", - match promise.state() { - PromiseState::Pending => Cow::Borrowed(""), - PromiseState::Fulfilled(val) => Cow::Owned(val.display().to_string()), - PromiseState::Rejected(reason) => Cow::Owned(format!( - " {}", - JsError::from_opaque(reason.clone()) - )), + f.write_str("Promise { ")?; + match promise.state() { + PromiseState::Pending => f.write_str("")?, + PromiseState::Fulfilled(val) => Display::fmt(&val.display(), f)?, + PromiseState::Rejected(reason) => { + write!(f, " {}", JsError::from_opaque(reason.clone()))?; } - ) + } + f.write_str(" }") } else if v.is_constructor() { // FIXME: ArrayBuffer is not [class ArrayBuffer] but we cannot distinguish it. let name = v .get_property(&PropertyKey::from(js_string!("name"))) .and_then(|d| Some(d.value()?.as_string()?.to_std_string_escaped())); match name { - Some(name) => format!("[class {name}]"), - None => "[class (anonymous)]".to_string(), + Some(name) => write!(f, "[class {name}]"), + None => f.write_str("[class (anonymous)]"), } } else if v.is_callable() { let name = v .get_property(&PropertyKey::from(js_string!("name"))) .and_then(|d| Some(d.value()?.as_string()?.to_std_string_escaped())); match name { - Some(name) => format!("[Function: {name}]"), - None => "[Function (anonymous)]".to_string(), + Some(name) => write!(f, "[Function: {name}]"), + None => f.write_str("[Function (anonymous)]"), } } else { - x.display_obj(print_internals) + Display::fmt(&x.display_obj(print_internals), f) } } - _ => x.display().to_string(), + JsVariant::Null => write!(f, "null"), + JsVariant::Undefined => write!(f, "undefined"), + JsVariant::Boolean(v) => write!(f, "{v}"), + JsVariant::Symbol(symbol) => { + write!(f, "{}", symbol.descriptive_string().to_std_string_escaped()) + } + JsVariant::String(v) => write!(f, "{:?}", v.to_std_string_escaped()), + JsVariant::Float64(v) => format_rational(v, f), + JsVariant::Integer32(v) => write!(f, "{v}"), + JsVariant::BigInt(num) => write!(f, "{num}n"), } } -impl JsValue { - /// A helper function for specifically printing object values - #[must_use] - pub fn display_obj(&self, print_internals: bool) -> String { - // A simple helper for getting the address of a value - // TODO: Find a more general place for this, as it can be used in other situations as well - fn address_of(t: &T) -> usize { - let my_ptr: *const T = t; - my_ptr.cast::<()>() as usize +fn log_object_to_internal( + f: &mut fmt::Formatter<'_>, + data: &JsValue, + encounters: &mut HashSet, + indent: usize, + print_internals: bool, +) -> fmt::Result { + if let Some(v) = data.as_object() { + // The in-memory address of the current object + let addr = std::ptr::from_ref(v.as_ref()).addr(); + + // We need not continue if this object has already been + // printed up the current chain + if encounters.contains(&addr) { + return f.write_str("[Cycle]"); } - fn display_obj_internal( - data: &JsValue, - encounters: &mut HashSet, - indent: usize, - print_internals: bool, - ) -> String { - if let Some(v) = data.as_object() { - // The in-memory address of the current object - let addr = address_of(v.as_ref()); - - // We need not continue if this object has already been - // printed up the current chain - if encounters.contains(&addr) { - return String::from("[Cycle]"); - } + // Mark the current object as encountered + encounters.insert(addr); - // Mark the current object as encountered - encounters.insert(addr); + if v.is::() { + return log_array_to(f, &v, print_internals, false); + } - let result = if print_internals { - print_obj_value_all(&v, display_obj_internal, indent, encounters).join(",\n") - } else { - print_obj_value_props( - &v, - display_obj_internal, - indent, - encounters, - print_internals, - ) - .join(",\n") - }; - - // If the current object is referenced in a different branch, - // it will not cause an infinite printing loop, so it is safe to be printed again - encounters.remove(&addr); - - let closing_indent = String::from_utf8(vec![b' '; indent.wrapping_sub(4)]) - .expect("Could not create the closing brace's indentation string"); - - let constructor_name = get_constructor_name_not_object(&v); - let constructor_prefix = match constructor_name { - Some(name) => { - format!("{} ", name.to_std_string_lossy()) - } - None => String::new(), - }; + let constructor_name = get_constructor_name_of(&v); + if let Some(name) = constructor_name { + write!(f, "{} ", name.to_std_string_lossy())?; + } + f.write_str("{\n")?; - format!("{constructor_prefix}{{\n{result}\n{closing_indent}}}") - } else { - // Every other type of data is printed with the display method - data.display().to_string() - } + if print_internals { + print_obj_value_internals(f, &v, indent, encounters)?; } + print_obj_value_props(f, &v, indent, encounters, print_internals)?; + write!(f, "{:>indent$}}}", "", indent = indent.saturating_sub(4))?; + + // If the current object is referenced in a different branch, + // it will not cause an infinite printing loop, so it is safe to be printed again + encounters.remove(&addr); + Ok(()) + } else { + // Every other type of data is printed with the display method + log_value_to(f, data, print_internals, false) + } +} - /// The constructor can be retrieved as `Object.getPrototypeOf(obj).constructor`. - /// - /// Also return `None` if constructor is `Object` as plain object does not need name. - fn get_constructor_name_not_object(obj: &JsObject) -> Option { - let prototype = obj.prototype()?; +/// The constructor can be retrieved as `Object.getPrototypeOf(obj).constructor`. +/// +/// Returns `None` if the constructor is `Object` as plain objects don't need a name. +fn get_constructor_name_of(obj: &JsObject) -> Option { + let prototype = obj.prototype()?; - // To neglect out plain object - // `Object.getPrototypeOf(Object.prototype)` => null. - // For user created `Object.create(Object.create(null))`, - // we also don't need to display its name. - prototype.prototype()?; + // To neglect out plain object + // `Object.getPrototypeOf(Object.prototype)` => null. + // For user created `Object.create(Object.create(null))`, + // we also don't need to display its name. + prototype.prototype()?; - let constructor_property = prototype - .borrow() - .properties() - .get(&PropertyKey::from(js_string!("constructor")))?; - let constructor = constructor_property.value()?; + let constructor_property = prototype + .borrow() + .properties() + .get(&PropertyKey::from(js_string!("constructor")))?; + let constructor = constructor_property.value()?; - let name = constructor - .as_object()? - .borrow() - .properties() - .get(&PropertyKey::from(js_string!("name")))? - .value()? - .as_string()?; + let name = constructor + .as_object()? + .borrow() + .properties() + .get(&PropertyKey::from(js_string!("name")))? + .value()? + .as_string()?; - Some(name) - } + Some(name) +} - // We keep track of which objects we have encountered by keeping their - // in-memory address in this set - let mut encounters = HashSet::new(); +impl JsValue { + /// A helper function for specifically printing object values + #[must_use] + pub fn display_obj(&self, print_internals: bool) -> String { + struct DisplayObj<'a>(&'a JsValue, bool); + impl Display for DisplayObj<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + log_object_to_internal(f, self.0, &mut HashSet::new(), 4, self.1) + } + } - display_obj_internal(self, &mut encounters, 4, print_internals) + DisplayObj(self, print_internals).to_string() } } impl Display for ValueDisplay<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self.value.variant() { - JsVariant::Null => write!(f, "null"), - JsVariant::Undefined => write!(f, "undefined"), - JsVariant::Boolean(v) => write!(f, "{v}"), - JsVariant::Symbol(symbol) => { - write!(f, "{}", symbol.descriptive_string().to_std_string_escaped()) - } - JsVariant::String(v) => write!(f, "\"{}\"", v.to_std_string_escaped()), - JsVariant::Float64(v) => format_rational(v, f), - JsVariant::Object(_) => { - write!(f, "{}", log_string_from(self.value, self.internals, true)) - } - JsVariant::Integer32(v) => write!(f, "{v}"), - JsVariant::BigInt(num) => write!(f, "{num}n"), - } + log_value_to(f, self.value, self.internals, true) } } /// This is different from the ECMAScript compliant number to string, in the printing of `-0`. /// /// This function prints `-0` as `-0` instead of positive `0` as the specification says. -/// This is done to make it easer for the user of the REPL to identify what is a `-0` vs `0`, -/// since the REPL is not bound to the ECMAScript specification we can do this. +/// This is done to make it easier for the user of the REPL to identify what is a `-0` vs `0`, +/// since the REPL is not bound to the ECMAScript specification, we can do this. fn format_rational(v: f64, f: &mut fmt::Formatter<'_>) -> fmt::Result { if v.is_sign_negative() && v == 0.0 { f.write_str("-0") } else { let mut buffer = ryu_js::Buffer::new(); - write!(f, "{}", buffer.format(v)) + f.write_str(buffer.format(v)) } }