Set a Vector field in place when the match on Vector.set hands the record back - #1443
Merged
Merged
Conversation
…cord back `match Vector.set(s.cells, i, x)` with `Some(updated) -> State.update(s, cells = updated, ...)` and `None -> s` copied the whole Vector on every set, on the VM and in generated Rust, even with `s` held by nothing else. The field-take work for Map did not reach it: the Vector is read in the match subject, not inside the update, and the None arm reads `s` whole, so every analysis saw a later read of the field. The None arm runs only when the index is out of range, and then the set changes nothing. `field_moves::vector_set_match` names this shape, and `movable_projections` places the target read in the Some arm, so it moves when nothing in the Some arm or after the match needs the field. - Generated Rust evaluates the index and the value, checks the index against the length, and moves the field into `set_unchecked` in the Some arm. - The VM compiles the subject as the new `VECTOR_SET_FIELD` opcode. It answers None for an index out of range without touching the record; otherwise it takes the Vector out of the record only when nothing off the stack holds the record and exactly the expected stack cells do, counted while the index and value are still on the stack, and writes in place only under the same fence as every other in-place vector write. Anything else copies, as `VECTOR_SET` does. - `perf-shared-update` reads the same facts, so it no longer reports this shape. The arena counts the vector elements a `Vector.set` copies, so the tests can assert that a set copied nothing. The new fixture's 2000 and 20,000 sets of a 100,000-cell Vector take 0.04 s instead of 0.47 s and 5.0 s on the VM, and 0.03 s instead of 0.20 s and 1.8 s in generated Rust, in release builds. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A record holds a 100,000-cell Vector, and each step of a tail-recursive loop does
Every backend copied the whole Vector on every set. This PR fixes the VM and generated Rust. wasm-gc follows in a separate PR (Vector versions, as #1433 did for Map).
Why the Map fixes did not cover it
field_movesmovess.cellsonly when no other read that can run with it overlaps it. The read sits in the match subject, and theNonearm readsswhole, so the field was cloned intoset_ownedandRc::make_mutcopied it.s.cellswas a plainRECORD_GET_NAMED. Even a taken field would not have helped: the MIR walker lowers aVector.setit cannot prove owned toVECTOR_SET, which always copies, and theNonearm would have seen an emptied field.The fix
The
Nonearm runs only when the index is out of range, and then the set changes nothing. So a backend that evaluates the index and the value, checks the index, and reads the target only in theSomearm never needs the target in theNonearm.field_moves::vector_set_matchnames the shape: a two-armSome/Nonematch (either order, or_afterSome) onVector.set(path, index, value)wherepathis a field path of a local.movable_projectionsplaces the target read in theSomearm, so the usual rules decide whether it moves. It now takes the builtin table.emit_mir_match_withemitslet idx = index.to_usize(); let value = …; match idx.filter(|i| *i < s.cells.len()) { Some(i) => { let updated = s.cells.set_unchecked(i, value); … } None => { … } }.VECTOR_SET_FIELD field holdersover[record, index, value]. An index out of range answersNoneand leaves the record alone. In range, the Vector leaves the record only when the record is not held elsewhere and exactlyholders + 1stack cells hold it, counted while the index and value are still on the stack. It is then written in place only whenconfirm_vector_grantagrees, the same fence asVECTOR_SET_OR_KEEP. The in-place write moved intostore_vector_element_in_place, shared by both opcodes. Anything else copies, asVECTOR_SETdoes. Onlylocal.fieldis handled on the VM; a deeper path compiles as before.perf-shared-updatereads the samemovable_projections, so the shape no longer warns. ASomearm that reads the old Vector still warns.Arena::vector_elements_copiedcounts elements thatVector.setcopies, the vector counterpart ofmap_entries_copied.Measurements
tests/fixtures/vector_field_set/main.av(the measured program; the step count is the first argument). Release builds, best of 3, wall time:bench/scenarios/vector_ops.tomlon the VM: p50 819 µs before, 850 µs after, which is within noise.Tests
tests/vm_record_field_take.rs: the fixture copies no element. A loop copies nothing, and an index past the end leaves the Vector in the record. TheNone-first arm order also copies nothing. Refusals that must copy and answer correctly: the caller keeps the record; another local names the record; another record holds it; two records share the Vector; a local bound to the old Vector; theSomearm reads the old Vector; the record is kept whole beside the new Vector; the update keeps the base's Vector; the written value holds the record (Vector<Option<Node>>inNode).…held_by_another_record…fails;…caller_still_holds…and…another_local_holds…fail;None-first tests fail;…two_records_share…and…another_local_holds_is_not_written…fail;movable_projectionscheck in the compiler: the old-Vector, kept-whole and update-keeps tests fail.tests/rust_work_spec.rs: the fixture's generatedstepmoves the field in theSomearm and prints what the VM prints.tests/fixtures/vector_field_set_shapescovers the hostile shapes plus a self-TCO loop, a Vector two records down, and a recursive node. It builds as Rust and prints the same as the VM and asmaindid before this change.field_movesunit tests for the shape and for aSomearm that reads the target again.tests/shared_update_spec.rs: the shape does not warn, and the read-back variant still warns.cargo test --workspacepasses locally, includingown_param_soundnessandvm_record_field_take.cargo fmtand all three CI clippy commands pass.python3 tools/regenerate_self_host.py --checkreports the self-host is fresh.Not covered: a Vector two records down (
o.inner.cells) is still copied on both backends, because the Rust move needs the inner update's base to be final and the VM opcode handles one field.