Changes in crates/runtime/src/map_ops.rs: #379

Merged

navicore merged 1 commit from map.set-migration into main

2026-03-28 21:40:07 +00:00

navicore commented

2026-03-28 21:31:31 +00:00

(Migrated from github.com)

map.set — Fast path via heap_value_mut(stack.sub(3)): if the map at sp-3 is sole-owned, pops key and value, inserts directly into the map. No Box alloc/dealloc, no Arc cycle on the map. Falls back to the original pop-all path when shared.
map.remove — Fast path via heap_value_mut(stack.sub(2)): same pattern, pops key, removes from map in place.

Both follow the same SAFETY documentation pattern established in the list.push review.

- map.set — Fast path via heap_value_mut(stack.sub(3)): if the map at sp-3 is sole-owned, pops key and value, inserts directly into the map. No Box alloc/dealloc, no Arc cycle on the map. Falls back to the original pop-all path when shared. - map.remove — Fast path via heap_value_mut(stack.sub(2)): same pattern, pops key, removes from map in place. Both follow the same SAFETY documentation pattern established in the list.push review.

claude[bot] commented

2026-03-28 21:35:53 +00:00

(Migrated from github.com)

PR Review: COW Fast Path for map.set and map.remove

The optimization is correct and follows the established list.push pattern well. Here are my findings:

ISSUES

Stale module-level documentation (minor)

Lines 30-31 in the module doc block still say the performance claim "Each mutation clones the underlying HashMap (O(n) for n entries)" - which is no longer universally true. Suggested update:

//! - map-set and map-remove are semantically functional (return updated map)
//! - Fast path: sole-owned maps are mutated in place (O(1) for insert/remove)
//! - Slow path: shared maps are cloned before mutation (O(n))

Safety comment incomplete re: Box vs Arc (minor)

The inline Safety comment at the fast-path explains the Arc uniqueness check but does not note that once you have &mut Value::Map(map) via heap_value_mut, you already have &mut Box<HashMap> - and Box is always uniquely owned, so no second check is needed (unlike the Variant case which has a two-level Arc::get_mut). Worth adding a line for future readers.

Missing slow-path tests (moderate)

Every existing test creates a fresh map without dup-ing it, so only the fast path is exercised. Per the project's contributing guidelines ("complex logic has tests nearby"), tests for the slow path should be included. A minimal example: dup the map to create a second Arc reference, then verify the modified map has the new key AND the original map is unchanged. A mirror test for map_remove would complete the coverage.

map_remove fast-path doc omits missing-key behavior (minor)

The original doc said "If the key doesn't exist, returns the map unchanged." The new fast-path doc omits this. Both paths handle it identically (silent no-op), but a note would make the doc self-contained.

NITS

Slow-path blank lines between logical steps were removed. The original visual separation was clearer.
The MapKey::from_value + panic! block is repeated across fast and slow paths (pre-existing). A small helper function would reduce duplication, but is not required.

SUMMARY TABLE

Category	Finding	Severity
Correctness	Core logic and Arc::get_mut guard are sound	OK
Correctness	Box uniqueness implicit but correct	OK
Documentation	Module-level performance claim is stale	Minor
Documentation	Safety comment incomplete re: Box vs Arc	Minor
Test coverage	No slow-path (shared map) tests	Moderate
Test coverage	No behavioral parity test for fast/slow paths	Moderate
Documentation	map_remove fast-path doc omits missing-key case	Minor
Style	Slow-path blank lines removed	Nit

Verdict: The optimization is correct. The two moderate gaps (slow-path test coverage) should be addressed before merge per the project's own contributing guidelines. The module doc update is a quick fix.

Generated with Claude Code

PR Review: COW Fast Path for map.set and map.remove The optimization is correct and follows the established list.push pattern well. Here are my findings: ISSUES 1. Stale module-level documentation (minor) Lines 30-31 in the module doc block still say the performance claim "Each mutation clones the underlying HashMap (O(n) for n entries)" - which is no longer universally true. Suggested update: ``` //! - map-set and map-remove are semantically functional (return updated map) //! - Fast path: sole-owned maps are mutated in place (O(1) for insert/remove) //! - Slow path: shared maps are cloned before mutation (O(n)) ``` 2. Safety comment incomplete re: Box vs Arc (minor) The inline Safety comment at the fast-path explains the Arc uniqueness check but does not note that once you have `&mut Value::Map(map)` via `heap_value_mut`, you already have `&mut Box<HashMap>` - and Box is always uniquely owned, so no second check is needed (unlike the Variant case which has a two-level Arc::get_mut). Worth adding a line for future readers. 3. Missing slow-path tests (moderate) Every existing test creates a fresh map without dup-ing it, so only the fast path is exercised. Per the project's contributing guidelines ("complex logic has tests nearby"), tests for the slow path should be included. A minimal example: dup the map to create a second Arc reference, then verify the modified map has the new key AND the original map is unchanged. A mirror test for map_remove would complete the coverage. 4. map_remove fast-path doc omits missing-key behavior (minor) The original doc said "If the key doesn't exist, returns the map unchanged." The new fast-path doc omits this. Both paths handle it identically (silent no-op), but a note would make the doc self-contained. NITS - Slow-path blank lines between logical steps were removed. The original visual separation was clearer. - The MapKey::from_value + panic! block is repeated across fast and slow paths (pre-existing). A small helper function would reduce duplication, but is not required. SUMMARY TABLE | Category | Finding | Severity | |----------|---------|----------| | Correctness | Core logic and Arc::get_mut guard are sound | OK | | Correctness | Box<HashMap> uniqueness implicit but correct | OK | | Documentation | Module-level performance claim is stale | Minor | | Documentation | Safety comment incomplete re: Box vs Arc | Minor | | Test coverage | No slow-path (shared map) tests | Moderate | | Test coverage | No behavioral parity test for fast/slow paths | Moderate | | Documentation | map_remove fast-path doc omits missing-key case | Minor | | Style | Slow-path blank lines removed | Nit | Verdict: The optimization is correct. The two moderate gaps (slow-path test coverage) should be addressed before merge per the project's own contributing guidelines. The module doc update is a quick fix. Generated with Claude Code