fix(lru-cache): faithful JS-value keys/values, GC rooting, TTL + updateAgeOnGet - #7136
fix(lru-cache): faithful JS-value keys/values, GC rooting, TTL + updateAgeOnGet#7136jdalton wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe native ChangesLRU cache fidelity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JavaScript
participant NativeDispatch
participant LruCacheHandle
participant GarbageCollector
JavaScript->>NativeDispatch: construct LRUCache(options)
NativeDispatch->>LruCacheHandle: create cache with options
JavaScript->>NativeDispatch: get, has, delete, or peek(key)
NativeDispatch->>LruCacheHandle: canonicalize key and access entry
LruCacheHandle-->>NativeDispatch: return value or undefined
GarbageCollector->>LruCacheHandle: scan cached heap values
LruCacheHandle-->>GarbageCollector: update forwarded value pointers
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bf0c65a to
51b0a1b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/perry-ext-lru-cache/src/tests.rs (2)
188-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce timing flakiness in the TTL tests.
update_age_on_get_refreshes_ttlandno_update_age_on_get_lets_ttl_expiredepend on real sleeps with a 30 ms margin against an 80 ms TTL. On a loaded runner a 50 ms sleep can overshoot 80 ms, and the refresh test then fails. Increase the TTL relative to the sleeps, or drive the mock-timer facility that the crate docs describe forjs_performance_now.♻️ Proposed change: widen the margins
- let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(80.0), true)); + let h = perry_ffi::register_handle(LruCacheHandle::new(10, Some(300.0), true)); js_lru_cache_set(h, 1.0, 111.0); // Halfway through the TTL, a get refreshes the clock. - std::thread::sleep(std::time::Duration::from_millis(50)); + std::thread::sleep(std::time::Duration::from_millis(150)); assert_eq!(js_lru_cache_get(h, 1.0), 111.0); // Another half-TTL later the entry is still live *because* it was - // refreshed (without updateAgeOnGet it would have expired at ~80ms). - std::thread::sleep(std::time::Duration::from_millis(50)); + // refreshed (without updateAgeOnGet it would have expired at ~300ms). + std::thread::sleep(std::time::Duration::from_millis(150)); assert_eq!(js_lru_cache_get(h, 1.0), 111.0, "get refreshed the TTL");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-ext-lru-cache/src/tests.rs` around lines 188 - 212, Reduce timing flakiness in update_age_on_get_refreshes_ttl and no_update_age_on_get_lets_ttl_expire by widening the TTL margin relative to their existing sleeps, or use the crate’s documented mock-timer facility for js_performance_now. Preserve the assertions that get refreshes the TTL when enabled and that the entry expires without refresh when disabled.
236-269: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore the GC global state even when an assertion fails.
cached_value_survives_gc_cyclemutates process-global state: it enables write barriers and pushes a shadow frame. The teardown at lines 267-268 runs only on the success path. If an assertion panics, barriers stay enabled and the shadow frame stays pushed for every later test in the same binary. Theunwrap_or_else(|poisoned| …)on the lock shows that a panic is expected to be survivable, so make the teardown unconditional with aDropguard.♻️ Proposed change: teardown in `Drop`
perry_runtime::gc::js_gc_write_barriers_emitted(1); let frame = perry_runtime::gc::js_shadow_frame_push(0); + struct GcGuard(u64); + impl Drop for GcGuard { + fn drop(&mut self) { + perry_runtime::gc::js_shadow_frame_pop(self.0); + perry_runtime::gc::js_gc_write_barriers_emitted(0); + } + } + let _gc_guard = GcGuard(frame); ensure_gc_scanner();perry_ffi::drop_handle(h); - perry_runtime::gc::js_shadow_frame_pop(frame); - perry_runtime::gc::js_gc_write_barriers_emitted(0); }Adjust
GcGuard's field type to the type thatjs_shadow_frame_pushreturns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-ext-lru-cache/src/tests.rs` around lines 236 - 269, Update cached_value_survives_gc_cycle to restore GC global state through an unconditional Drop guard rather than success-path teardown. Add or reuse GcGuard, set its field type to the return type of js_shadow_frame_push, and have its Drop implementation pop the shadow frame and disable write barriers; ensure the guard is created after acquiring GC_TEST_LOCK and remains active through all assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/lower_call/builtin.rs`:
- Around line 531-536: Update the "LRUCache" arm to lower every argument after
the first via the existing side-effect-preserving pattern used by neighboring
arms such as "Utf8Stream" and "EventEmitter". Keep the first argument as
opts_val, and ensure calls like new LRUCache(opts, f()) still emit f().
In `@crates/perry-ext-lru-cache/src/lib.rs`:
- Around line 126-135: Update the string-key conversion logic around
js_get_string_pointer_unified and read_bytes so failed materialization returns a
distinct non-colliding CacheKey variant instead of
CacheKey::Str(Box::default()). Preserve CacheKey::Str for successfully
materialized strings, including the real empty string, and add the variant to
CacheKey and any exhaustive matching or hashing logic that requires it.
- Around line 248-252: The max option handling in LruCacheHandle initialization
must cap large finite values before converting n to usize and passing max to
LruCacheHandle::new. Add an upper bound to the n >= 1.0 branch, preserving valid
values while preventing oversized allocations from lru’s internal HashMap.
---
Nitpick comments:
In `@crates/perry-ext-lru-cache/src/tests.rs`:
- Around line 188-212: Reduce timing flakiness in
update_age_on_get_refreshes_ttl and no_update_age_on_get_lets_ttl_expire by
widening the TTL margin relative to their existing sleeps, or use the crate’s
documented mock-timer facility for js_performance_now. Preserve the assertions
that get refreshes the TTL when enabled and that the entry expires without
refresh when disabled.
- Around line 236-269: Update cached_value_survives_gc_cycle to restore GC
global state through an unconditional Drop guard rather than success-path
teardown. Add or reuse GcGuard, set its field type to the return type of
js_shadow_frame_push, and have its Drop implementation pop the shadow frame and
disable write barriers; ensure the guard is created after acquiring GC_TEST_LOCK
and remains active through all assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 211cad3f-14d1-4dd6-902b-e081fdf14826
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
changelog.d/7136-lru-cache-faithful.mdcrates/perry-codegen/src/lower_call/builtin.rscrates/perry-codegen/src/lower_call/native_table/node_misc.rscrates/perry-ext-lru-cache/Cargo.tomlcrates/perry-ext-lru-cache/src/lib.rscrates/perry-ext-lru-cache/src/tests.rs
| "LRUCache" => { | ||
| let max_val = if let Some(opts_arg) = args.first() { | ||
| let mut found_max: Option<String> = None; | ||
| if let Some(props) = extract_options_fields(ctx, opts_arg) { | ||
| for (k, vexpr) in &props { | ||
| if k == "max" { | ||
| found_max = Some(lower_expr(ctx, vexpr)?); | ||
| } else { | ||
| // Lower other fields for side effects (e.g. ttl | ||
| // option's setter calls). | ||
| let _ = lower_expr(ctx, vexpr)?; | ||
| } | ||
| } | ||
| } else { | ||
| // Non-literal arg (variable, dynamic shape) — lower for | ||
| // side effects only; cannot extract max statically. | ||
| let _ = lower_expr(ctx, opts_arg)?; | ||
| } | ||
| found_max.unwrap_or_else(|| "100.0".to_string()) | ||
| let opts_val = if let Some(opts_arg) = args.first() { | ||
| lower_expr(ctx, opts_arg)? | ||
| } else { | ||
| "100.0".to_string() | ||
| double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Lower the remaining arguments for their side effects.
This arm lowers only args.first(). Every neighboring arm in this file lowers the tail arguments for side effects, for example Utf8Stream at lines 74-76 and EventEmitter at lines 278-280. If a caller writes new LRUCache(opts, f()), the call to f() is never emitted. Keep the convention.
🐛 Proposed fix
let opts_val = if let Some(opts_arg) = args.first() {
lower_expr(ctx, opts_arg)?
} else {
double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED))
};
+ // Lower any extra args for side effects.
+ for arg in args.iter().skip(1) {
+ let _ = lower_expr(ctx, arg)?;
+ }
let blk = ctx.block();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "LRUCache" => { | |
| let max_val = if let Some(opts_arg) = args.first() { | |
| let mut found_max: Option<String> = None; | |
| if let Some(props) = extract_options_fields(ctx, opts_arg) { | |
| for (k, vexpr) in &props { | |
| if k == "max" { | |
| found_max = Some(lower_expr(ctx, vexpr)?); | |
| } else { | |
| // Lower other fields for side effects (e.g. ttl | |
| // option's setter calls). | |
| let _ = lower_expr(ctx, vexpr)?; | |
| } | |
| } | |
| } else { | |
| // Non-literal arg (variable, dynamic shape) — lower for | |
| // side effects only; cannot extract max statically. | |
| let _ = lower_expr(ctx, opts_arg)?; | |
| } | |
| found_max.unwrap_or_else(|| "100.0".to_string()) | |
| let opts_val = if let Some(opts_arg) = args.first() { | |
| lower_expr(ctx, opts_arg)? | |
| } else { | |
| "100.0".to_string() | |
| double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) | |
| }; | |
| "LRUCache" => { | |
| let opts_val = if let Some(opts_arg) = args.first() { | |
| lower_expr(ctx, opts_arg)? | |
| } else { | |
| double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) | |
| }; | |
| // Lower any extra args for side effects. | |
| for arg in args.iter().skip(1) { | |
| let _ = lower_expr(ctx, arg)?; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/lower_call/builtin.rs` around lines 531 - 536,
Update the "LRUCache" arm to lower every argument after the first via the
existing side-effect-preserving pattern used by neighboring arms such as
"Utf8Stream" and "EventEmitter". Keep the first argument as opts_val, and ensure
calls like new LRUCache(opts, f()) still emit f().
| if jv.is_any_string() { | ||
| // Materialize either string repr into a heap header, then copy bytes. | ||
| let ptr = unsafe { js_get_string_pointer_unified(key) } as *mut StringHeader; | ||
| if !ptr.is_null() { | ||
| let handle = unsafe { JsString::from_raw(ptr) }; | ||
| if let Some(bytes) = read_bytes(handle) { | ||
| return CacheKey::Str(bytes.to_vec().into_boxed_slice()); | ||
| } | ||
| } | ||
| CacheKey::Str(Box::default()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distinguish a failed string materialization from an empty string key.
If js_get_string_pointer_unified returns null or read_bytes returns None, the code falls back to CacheKey::Str(Box::default()). That value equals the key for the real "" string, and every failed materialization aliases the same key. A get can then return a value stored under an unrelated key. Use a distinct variant for the unresolvable case so it cannot collide.
🐛 Proposed fix: add a non-colliding variant
/// Heap pointer identity (lower 48 bits) for object/array/function keys.
Obj(u64),
+ /// A string key whose bytes could not be materialized. Kept separate from
+ /// `Str(b"")` so a failed read never aliases the empty-string key.
+ UnresolvedStr(u64),
}- CacheKey::Str(Box::default())
+ CacheKey::UnresolvedStr(key.to_bits())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if jv.is_any_string() { | |
| // Materialize either string repr into a heap header, then copy bytes. | |
| let ptr = unsafe { js_get_string_pointer_unified(key) } as *mut StringHeader; | |
| if !ptr.is_null() { | |
| let handle = unsafe { JsString::from_raw(ptr) }; | |
| if let Some(bytes) = read_bytes(handle) { | |
| return CacheKey::Str(bytes.to_vec().into_boxed_slice()); | |
| } | |
| } | |
| CacheKey::Str(Box::default()) | |
| if jv.is_any_string() { | |
| // Materialize either string repr into a heap header, then copy bytes. | |
| let ptr = unsafe { js_get_string_pointer_unified(key) } as *mut StringHeader; | |
| if !ptr.is_null() { | |
| let handle = unsafe { JsString::from_raw(ptr) }; | |
| if let Some(bytes) = read_bytes(handle) { | |
| return CacheKey::Str(bytes.to_vec().into_boxed_slice()); | |
| } | |
| } | |
| CacheKey::UnresolvedStr(key.to_bits()) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-ext-lru-cache/src/lib.rs` around lines 126 - 135, Update the
string-key conversion logic around js_get_string_pointer_unified and read_bytes
so failed materialization returns a distinct non-colliding CacheKey variant
instead of CacheKey::Str(Box::default()). Preserve CacheKey::Str for
successfully materialized strings, including the real empty string, and add the
variant to CacheKey and any exhaustive matching or hashing logic that requires
it.
| if let Some(n) = option_number(ptr, "max") { | ||
| if n >= 1.0 { | ||
| max = n as usize; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Fetch the lru 0.18 source and inspect `LruCache::new` / `with_capacity`.
mkdir -p /tmp/lru && cd /tmp/lru
curl -sL https://crates.io/api/v1/crates/lru/0.18.0/download -o lru.crate
tar xzf lru.crate
rg -n -A 20 'pub fn new\(|fn construct\(|with_capacity' lru-0.18.0/src/lib.rs | head -80Repository: PerryTS/perry
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files around target library =="
git ls-files | rg 'crates/perry-ext-lru-cache/src/lib.rs|Cargo.toml|Cargo.lock' || true
echo "== target snippet =="
cat -n crates/perry-ext-lru-cache/src/lib.rs | sed -n '220,270p'
echo "== dependency declarations for lru =="
rg -n 'name = "lru"|lru\s*\{' Cargo.toml crates/perry-ext-lru-cache/Cargo.toml Cargo.lock 2>/dev/null || true
rg -n 'lru ' Cargo.lock 2>/dev/null || true
echo "== option_number definition =="
rg -n 'fn option_number|option_number' crates/perry-ext-lru-cache/src/lib.rs crates -g '*.rs' -C 4 || trueRepository: PerryTS/perry
Length of output: 28289
🌐 Web query:
lru 0.18.0 LruCache::new with_capacity source HashMap with_capacity cap
💡 Result:
In the lru crate (version 0.18.0), the LruCache::new constructor does not use an explicit with_capacity method on the LruCache struct itself [1][2][3]. Instead, it internally utilizes std::collections::HashMap::with_capacity [2][3]. The implementation of LruCache::new is as follows: pub fn new(cap: NonZeroUsize) -> LruCache<K, V> { LruCache::construct(cap, HashMap::with_capacity(cap.get)) } The constructor takes a NonZeroUsize to define the capacity, which ensures the cache is always initialized with a valid, non-zero limit [1][2][3]. This capacity is passed directly to the underlying HashMap via HashMap::with_capacity, which pre-allocates memory for the hash map based on that size [2][3]. This design differs from some other LRU implementations that might expose a public with_capacity method [4][5]. In the lru crate, the primary constructor is new, and the internal HashMap capacity is managed automatically based on the NonZeroUsize value provided [2][3].
Citations:
- 1: https://docs.rs/lru/latest/lru/struct.LruCache.html
- 2: https://docs.rs/lru/latest/x86_64-pc-windows-msvc/src/lru/lib.rs.html
- 3: https://github.com/jeromefroe/lru-rs/blob/master/src/lib.rs
- 4: https://tikv.github.io/doc/tikv_util/lru/struct.LruCache.html
- 5: https://github.com/pola-rs/polars/blob/b6ae1153/crates/polars-utils/src/cache.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lru dependency lock section =="
python3 - <<'PY'
from pathlib import Path
p = Path("Cargo.lock")
text = p.read_text()
for line in text.splitlines():
if line.startswith('name = "lru"'):
print(line)
PY
echo "== LruCacheHandle and cache construction =="
cat -n crates/perry-ext-lru-cache/src/lib.rs | sed -n '1,130p'
echo "== Rust as-cast saturation probe =="
python3 - <<'PY'
import subprocess, sys
code = r'''
#![cfg(target_pointer_width = "")]
fn main() {
let values: &[f64] = &[1e12, 1e19, 1e30, f64::INFINITY, f64::MAX, 1.0, 2.7, 0.0, -1.0];
for v in values {
println!("{v} -> {}", if v.is_finite() { v as usize } else { usize::MAX });
}
}
'''
# Avoid executing repository code: create temp file and run cargo/rustc only if tooling exists.
# For deterministic behavior, use a tiny external crate-free source is not possible here due command syntax.
PY
cat > /tmp/rust_f64_usize_sat.rs <<'RS'
#![allow(unused)]
fn main() {
let values: &[f64] = &[1e12, 1e19, 1e30, f64::INFINITY, f64::MAX, 1.0, 2.7, 0.0, -1.0];
for v in values {
println!("{v} -> {}", if v.is_finite() { v as usize } else { usize::MAX });
}
}
RS
if command -v rustc >/dev/null 2>&1; then
rustc /tmp/rust_f64_usize_sat.rs -o /tmp/rust_f64_usize_sat
/tmp/rust_f64_usize_sat
else
echo "rustc not available"
fiRepository: PerryTS/perry
Length of output: 6763
🌐 Web query:
docs.rs lru 0.18.0 source lru.rs new HashMap::with_capacity cap
💡 Result:
In version 0.18.0 of the lru crate, the LruCache::new function initializes the internal HashMap using HashMap::with_capacity(cap.get()) [1][2]. Specifically, the source code for the new constructor in src/lib.rs is implemented as follows [3]: pub fn new(cap: NonZeroUsize) -> LruCache<K, V> { LruCache::construct(cap, HashMap::with_capacity(cap.get)) } The LruCache::construct function then uses this map to initialize the cache structure [3]. For caches utilizing a custom hasher, the LruCache::with_hasher method similarly uses HashMap::with_capacity_and_hasher(cap.into(), hash_builder) to allocate the internal map [1][3].
Citations:
- 1: https://docs.rs/lru/latest/x86_64-pc-windows-msvc/src/lru/lib.rs.html
- 2: https://docs.rs/lru/latest/src/lru/lib.rs.html
- 3: https://github.com/jeromefroe/lru-rs/blob/master/src/lib.rs
🌐 Web query:
site:docs.rs lru 0.18.0 LruCache::new HashMap with_capacity
💡 Result:
In the lru crate version 0.18.0, the LruCache::new constructor takes a NonZeroUsize parameter to specify the capacity of the cache [1][2]. The implementation internally initializes a HashMap with this capacity [2][3]: pub fn new(cap: NonZeroUsize) -> LruCache<K, V> { LruCache::construct(cap, HashMap::with_capacity(cap.get)) } As shown above, the constructor implicitly calls HashMap::with_capacity using the value provided by the NonZeroUsize input to allocate the underlying hash map [2][3]. Users do not need to manually specify a hash map capacity, as this is handled automatically during the initialization of the LruCache [2].
Citations:
- 1: https://docs.rs/lru/latest/lru/struct.LruCache.html
- 2: https://docs.rs/lru/latest/src/lru/lib.rs.html
- 3: https://docs.rs/lru/latest/x86_64-pc-windows-msvc/src/lru/lib.rs.html
Clamp max before creating the LruCache.
option_number accepts large finite numbers, and n as usize saturates to usize::MAX, which lru 0.18 passes to the internal HashMap capacity. A call like new LRUCache({ max: 1e12 }) can request a huge allocation and abort the process. Add an upper bound before passing max to LruCacheHandle::new.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-ext-lru-cache/src/lib.rs` around lines 248 - 252, The max option
handling in LruCacheHandle initialization must cap large finite values before
converting n to usize and passing max to LruCacheHandle::new. Add an upper bound
to the n >= 1.0 branch, preserving valid values while preventing oversized
allocations from lru’s internal HashMap.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-ext-lru-cache/src/tests.rs`:
- Around line 236-268: Add a local Drop guard immediately after the GC setup in
the test, covering the state changed by js_gc_write_barriers_emitted and
js_shadow_frame_push. Implement its cleanup to pop frame and disable write
barriers, then remove the manual cleanup calls at the test end so cleanup also
runs during panics from ensure_gc_scanner, js_lru_cache_get, or assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a229d22a-8368-4eb3-aeed-92ef3ac8160a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
changelog.d/7136-lru-cache-faithful.mdcrates/perry-codegen/src/lower_call/builtin.rscrates/perry-codegen/src/lower_call/native_table/node_misc.rscrates/perry-ext-lru-cache/Cargo.tomlcrates/perry-ext-lru-cache/src/lib.rscrates/perry-ext-lru-cache/src/tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- changelog.d/7136-lru-cache-faithful.md
- crates/perry-codegen/src/lower_call/native_table/node_misc.rs
- crates/perry-codegen/src/lower_call/builtin.rs
- crates/perry-ext-lru-cache/Cargo.toml
- crates/perry-ext-lru-cache/src/lib.rs
| let _lock = GC_TEST_LOCK | ||
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()); | ||
|
|
||
| // Match the runtime's evacuation preconditions: write barriers active | ||
| // and a live shadow frame (mirrors perry-ext-events' scanner test). | ||
| perry_runtime::gc::js_gc_write_barriers_emitted(1); | ||
| let frame = perry_runtime::gc::js_shadow_frame_push(0); | ||
| ensure_gc_scanner(); | ||
|
|
||
| let h = js_lru_cache_new(f64::from_bits(TAG_UNDEFINED)); | ||
| // A >5-byte string forces the heap `StringHeader` repr (not inline SSO), | ||
| // so it is a real collectable allocation. Its ONLY root is the cache. | ||
| js_lru_cache_set(h, 1.0, string_value("value-object-1234567890")); | ||
|
|
||
| // Reclaims unrooted nursery allocations and evacuates rooted survivors. | ||
| let _ = perry_runtime::gc::gc_collect_minor(); | ||
|
|
||
| let got = js_lru_cache_get(h, 1.0); | ||
| assert_ne!( | ||
| got.to_bits(), | ||
| TAG_UNDEFINED, | ||
| "cached value was collected — root scanner did not keep it alive" | ||
| ); | ||
| assert_eq!( | ||
| read_string_value(got).as_deref(), | ||
| Some("value-object-1234567890"), | ||
| "cached value corrupted across GC — slot was not rewritten to the forwarded address" | ||
| ); | ||
|
|
||
| perry_ffi::drop_handle(h); | ||
| perry_runtime::gc::js_shadow_frame_pop(frame); | ||
| perry_runtime::gc::js_gc_write_barriers_emitted(0); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make GC-state cleanup panic-safe.
Lines 242-244 modify process-global GC state. If ensure_gc_scanner, js_lru_cache_get, or an assertion panics, lines 266-268 do not run. Later tests can then run with write barriers enabled and an unpopped shadow frame.
Install a local Drop guard immediately after the GC setup. The guard must always pop frame and disable write barriers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-ext-lru-cache/src/tests.rs` around lines 236 - 268, Add a local
Drop guard immediately after the GC setup in the test, covering the state
changed by js_gc_write_barriers_emitted and js_shadow_frame_push. Implement its
cleanup to pop frame and disable write barriers, then remove the manual cleanup
calls at the test end so cleanup also runs during panics from ensure_gc_scanner,
js_lru_cache_get, or assertions.
…teAgeOnGet
perry-ext-lru-cache only handled numeric (f64) keys/values with no TTL, so
real usage (string keys -> object/string values with ttl + updateAgeOnGet,
e.g. Socket Firewall) silently misbehaved.
- Keys/values are treated as real NaN-boxed JS values, not raw f64 bits.
String keys hash/compare by CONTENT (materialized via
js_get_string_pointer_unified); numbers/bools/null/undefined key by
canonical value (SameValueZero). get returns real `undefined` on a miss;
has/delete return real booleans.
- Cached heap values are GC roots while cached: a mutable root scanner
(gc_register_mutable_root_scanner_named) visits every cached value slot
each cycle, so values are marked AND rewritten to their forwarded address
under copying evacuation -- fixing a use-after-free.
- new LRUCache({ max, ttl, updateAgeOnGet }) is parsed from the NaN-boxed
options object; codegen forwards the whole object (dynamic options work)
instead of statically extracting only max. ttl uses performance.now for
expiry (get/has/peek treat expired as absent; get evicts); updateAgeOnGet
refreshes the TTL clock on a live get. peek wired into method dispatch.
Adds Rust unit tests (string-key content round-trip, object-value survival
across a forced GC cycle, TTL expiry, updateAgeOnGet, eviction at max) --
all green via `cargo test -p perry-ext-lru-cache`.
Tracking: PerryTS#466 (Phase 5 native bindings). PR PerryTS#7136.
51b0a1b to
de11c2b
Compare
1 participant
Perry's built-in
lru-cachebinding only ever worked correctly for numeric keys and numeric values with no expiry. The way people actually use the package, string keys mapping to objects or strings withttlandupdateAgeOnGetset, misbehaved silently. Three separate things went wrong, and none of them raised an error.A
get("k")after aset("k", …)would miss, because string keys were compared by their pointer bits rather than their text, so two separately allocated copies of the same string looked like different keys. Cached values that live on the heap were never registered as garbage-collector roots, so the collector was free to reclaim or relocate a value that was still in the cache, and reading it afterwards produced theTypeError: value is not a functionclass of failure. And thettlandupdateAgeOnGetoptions were parsed but ignored, so entries never expired.This makes the binding behave like the npm package for the surface it covers. The extern signatures do not change; the interface stays 64-bit-float-wide.
Background: how a JavaScript value fits in a 64-bit float
The binding's interface passes every key and value as an
f64, a 64-bit floating-point number. That is not a restriction to numbers, because Perry uses NaN-boxing.A 64-bit float has a large family of bit patterns that all mean "not a number", and only one of them is ever produced by arithmetic. The rest are unused, so a runtime can hide other things in them: a pointer to a heap object, a boolean,
null,undefined. A real number is stored as itself, and everything else is stored as a tagged pattern inside the unused NaN space.The old binding treated the incoming
f64as raw bits rather than as a NaN-boxed value. That is the root of the first two defects: raw bits of a string are its pointer, so comparing them compares addresses, and raw bits are invisible to the collector, so nothing kept the pointed-at object alive.Keys and values are now real NaN-boxed values
String keys are hashed and compared by content. The string's bytes are materialized through
js_get_string_pointer_unifiedand the map is keyed on those bytes, so two separately allocated copies of"k"are the same key.Numbers, booleans,
null, andundefinedare keyed by canonical value using SameValueZero, the same comparison JavaScript's ownMapuses. That unifies+0with-0and treats allNaNvalues as one key.A miss from
getnow returns a realundefined, where it previously returned a rawNaN.Cached values are garbage-collector roots while cached
A mutable root scanner is registered with
gc_register_mutable_root_scanner_named, and it visits every cached value slot on each collection cycle.That does two jobs at once. It marks the value as live, so the collector does not reclaim something the cache still holds. And under copying evacuation, where the collector physically moves objects, it rewrites the cache's stored pointer to the value's forwarded address, so the cache keeps pointing at the object after it moves.
This mirrors what
perry-ext-eventsandperry-ext-exponential-backoffalready do.Constructor options are now honored
new LRUCache({ max, ttl, updateAgeOnGet })is parsed from the NaN-boxed options object at runtime. Code generation now forwards the whole object rather than statically extracting onlymax, which is what makes dynamically built or variable option objects work rather than only object literals.ttlsets a per-entry lifetime measured against the runtime'sperformance.now()clock.get,has, andpeekall treat an expired entry as absent, andgetevicts it.updateAgeOnGetresets an entry's expiry clock on agetthat finds it still live.peekis now wired into method dispatch, andhasanddeletereturn real booleans.Test runs
cargo test -p perry-ext-lru-cachereports 16 passing. The new tests cover a string-key content round-trip, an object value surviving a forcedgc_collect_minor()cycle, expiry and eviction underttl,updateAgeOnGetrefreshing versus not refreshing, eviction oncemaxis reached, and primitive-key derivation.The full Perry compiler builds clean with the code-generation changes.
What is deliberately not covered
maxSizeandsizeCalculation,disposeanddisposeAfter,fetch,allowStale, per-call option objects, and the iterator surface are all out of scope, because the(key, value)interface cannot carry them.Object-identity keys work through pointer identity, but they are not tracked across a garbage-collector relocation, so an object key can stop matching after a collection moves it. Primitive keys are the collector-safe path and cover all real usage seen so far.
Tracked under #466, Phase 5 native bindings.
Summary by CodeRabbit
New Features
peekaccess without updating entry recency.Bug Fixes