Recognize an array index without formatting it back to a string

`array_index_property_key` decided whether a string key names an array index by
parsing it and then formatting the parsed number into a **fresh `String`** to
compare against the original. That is a heap allocation inside a predicate the
element path asks constantly: `has_index`, the element read and the element
write each call it, and so do the enumeration filters, the descriptor paths,
and the proxy and typed-array key checks.

It showed up as exactly that -- 39 allocator samples on `string-tagcloud`,
whose profile is 40% malloc and free.

The canonical spelling is what the digits already say: no sign, no leading zero
unless the key is `"0"`, at most ten digits, and a value below `u32::MAX`,
which is the length bound rather than an index. The accumulation is checked
because ten digits still overflow above `4294967295` -- the test added here
caught precisely that, an unchecked first draft accepting `"4294967296"` as
index 0.

39-case corpus 0.9971: `math-spectral-norm` 0.946, `date-format-tofte` 0.956,
`date-format-xparb` 0.958, `string-tagcloud` 0.960, `string-unpack-code` 0.972.

`object.rs` reached the file-size guard, so its unit tests move to
`value/object/tests.rs`, next to the two other test files the crate already
keeps beside their subject.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: Lewin671

crates/qjs-runtime/src/value/object.rs

@@ -1787,197 +1787,41 @@ fn is_internal_property_key(key: &str) -> bool {
     key.starts_with('\0')
 }
 
+/// Parses an array index in its canonical decimal spelling.
+///
+/// This is a predicate on the element path -- every string-keyed array access
+/// asks it, and `has_index`, the element read, and the element write each ask
+/// it again -- and it used to answer by formatting the parsed number back into
+/// a fresh `String` to compare against the key. That is a heap allocation per
+/// array access, and it showed up as such: 39 allocator samples on
+/// `string-tagcloud` alone.
+///
+/// The canonical form is exactly what the digits already say: no sign, no
+/// leading zero unless the key is `"0"`, and a value below `u32::MAX` (the
+/// array length itself is not an index).
 fn array_index_property_key(key: &str) -> Option<u32> {
-    key.parse::<u32>()
-        .ok()
-        .filter(|index| *index < u32::MAX && index.to_string() == key)
+    let bytes = key.as_bytes();
+    // `u32::MAX` has ten digits, so anything longer is out of range; a leading
+    // zero is a distinct string property rather than an index.
+    if bytes.is_empty() || bytes.len() > 10 || (bytes[0] == b'0' && bytes.len() > 1) {
+        return None;
+    }
+    let mut index: u32 = 0;
+    for byte in bytes {
+        let digit = byte.wrapping_sub(b'0');
+        if digit > 9 {
+            return None;
+        }
+        // Ten digits still overflow above `4294967295`, so the accumulation is
+        // checked rather than merely bounded by the length.
+        index = index.checked_mul(10)?.checked_add(u32::from(digit))?;
+    }
+    (index < u32::MAX).then_some(index)
 }
 
 fn is_array_index_key(key: &str) -> bool {
     array_index_property_key(key).is_some()
 }
 
 #[cfg(test)]
-mod tests {
-    use std::{collections::HashMap, mem, rc::Rc};
-
-    use super::{ObjectData, ObjectLiteralShape, ObjectRef, OwnDataPropertyWrite, PropertyStorage};
-    use crate::{Property, Value};
-
-    #[test]
-    fn cloned_object_is_a_pointer_sized_shared_handle() {
-        let object = ObjectRef::new(HashMap::new());
-        let cloned = object.clone();
-
-        assert!(Rc::ptr_eq(&object.0, &cloned.0));
-        assert!(object.ptr_eq(&cloned));
-        assert_eq!(
-            mem::size_of::<ObjectRef>(),
-            mem::size_of::<Rc<ObjectData>>()
-        );
-    }
-
-    #[test]
-    fn ordinary_object_keeps_cold_state_unallocated() {
-        let object = ObjectRef::new(HashMap::from([
-            ("a".to_owned(), Value::Number(1.0)),
-            ("b".to_owned(), Value::Number(2.0)),
-        ]));
-
-        assert_eq!(object.get("a"), Some(Value::Number(1.0)));
-        assert!(object.own_property_symbols().is_empty());
-        assert!(object.to_string_tag().is_none());
-        assert!(object.0.cold.get().is_none());
-        // Boxing the cold `Dynamic` property-storage payload (HashMap + Vec)
-        // keeps this at 104 bytes instead of the 136 it cost when that
-        // payload sized the whole `PropertyStorage` enum for every object.
-        assert!(mem::size_of::<ObjectData>() <= 112);
-    }
-
-    #[test]
-    fn ordinary_small_object_promotes_only_after_the_compact_limit() {
-        let object = ObjectRef::new(HashMap::new());
-
-        for index in 0..PropertyStorage::SMALL_LIMIT {
-            object.set(format!("field{index}"), Value::Number(index as f64));
-        }
-        assert!(matches!(
-            &*object.0.properties.borrow(),
-            PropertyStorage::Small { entries } if entries.len() == PropertyStorage::SMALL_LIMIT
-        ));
-
-        let dynamic_index = PropertyStorage::SMALL_LIMIT;
-        object.set(
-            format!("field{dynamic_index}"),
-            Value::Number(dynamic_index as f64),
-        );
-        assert!(matches!(
-            &*object.0.properties.borrow(),
-            PropertyStorage::Dynamic(dynamic)
-                if dynamic.properties.len() == PropertyStorage::SMALL_LIMIT + 1
-                    && dynamic.order.len() == PropertyStorage::SMALL_LIMIT + 1
-        ));
-        assert_eq!(
-            object.own_property_names(),
-            (0..=PropertyStorage::SMALL_LIMIT)
-                .map(|index| format!("field{index}"))
-                .collect::<Vec<_>>()
-        );
-    }
-
-    #[test]
-    fn ordinary_object_retains_shared_static_property_key() {
-        let object = ObjectRef::new(HashMap::new());
-        let key: Rc<str> = Rc::from("field");
-
-        object.set_shared_key(Rc::clone(&key), Value::Number(1.0));
-
-        assert!(matches!(
-            &*object.0.properties.borrow(),
-            PropertyStorage::Small { entries }
-                if entries.len() == 1 && Rc::ptr_eq(&entries[0].0, &key)
-        ));
-        assert_eq!(object.get("field"), Some(Value::Number(1.0)));
-    }
-
-    #[test]
-    fn small_object_removal_preserves_property_order() {
-        let object = ObjectRef::new(HashMap::new());
-        object.set("first".to_owned(), Value::Number(1.0));
-        object.set("second".to_owned(), Value::Number(2.0));
-        object.set("third".to_owned(), Value::Number(3.0));
-
-        assert!(object.delete_own_property("second"));
-        object.set("second".to_owned(), Value::Number(4.0));
-
-        assert_eq!(object.own_property_names(), ["first", "third", "second"]);
-        assert!(matches!(
-            &*object.0.properties.borrow(),
-            PropertyStorage::Small { entries } if entries.len() == 3
-        ));
-    }
-
-    #[test]
-    fn small_object_enumerates_indices_before_strings() {
-        let object = ObjectRef::new(HashMap::new());
-        object.set("10".to_owned(), Value::Number(10.0));
-        object.set("label".to_owned(), Value::Number(0.0));
-        object.set("2".to_owned(), Value::Number(2.0));
-
-        assert_eq!(object.own_property_names(), ["2", "10", "label"]);
-        assert!(matches!(
-            &*object.0.properties.borrow(),
-            PropertyStorage::Small { entries } if entries.len() == 3
-        ));
-    }
-
-    #[test]
-    fn existing_own_data_write_updates_or_rejects_without_slow_path() {
-        let object = ObjectRef::new(HashMap::from([("writable".to_owned(), Value::Number(1.0))]));
-        object.define_property(
-            "readonly".to_owned(),
-            Property::data(Value::Number(2.0), true, false, true),
-        );
-
-        assert!(matches!(
-            object.write_existing_own_data_property("writable", &Value::Number(3.0)),
-            OwnDataPropertyWrite::Written
-        ));
-        assert_eq!(object.get("writable"), Some(Value::Number(3.0)));
-        assert!(matches!(
-            object.write_existing_own_data_property("readonly", &Value::Number(4.0)),
-            OwnDataPropertyWrite::ReadOnly
-        ));
-        assert_eq!(object.get("readonly"), Some(Value::Number(2.0)));
-        assert!(matches!(
-            object.write_existing_own_data_property("missing", &Value::Number(5.0)),
-            OwnDataPropertyWrite::NeedsSlowPath
-        ));
-    }
-
-    #[test]
-    fn literal_pair_keeps_inline_values_until_descriptor_mutation() {
-        let shape = ObjectLiteralShape::new(vec![Rc::from("a"), Rc::from("b")]);
-        let object =
-            ObjectRef::with_literal_pair(shape, [Value::Number(1.0), Value::Number(2.0)], None);
-
-        assert!(matches!(
-            &*object.0.properties.borrow(),
-            PropertyStorage::ShapedPair { .. }
-        ));
-        assert!(matches!(
-            object.write_existing_own_data_property("a", &Value::Number(3.0)),
-            OwnDataPropertyWrite::Written
-        ));
-        assert_eq!(object.get("a"), Some(Value::Number(3.0)));
-        assert!(matches!(
-            &*object.0.properties.borrow(),
-            PropertyStorage::ShapedPair { .. }
-        ));
-
-        object.define_property(
-            "a".to_owned(),
-            Property::data(Value::Number(4.0), false, false, true),
-        );
-        assert!(matches!(
-            &*object.0.properties.borrow(),
-            PropertyStorage::Dynamic(_)
-        ));
-        let descriptor = object.own_property("a").expect("defined property");
-        assert_eq!(descriptor.value, Value::Number(4.0));
-        assert!(!descriptor.enumerable);
-        assert!(!descriptor.writable);
-    }
-
-    #[test]
-    fn module_namespace_own_data_write_stays_on_slow_path() {
-        let object = ObjectRef::new(HashMap::from([("exported".to_owned(), Value::Number(1.0))]));
-        object.mark_module_namespace_exotic();
-
-        assert!(matches!(
-            object.write_existing_own_data_property("exported", &Value::Number(2.0)),
-            OwnDataPropertyWrite::NeedsSlowPath
-        ));
-        assert_eq!(object.get("exported"), Some(Value::Number(1.0)));
-    }
-}
+mod tests;