-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): grow large pre-sized arrays densely #9376
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Large pre-sized arrays now materialize their dense backing storage incrementally | ||
| during sequential indexed writes instead of falling back to quadratic sparse | ||
| property insertion. Growth also preserves expandos and GC-traced element slots. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,44 @@ | ||
| //! Indexing support split out of `indexing.rs` to keep it under the repo's | ||
| //! 2000-line cap: the strict-store TypeError throwers, the prototype | ||
| //! indexed-property / iterator invalidation latches, and the dense keys-array | ||
| //! slot helpers. Pure move except for the `use` lines and `pub(super)` | ||
| //! visibility on items `indexing.rs` still calls. | ||
| //! indexed-property / iterator invalidation latches, sparse index helpers, and | ||
| //! the dense keys-array slot helpers. Pure move except for the `use` lines and | ||
| //! `pub(super)` visibility on items `indexing.rs` still calls. | ||
| use super::*; | ||
| use std::sync::atomic::{AtomicBool, AtomicU8, Ordering}; | ||
|
|
||
| /// Largest hole (`index - length`) an extending write may create while still | ||
| /// growing the dense backing store once the array is large. Sparse storage is | ||
| /// for jumps far beyond the current length; sequential growth must stay dense | ||
| /// because routing it through string-keyed property sets is quadratic. | ||
| pub(super) const DENSE_ARRAY_GAP_LIMIT: u32 = 1024; | ||
|
|
||
| #[inline] | ||
| pub(super) unsafe fn array_sparse_index_property_get( | ||
| arr: *const ArrayHeader, | ||
| index: u32, | ||
| ) -> Option<f64> { | ||
| let arr = clean_arr_ptr(arr); | ||
| if arr.is_null() || index < (*arr).capacity { | ||
| return None; | ||
| } | ||
| let key = index.to_string(); | ||
| array_named_property_get_by_name(arr, &key) | ||
| } | ||
|
|
||
| pub(super) unsafe fn array_sparse_index_property_set( | ||
| arr: *mut ArrayHeader, | ||
| index: u32, | ||
| value: f64, | ||
| ) { | ||
| let key = index.to_string(); | ||
| let key_ptr = crate::string::js_string_from_bytes(key.as_ptr(), key.len() as u32); | ||
| array_named_property_set(arr, key_ptr, value); | ||
| let new_length = index + 1; | ||
| if (*arr).length < new_length { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: printf '%s\n' '--- repository conventions ---'
find /tmp/coderabbit-repo-knowledge/perryts-perry-d4a878bc -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- changed file ---'
cat -n crates/perry-runtime/src/array/indexing_support.rs | sed -n '1,130p'
printf '%s\n' '--- nearby callers and symbols ---'
rg -n -A12 -B8 'array_sparse_index_property_set|array_named_property_set|js_string_from_bytes|RuntimeHandleScope' crates/perry-runtime/src/array crates/perry-runtime/src | head -240Repository: PerryTS/perry Length of output: 30854 🏁 Script executed: printf '%s\n' '--- applicable convention ---'
cat /tmp/coderabbit-repo-knowledge/perryts-perry-d4a878bc/conventions/crates-perry-runtime.md
printf '%s\n' '--- relevant learned guidance ---'
cat /tmp/coderabbit-repo-knowledge/perryts-perry-d4a878bc/learnings/crates-perry-runtime-src.md
printf '%s\n' '--- exact definitions and callers ---'
rg -n '^\s*(pub\s+)?(unsafe\s+)?fn\s+(array_sparse_index_property_set|array_named_property_set|js_string_from_bytes|clean_arr_ptr)|array_sparse_index_property_set\(' crates/perry-runtime/src
printf '%s\n' '--- handle scope definition ---'
rg -n -A45 -B8 'struct RuntimeHandleScope|impl RuntimeHandleScope|root_nanbox_f64' crates/perry-runtime/src/gc crates/perry-runtime/src | head -220Repository: PerryTS/perry Length of output: 24573 🏁 Script executed: printf '%s\n' '--- sparse-write caller contexts ---'
cat -n crates/perry-runtime/src/array/indexing.rs | sed -n '1100,1160p;1210,1270p;1880,1960p'
printf '%s\n' '--- bound named-property setter ---'
rg -n -A35 -B12 'array_named_property_set' crates/perry-runtime/src
printf '%s\n' '--- string allocator implementation ---'
rg -n -A45 -B12 'js_string_from_bytes' crates/perry-runtime/src/string crates/perry-runtime/src | grep -v geisterhand_registry | head -180
printf '%s\n' '--- handle accessors ---'
cat -n crates/perry-runtime/src/gc/roots/runtime_handles.rs | sed -n '128,210p'Repository: PerryTS/perry Length of output: 50369 🏁 Script executed: printf '%s\n' '--- string source files ---'
fd -i 'string' crates/perry-runtime/src -t f | head -40
printf '%s\n' '--- js_string_from_bytes declaration and body ---'
rg -n -C 18 'js_string_from_bytes' crates/perry-runtime/src/string crates/perry-runtime/src/string.rs 2>/dev/null | head -120
printf '%s\n' '--- array_spec_set setup and sparse branches ---'
cat -n crates/perry-runtime/src/array/indexing.rs | sed -n '1760,1960p'
printf '%s\n' '--- GC allocation calls in string implementation ---'
rg -n -C 8 'arena_alloc_gc|gc_malloc|collect|safepoint' crates/perry-runtime/src/string crates/perry-runtime/src/string.rs 2>/dev/null | head -160Repository: PerryTS/perry Length of output: 36749 🏁 Script executed: files=$(rg -l 'js_string_from_bytes' crates/perry-runtime/src/string)
printf '%s\n' '--- files containing js_string_from_bytes ---'
printf '%s\n' "$files"
for f in $files; do
printf '%s\n' "--- $f ---"
rg -n -A35 -B8 '(^|::)js_string_from_bytes|pub .*js_string_from_bytes|fn js_string_from_bytes' "$f"
done
printf '%s\n' '--- GC movement and root-scanner contract ---'
rg -n -A20 -B12 'SkipDisabled|moving|evacuat|forwarding|raw pointer|native.*frame|RuntimeHandle' crates/perry-runtime/src/gc crates/perry-runtime/src/arena crates/perry-runtime/src | head -220Repository: PerryTS/perry Length of output: 34595 🏁 Script executed: printf '%s\n' '--- string storage allocation path ---'
rg -n -A55 -B12 'fn string_storage_alloc|fn string_storage_alloc_longlived|string_storage_alloc\(' crates/perry-runtime/src/string/alloc.rs crates/perry-runtime/src/string/mod.rs
printf '%s\n' '--- arena allocator collection contract ---'
rg -n -A70 -B15 'pub.*arena_alloc_gc|fn arena_alloc_gc|gc_check_trigger|copying.*minor|collect.*minor' crates/perry-runtime/src/arena crates/perry-runtime/src/gc | head -260
printf '%s\n' '--- root handle pointer refresh methods ---'
cat -n crates/perry-runtime/src/gc/roots/runtime_handles.rs | sed -n '128,210p'Repository: PerryTS/perry Length of output: 50369 Root and reload 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| (*arr).length = new_length; | ||
| } | ||
| } | ||
|
|
||
| /// Resolve a raw array head a generated loop re-read from its root after a | ||
| /// callback returned: the callback may have grown the array, leaving the root | ||
| /// on a forwarding stub. Pure `clean_arr_ptr`; null for anything that is not | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| //! Regression coverage for #9371: a large `new Array(length)` keeps a small | ||
| //! initial backing store, but writes at the dense frontier must grow that | ||
| //! store instead of accumulating numeric keys in the named-property table. | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn large_presized_array_grows_its_dense_frontier() { | ||
| unsafe { | ||
| const LENGTH: u32 = 1_200_000; | ||
| let mut arr = js_array_constructor_single(LENGTH as f64); | ||
| assert_eq!((*arr).length, LENGTH); | ||
| assert_eq!((*arr).capacity, MIN_ARRAY_CAPACITY); | ||
|
|
||
| for index in 0..LENGTH { | ||
| let old_capacity = (*arr).capacity; | ||
| arr = js_array_set_f64_extend(arr, index, index as f64 + 0.25); | ||
| if (*arr).capacity != old_capacity { | ||
| assert_eq!( | ||
| js_array_get_f64(arr, 0), | ||
| 0.25, | ||
| "growth at index {index} lost the first value (capacity {old_capacity} -> {})", | ||
| (*arr).capacity | ||
| ); | ||
| assert_eq!( | ||
| js_array_get_f64(arr, index), | ||
| index as f64 + 0.25, | ||
| "growth at index {index} lost the current value" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| assert_eq!((*arr).length, LENGTH); | ||
| assert!( | ||
| (*arr).capacity >= LENGTH, | ||
| "sequential in-bounds writes must grow dense storage" | ||
| ); | ||
| for index in 0..LENGTH { | ||
| assert_eq!(js_array_get_f64(arr, index), index as f64 + 0.25); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn existing_sparse_indices_prevent_dense_growth_from_hiding_them() { | ||
| unsafe { | ||
| let mut arr = js_array_constructor_single(1_000_001.0); | ||
| arr = js_array_set_f64_extend(arr, 500_000, 7.0); | ||
| let capacity = (*arr).capacity; | ||
| assert!(500_000 >= capacity, "fixture must take sparse storage"); | ||
|
|
||
| arr = js_array_set_f64_extend(arr, capacity, 9.0); | ||
|
|
||
| assert_eq!( | ||
| (*arr).capacity, | ||
| capacity, | ||
| "growth must not cover an existing sparse numeric property" | ||
| ); | ||
| assert_eq!(js_array_get_f64(arr, capacity), 9.0); | ||
| assert_eq!(js_array_get_f64(arr, 500_000), 7.0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Allow dense growth below the nearest sparse numeric index.
After
new Array(1_200_000); arr[500_000] = 7, a write at capacity 16 fails this condition. Growth to 32 cannot hide index 500,000. Every later sequential prefix write then usesarray_named_property_set, whose linear property search restores quadratic behavior.Track the nearest sparse numeric index and allow dense growth below it. Migrate that sparse entry when growth reaches it.
crates/perry-runtime/src/array/indexing.rs#L1910-L1912: replace the boolean guard with a boundary check against the nearest sparse numeric index.crates/perry-runtime/src/array/large_presized_tests.rs#L52-L57: assert that growth preserves the sparse value without growing across its index.📍 Affects 2 files
crates/perry-runtime/src/array/indexing.rs#L1910-L1912(this comment)crates/perry-runtime/src/array/large_presized_tests.rs#L52-L57🤖 Prompt for AI Agents