Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ The generated loop is now written from the program's source alone, and the manif

### Fixed

- **The VM and generated Rust set a Vector in a field of a record in place when the match on `Vector.set` hands the record back whole.** In `match Vector.set(s.cells, i, x)` with `Option.Some(updated) -> State.update(s, cells = updated, …)` and `Option.None -> s`, the `None` arm still needs `s.cells`, so every set copied the whole Vector, even with `s` held by nothing else. The `None` arm runs only when the index is out of range, and then the set changes nothing, so the index is now checked first and the Vector leaves the record only in the `Some` arm. Generated Rust moves the field there. The VM takes it out of the record only when the record is held by exactly the holders the compiler accounted for, and writes it in place only when nothing else holds it. `aver check` no longer reports this shape as `perf-shared-update`. In a release build, 2000 sets of a 100,000-cell Vector held by a record run in 0.04 s instead of 0.47 s on the VM and 0.03 s instead of 0.20 s in generated Rust, and 20,000 sets in 0.04 s instead of 5.0 s and 0.03 s instead of 1.8 s. A Vector two records down (`o.inner.cells`) is still copied on both, and wasm-gc still copies it.
- **Generated Rust hands a record to functions that tail-call each other without copying it.** The public function of such a group borrowed a record or collection argument and cloned it into the loop that runs the group, so while the caller still held its copy, the first `Map.set` on a Map inside it copied the whole Map, once per call. It now takes the argument by value, and a caller at its last use moves it in; a caller that keeps it clones at the call, as the function did before. An argument every function of the group passes on unchanged is still borrowed. A loop handing a pool with a million-entry Map to such a pair 100 times ran in 0.64 s and now runs in 0.07 s.
- **`perf-shared-update` reports the copies generated code makes, and only those.** The check now reads the same field moves the Rust backend makes, from the module lowered as it compiles. `done = toppedUp(s.pool, s.book, s.nextKey)?` followed by `S.update(s, pool = done.pool, book = done.book, nextKey = done.nextKey)` moves the Book out of `s` and no longer warns. `match (s.window.created, s.height)` with `(created, _) -> f(s, absorbed(created, k))` copies the Map, since `s` still holds it, and now warns: "`absorbed` updates `created`, read from `s.window.created`, a Map that is still held by `s`". A field of a record a loop hands on unchanged is reported too.
- **Generated Rust moves the rest of a nested record into its update.** `Setting.update(setting, window = Window.update(setting.window, created = Map.set(setting.window.created, k, v)), height = setting.height + 1)` used to clone `setting.window` and `setting.window.created`, so every `Map.set` copied the Map. When nothing reads that part of `setting` again, the Map now moves into `Map.set` and the other fields of `setting.window` move into the new `Window`.
Expand Down
17 changes: 17 additions & 0 deletions aver-memory/src/arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ impl<T: ArenaTypes> Arena<T> {
list_elements_scanned: 0,
list_elements_flattened: SharedCount::default(),
map_entries_copied: 0,
vector_elements_copied: 0,
map_entries_scanned: 0,
vector_elements_scanned: 0,
out_of_region_entries_read: 0,
Expand Down Expand Up @@ -75,6 +76,7 @@ impl<T: ArenaTypes> Arena<T> {
list_elements_scanned: 0,
list_elements_flattened: SharedCount::default(),
map_entries_copied: 0,
vector_elements_copied: 0,
map_entries_scanned: 0,
vector_elements_scanned: 0,
out_of_region_entries_read: 0,
Expand Down Expand Up @@ -742,6 +744,20 @@ impl<T: ArenaTypes> Arena<T> {
self.map_entries_copied += entries as u64;
}

/// Vector elements `Vector.set` duplicated to preserve a target it was not
/// allowed to write in place. Per-arena, as [`Arena::map_entries_copied`].
#[inline]
pub fn vector_elements_copied(&self) -> u64 {
self.vector_elements_copied
}

/// Record that `elements` vector elements were duplicated to preserve a
/// target the caller was not allowed to write in place.
#[inline]
pub fn note_vector_elements_copied(&mut self, elements: usize) {
self.vector_elements_copied += elements as u64;
}

/// Add `child`'s copy / scan totals to this arena's.
///
/// A child arena counts from zero ([`Arena::clone_static`]), so work an
Expand All @@ -756,6 +772,7 @@ impl<T: ArenaTypes> Arena<T> {
self.list_elements_flattened
.add(child.list_elements_flattened.get());
self.map_entries_copied += child.map_entries_copied;
self.vector_elements_copied += child.vector_elements_copied;
self.map_entries_scanned += child.map_entries_scanned;
self.vector_elements_scanned += child.vector_elements_scanned;
self.out_of_region_entries_read += child.out_of_region_entries_read;
Expand Down
4 changes: 4 additions & 0 deletions aver-memory/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1491,6 +1491,10 @@ pub struct Arena<T: ArenaTypes> {
/// and [`Arena::absorb_copy_counters`] folds a child's total back into its
/// parent when the branch rejoins.
map_entries_copied: u64,
/// Total vector elements `Vector.set` duplicated because it was not
/// allowed to write its target in place. Per-arena like
/// `map_entries_copied`.
vector_elements_copied: u64,
/// Total map entries the collector has *read* while deciding whether a live
/// map needs rewriting. A map whose `all_immediate` flag is set is
/// returned unread and adds nothing here; a map holding anything
Expand Down
5 changes: 4 additions & 1 deletion src/checker/shared_update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,10 @@ pub fn collect_shared_update_warnings(
let mut bound = HashMap::new();
collect_bound(&f.body.node, &mut bound);
let body = Body {
movable: crate::ir::mir::field_moves::movable_projections(&f.body.node),
movable: crate::ir::mir::field_moves::movable_projections(
&f.body.node,
&program.builtins,
),
carried: carried_params(f),
bound,
};
Expand Down
68 changes: 62 additions & 6 deletions src/codegen/rust/from_mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,9 +487,13 @@ impl MirFnEmitPolicy {
/// Record which field reads in `mir_fn`'s body may move their field
/// out of the record local they read. The facts are addresses into
/// this very body, so the policy must emit `mir_fn.body` itself.
pub(super) fn apply_field_moves(&mut self, mir_fn: &crate::ir::mir::MirFn) {
pub(super) fn apply_field_moves(
&mut self,
mir_fn: &crate::ir::mir::MirFn,
builtins: &[String],
) {
self.movable_projections =
crate::ir::mir::field_moves::movable_projections(&mir_fn.body.node);
crate::ir::mir::field_moves::movable_projections(&mir_fn.body.node, builtins);
self.moved_roots =
crate::ir::mir::field_moves::moved_roots(&mir_fn.body.node, &self.movable_projections);
}
Expand Down Expand Up @@ -735,7 +739,10 @@ pub(super) fn compute_owned_record_params(
program.fn_by_id(*id).map(|mir_fn| {
(
*id,
crate::ir::mir::field_moves::movable_projections(&mir_fn.body.node),
crate::ir::mir::field_moves::movable_projections(
&mir_fn.body.node,
&program.builtins,
),
)
})
})
Expand Down Expand Up @@ -3102,6 +3109,37 @@ fn emit_mir_match_with(
.unwrap_or_default()
};

// `match Vector.set(s.cells, i, x)` whose target may move out of `s`
// (`field_moves::vector_set_match`): evaluate the index and the value,
// check the index, and read the target only in the `Some` arm. The
// `None` arm, which may read `s` whole, then still finds the field there,
// and the `Some` arm hands `set_unchecked` the only reference.
if let Some(set) = crate::ir::mir::field_moves::vector_set_match(m, emit_ctx.mir_builtins)
&& super::ownership::projection_root_local(&set.target.node).is_some_and(|root| {
super::ownership::projection_moves(&set.target.node, root, emit_ctx)
})
{
let index = emit_mir_expr(set.index, emit_ctx)?;
let value = mir_clone_arg(
emit_mir_expr(set.value, emit_ctx)?,
&set.value.node,
emit_ctx,
);
let target = emit_mir_expr(set.target, emit_ctx)?;
let moved = mir_clone_arg(target.clone(), &set.target.node, emit_ctx);
let updated = match set.updated {
Some((_, name)) if name != "_" => aver_name_to_rust(name),
_ => "_".to_string(),
};
let index_temp = generated_ident("idx");
let value_temp = generated_ident("value");
let some = &arm_bodies[set.some_arm];
let none = &arm_bodies[set.none_arm];
return Some(format!(
"{{ let {index_temp} = ({index}).to_usize(); let {value_temp} = {value}; match {index_temp}.filter(|{index_temp}| *{index_temp} < {target}.len()) {{ Some({index_temp}) => {{ let {updated} = {moved}.set_unchecked({index_temp}, {value_temp}); {some} }} None => {{ {none} }} }} }}"
));
}

// ── 1. Single-arm irrefutable → `let` destructuring. ──
// Mirror of `emit_match`'s first branch.
if arms.len() == 1 && resolved_pattern_is_irrefutable(&arms[0].pattern) {
Expand Down Expand Up @@ -3923,7 +3961,13 @@ pub(super) fn emit_mir_fn_body_routed(
// `bare_fn_facts`), so body and signature agree on which params /
// return are bare.
policy.apply_bare_i64(mir_fn.fn_id, ctx);
policy.apply_field_moves(mir_fn);
policy.apply_field_moves(
mir_fn,
ctx.mir_program
.as_ref()
.map(|p| p.builtins.as_slice())
.unwrap_or(&[]),
);
let emit_ctx = MirEmitCtx::for_fn(ctx, &policy);
let body = emit_mir_fn_body(&mir_fn.body, &emit_ctx)?;
let Some(prologue) = post_checkpoint_prologue else {
Expand Down Expand Up @@ -4032,7 +4076,13 @@ pub(super) fn emit_mir_tco_fn(
for n in &rc_names {
policy.owned_params.remove(n);
}
policy.apply_field_moves(mir_fn);
policy.apply_field_moves(
mir_fn,
ctx.mir_program
.as_ref()
.map(|p| p.builtins.as_slice())
.unwrap_or(&[]),
);
let emit_ctx = MirEmitCtx::for_fn(ctx, &policy);

// Render the body in tail position FIRST — bail before emitting any
Expand Down Expand Up @@ -4452,7 +4502,13 @@ pub(super) fn emit_mir_mutual_tco_block(
// Each arm binds its params by value, so a field read may move out
// of one exactly as in any other body; the arm's updates then see
// which records gave a field up.
policy.apply_field_moves(mir_fn);
policy.apply_field_moves(
mir_fn,
ctx.mir_program
.as_ref()
.map(|p| p.builtins.as_slice())
.unwrap_or(&[]),
);
let mut arm_ctx = MirEmitCtx::for_fn(ctx, &policy);
// Mutual invariants are `rc_wrapped` for owning reads, but unlike
// self-TCO's `Arc<T>` representation they are extra `&T` trampoline
Expand Down
Loading
Loading