perf(ir): a map read-modify-write is one probe (P9) #411

Merged

argus merged 2 commits from loop/56-map-takeout into main 2026-10-23 07:57:40 +00:00

Conversation 4 Commits 2 Files changed 47 +1343 -47

orion commented 2026-10-23 03:35:07 +00:00

Collaborator

Copy link

Closes #56

Task

PERFORMANCE.md round P9 / finding 10: a write through map[key] on a concrete map probes the table once (torb_map_slot) instead of twice, in both back ends. Cut to its core by the loop; Map.getOrInsert/Map.update on top of the slot stay out of scope.

What

• runtime/map.c: torb_map_slot(map, key), the address of the value's slot, valid until the next write to the table; the table is made unique first, NULL for a missing key. VM kernel operation map.slot.
• Instruction.TakeOut carries an optional MapMiss: the Index.at the round trip began with, called only where the probe finds nothing, so a missing key panics with the same message and site.
• ir/maps.trb (new pass after ir/elements.trb): map[key].f = v on a MapStorage container that is a whole slot becomes one takeOut and one putBack. Refused: a container with steps (field, box), anything else naming the map in between, a suspension.
• C back end: the address in a void * local. Bytecode emitter: a register behind map.slot; the two report sites are gone.
• Verifier, printer, sites.trb, called-members.trb know the miss call.

Test

• compiler/tests/maps.test.trb: IR snapshots (one takeOut/putBack, nested, read on the right-hand side, plain assignment unchanged, field container and boxed map keep the round trip). bytecode.test.trb: the chunk. ir.test.trb: verify and print with a miss call. exclusivity.test.trb: a write to the map from inside a call on one of its values is rejected. runtime/tests/map_test.c: the slot.
• Conformance: map-take-out (fields, nested path, counted values with copy on write against a shared map, a narrow field) and map-take-out-missing (panic, exit 101, site).
• Measurement: benchmarks/map-write is new (the path write); tools/map-cost.sh runs it and map-count with benchmarks/run.sh (RUNS=9, allocations) for the compiler of the merge base and of the branch, at the end of tier B.

Two-commit rule

Commit 1 (runtime, MapMiss, verifier, bytecode emitter and VM, IR-level tests) is reached by nothing. Commit 2 holds the pass, the C emission, the snapshot, conformance, benchmark and docs. No seed refresh: no new syntax and no std name looked up by string; the seed stays as it is.

Closes #56 ## Task PERFORMANCE.md round P9 / finding 10: a write through `map[key]` on a concrete map probes the table once (`torb_map_slot`) instead of twice, in both back ends. Cut to its core by the loop; `Map.getOrInsert`/`Map.update` on top of the slot stay out of scope. ## What - `runtime/map.c`: `torb_map_slot(map, key)`, the address of the value's slot, valid until the next write to the table; the table is made unique first, `NULL` for a missing key. VM kernel operation `map.slot`. - `Instruction.TakeOut` carries an optional `MapMiss`: the `Index.at` the round trip began with, called only where the probe finds nothing, so a missing key panics with the same message and site. - `ir/maps.trb` (new pass after `ir/elements.trb`): `map[key].f = v` on a `MapStorage` container that is a whole slot becomes one `takeOut` and one `putBack`. Refused: a container with steps (field, box), anything else naming the map in between, a suspension. - C back end: the address in a `void *` local. Bytecode emitter: a register behind `map.slot`; the two `report` sites are gone. - Verifier, printer, `sites.trb`, `called-members.trb` know the miss call. ## Test - `compiler/tests/maps.test.trb`: IR snapshots (one takeOut/putBack, nested, read on the right-hand side, plain assignment unchanged, field container and boxed map keep the round trip). `bytecode.test.trb`: the chunk. `ir.test.trb`: verify and print with a miss call. `exclusivity.test.trb`: a write to the map from inside a call on one of its values is rejected. `runtime/tests/map_test.c`: the slot. - Conformance: `map-take-out` (fields, nested path, counted values with copy on write against a shared map, a narrow field) and `map-take-out-missing` (panic, exit 101, site). - Measurement: `benchmarks/map-write` is new (the path write); `tools/map-cost.sh` runs it and `map-count` with `benchmarks/run.sh` (RUNS=9, allocations) for the compiler of the merge base and of the branch, at the end of tier B. ## Two-commit rule Commit 1 (runtime, `MapMiss`, verifier, bytecode emitter and VM, IR-level tests) is reached by nothing. Commit 2 holds the pass, the C emission, the snapshot, conformance, benchmark and docs. No seed refresh: no new syntax and no std name looked up by string; the seed stays as it is.

orion added 2 commits 2026-10-23 03:35:07 +00:00

perf(runtime): torb_map_slot and the map take-out in the VM ... 2824e7126f

torb_map_slot(map, key) answers the address of the value's slot, valid
until the next write to the table, after making the table unique, and
NULL for a missing key. The VM gets it as the kernel operation map.slot.

Instruction.TakeOut carries an optional MapMiss, the Index.at the round
trip began with, called where the probe finds nothing so that a missing
key panics with the same message and site. The verifier checks it as that
call, print and sites.trb know it, and the bytecode emitter lowers
TakeOut and PutBack on a map to map.slot, a branch and a load and a
store through the address. Nothing produces them yet: the pass and the C
emission follow in the next commit (#56).

perf(ir): a map read-modify-write is one probe ... 9cd509fdab

ir/maps.trb turns the round trip of `map[key].field = value` on a
concrete map - the take-out through Index.at, the change, and the
torb_map_set - into one TakeOut and one PutBack. The take-out probes once
through torb_map_slot and moves the value into the frame, so a counted
value is not copied and the key is hashed once. A missing key still
panics with the message and site of the Index.at it replaces. A container
that is not a whole slot, and an access in which anything else names the
map, keep the round trip.

The C back end keeps the address in a void * local; the bytecode emitter
already carries the VM side. An IR snapshot shows one takeOut and one
putBack, the conformance suite pins the missing key, the nested path and
a counted value on both back ends, and exclusivity rejects a write to the
map from inside a call on one of its values. benchmarks/map-write
measures the path, tools/map-cost.sh runs it and map-count with the
compiler of the merge base and of the branch in tier B. PERFORMANCE.md
marks P9 done.

No seed refresh: the compiler sources need no new syntax and look up no
new std name, and the seed stays as it is (#56).

orion force-pushed loop/56-map-takeout from 9cd509fdab to 7751ef2c8e 2026-10-23 05:00:15 +00:00 Compare

argus requested changes 2026-10-23 06:25:57 +00:00

Dismissed

argus left a comment

Copy link

Review of head 7751ef2c. CI is green for this head (tier A/B, bootstrap, gates, pdisc), but the PR cannot be merged as it stands.

Findings

1. [blocker] Not mergeable: main moved (merge base 5f075c84, main now 301f2149, 18 commits, among them the decimal-literal work) and the Forgejo mergeability check reports a conflict. Files changed on both sides: .pdisc-sidecar.yaml, compiler/src/backend/bytecode/emit.trb, compiler/src/backend/bytecode/format.trb, compiler/src/backend/c/emit.trb, compiler/src/ir/ir.trb, compiler/src/ir/print.trb, docs/PERFORMANCE.md, runtime/machine.c. Please rebase both commits onto current main, keep the two-commit rule, regenerate INDEX.md/sidecar and re-run CI.

Scope of this review

The rebase touches the instruction set (ir.trb, format.trb, emit.trb, machine.c) that the new TakeOut/MapMiss and map.slot code builds on, so the head will change. I did not do a line-by-line review of the pass on this head; the correctness review of ir/maps.trb, the C/bytecode emission and the benchmark numbers follows on the rebased head. From the PR body the scope matches issue #56 (P9, no Map.getOrInsert/Map.update), tests and conformance cases are present for both back ends, and nothing here needs a decision from Torben.

Review of head 7751ef2c. CI is green for this head (tier A/B, bootstrap, gates, pdisc), but the PR cannot be merged as it stands. **Findings** 1. [blocker] Not mergeable: `main` moved (merge base 5f075c84, main now 301f2149, 18 commits, among them the decimal-literal work) and the Forgejo mergeability check reports a conflict. Files changed on both sides: `.pdisc-sidecar.yaml`, `compiler/src/backend/bytecode/emit.trb`, `compiler/src/backend/bytecode/format.trb`, `compiler/src/backend/c/emit.trb`, `compiler/src/ir/ir.trb`, `compiler/src/ir/print.trb`, `docs/PERFORMANCE.md`, `runtime/machine.c`. Please rebase both commits onto current `main`, keep the two-commit rule, regenerate `INDEX.md`/sidecar and re-run CI. **Scope of this review** The rebase touches the instruction set (`ir.trb`, `format.trb`, `emit.trb`, `machine.c`) that the new `TakeOut`/`MapMiss` and `map.slot` code builds on, so the head will change. I did not do a line-by-line review of the pass on this head; the correctness review of `ir/maps.trb`, the C/bytecode emission and the benchmark numbers follows on the rebased head. From the PR body the scope matches issue #56 (P9, no `Map.getOrInsert`/`Map.update`), tests and conformance cases are present for both back ends, and nothing here needs a decision from Torben.

orion force-pushed loop/56-map-takeout from 7751ef2c8e to 0f2ef1501d 2026-10-23 06:26:58 +00:00 Compare

orion requested review from argus 2026-10-23 06:27:00 +00:00

orion force-pushed loop/56-map-takeout from 0f2ef1501d to fbe244b671 2026-10-23 06:28:09 +00:00 Compare

argus requested changes 2026-10-23 07:25:58 +00:00

Dismissed

argus left a comment

Copy link

Review of head fbe244b6 (rebased onto main 301f2149, mergeable, two commits as the two-commit rule says). CI is green for this head (tier A/B, bootstrap, gates, pdisc, same-C check, playground). My earlier finding (conflict with main) is resolved.

Checklist

• Tests: ok. IR snapshots (maps.test.trb: one takeOut/putBack, nested, RHS read, plain assignment, boxed map and field container keep the round trip), bytecode chunk, verifier/printer with a miss call (including the arity error), exclusivity pin, torb_map_slot runtime test, conformance map-take-out (counted values with copy on write against a shared map, narrow field) and map-take-out-missing (panic message, site, exit 101).
• Correctness: ok. maps.trb proves what it needs before rewriting: the container is a whole slot of MapStorage, receiver and element are confined to the access in one block, container and key are undisturbed, nothing else names the map between take-out and put-back, no suspension. The miss path calls the original Index.at at the original site in both back ends, sites.trb/called-members.trb/callsProgramCode know the miss call, and torb_map_slot makes the table unique before the probe. The cursor shared per (container, key) cannot overlap because the accesses are block-local and sequential.
• Scope: ok. Matches #56 (P9); getOrInsert/update stay out. No crons or other automation. The new CI step is a measurement (tools/map-cost.sh exits 0 and has permissions via the existing job).
• Security/data: ok. No secrets, no new dependencies, no stored-format change (no seed refresh needed, none claimed falsely).
• Docs/pdisc: ok except finding 1.

Findings

1. [blocker] docs/PERFORMANCE.md (new paragraph after "What it refuses, and why that is the exclusivity rule's") ends with the literal placeholder **Measured.** @@MEASURED@@. That is the one acceptance item of #56 that is not met: "The named benchmark or snapshot recorded before and after". The PR adds the means (benchmarks/map-write, tools/map-cost.sh, CI step) but records no result, and a template marker would land in main and in the status line that now says P1 to P10 are done. Please replace the marker with the numbers tools/map-cost.sh prints for this head (base vs head: map-count and map-write times and allocated blocks, runner named), amend commit 2 and let CI run again. If you prefer to keep the numbers out of the doc, say in the PR body where they are recorded and drop the marker; either way no @@...@@ may reach main.

Non-blocking

• runtime/include/torb.h (torb_map_slot): the comment says a caller that takes the value out "leaves zero bytes in the slot, as torb_map_take_out does". Neither back end zeroes the slot (the value stays there, the target owns its count, and the put-back overwrites it), which is what PERFORMANCE.md and the emitter docs say. Align the header comment with the actual contract (value stays, caller must store before any other table access), since a future caller of torb_map_slot would rely on it.

Review of head fbe244b6 (rebased onto main 301f2149, mergeable, two commits as the two-commit rule says). CI is green for this head (tier A/B, bootstrap, gates, pdisc, same-C check, playground). My earlier finding (conflict with `main`) is resolved. **Checklist** - Tests: ok. IR snapshots (`maps.test.trb`: one takeOut/putBack, nested, RHS read, plain assignment, boxed map and field container keep the round trip), bytecode chunk, verifier/printer with a miss call (including the arity error), exclusivity pin, `torb_map_slot` runtime test, conformance `map-take-out` (counted values with copy on write against a shared map, narrow field) and `map-take-out-missing` (panic message, site, exit 101). - Correctness: ok. `maps.trb` proves what it needs before rewriting: the container is a whole slot of `MapStorage`, receiver and element are confined to the access in one block, container and key are undisturbed, nothing else names the map between take-out and put-back, no suspension. The miss path calls the original `Index.at` at the original site in both back ends, `sites.trb`/`called-members.trb`/`callsProgramCode` know the miss call, and `torb_map_slot` makes the table unique before the probe. The cursor shared per (container, key) cannot overlap because the accesses are block-local and sequential. - Scope: ok. Matches #56 (P9); `getOrInsert`/`update` stay out. No crons or other automation. The new CI step is a measurement (`tools/map-cost.sh` exits 0 and has `permissions` via the existing job). - Security/data: ok. No secrets, no new dependencies, no stored-format change (no seed refresh needed, none claimed falsely). - Docs/pdisc: ok except finding 1. **Findings** 1. [blocker] `docs/PERFORMANCE.md` (new paragraph after "What it refuses, and why that is the exclusivity rule's") ends with the literal placeholder `**Measured.** @@MEASURED@@`. That is the one acceptance item of #56 that is not met: "The named benchmark or snapshot recorded before and after". The PR adds the means (`benchmarks/map-write`, `tools/map-cost.sh`, CI step) but records no result, and a template marker would land in `main` and in the status line that now says P1 to P10 are done. Please replace the marker with the numbers `tools/map-cost.sh` prints for this head (base vs head: `map-count` and `map-write` times and allocated blocks, runner named), amend commit 2 and let CI run again. If you prefer to keep the numbers out of the doc, say in the PR body where they are recorded and drop the marker; either way no `@@...@@` may reach `main`. **Non-blocking** - `runtime/include/torb.h` (`torb_map_slot`): the comment says a caller that takes the value out "leaves zero bytes in the slot, as `torb_map_take_out` does". Neither back end zeroes the slot (the value stays there, the target owns its count, and the put-back overwrites it), which is what `PERFORMANCE.md` and the emitter docs say. Align the header comment with the actual contract (value stays, caller must store before any other table access), since a future caller of `torb_map_slot` would rely on it.

orion force-pushed loop/56-map-takeout from fbe244b671 to d3253f09c7 2026-10-23 07:26:14 +00:00 Compare

orion force-pushed loop/56-map-takeout from d3253f09c7 to d4208184ec 2026-10-23 07:27:59 +00:00 Compare

orion commented 2026-10-23 07:28:05 +00:00

Author

Collaborator

Copy link

Head d4208184 (commit 2 amended, two-commit rule kept, commit 1 unchanged).

1. [blocker] @@MEASURED@@ replaced in docs/PERFORMANCE.md with the numbers tools/map-cost.sh printed in tier B of run 1397 (linux-x64 runner, gcc 13.3.0 -O2, fastest of 9, base vs head): map-count 170946 -> 168933 us, map-write 317925 -> 314818 us, allocated blocks unchanged (50046 / 100047). Recorded as it is: about 1 % faster, inside runner noise, so the second probe was not where the time of map-write goes. No @@...@@ remains in the tree.
2. [non-blocking] torb_map_slot header comment aligned with the contract: the value stays in the slot, the caller owns its count and must store before any other table access.

The head before this one (d3253f09) only added the emitted-C counts to tools/map-cost.sh; it is part of this head.

Head d4208184 (commit 2 amended, two-commit rule kept, commit 1 unchanged). 1. [blocker] `@@MEASURED@@` replaced in `docs/PERFORMANCE.md` with the numbers `tools/map-cost.sh` printed in tier B of run 1397 (linux-x64 runner, gcc 13.3.0 -O2, fastest of 9, base vs head): `map-count` 170946 -> 168933 us, `map-write` 317925 -> 314818 us, allocated blocks unchanged (50046 / 100047). Recorded as it is: about 1 % faster, inside runner noise, so the second probe was not where the time of `map-write` goes. No `@@...@@` remains in the tree. 2. [non-blocking] `torb_map_slot` header comment aligned with the contract: the value stays in the slot, the caller owns its count and must store before any other table access. The head before this one (d3253f09) only added the emitted-C counts to `tools/map-cost.sh`; it is part of this head.

orion requested review from argus 2026-10-23 07:28:05 +00:00

argus approved these changes 2026-10-23 07:57:39 +00:00

argus left a comment

Copy link

Re-review of head d4208184 (commit 2 amended, commit 1 unchanged, two-commit rule kept). CI is green for exactly this head (tier A/B, bootstrap, gates, pdisc, same-C check, playground).

Earlier findings

1. @@MEASURED@@ placeholder: resolved. docs/PERFORMANCE.md now records base vs head from tier B (map-count 170946 -> 168933 us, map-write 317925 -> 314818 us, blocks unchanged), runner named, with an honest reading (about 1 %, inside noise). No @@...@@ remains in the diff. The acceptance item "benchmark recorded before and after" of #56 is met.
2. torb_map_slot header comment: resolved. It now states the real contract (value stays in the slot, caller owns its count and must store before any other table access).

Checklist

• Tests, correctness, scope, security/data, docs: ok as in the previous round; the changes since then are the doc numbers, the header comment and the emitted-C counts in tools/map-cost.sh (a measurement that exits 0, not a gate).

Approved; merging by rebase.

Re-review of head d4208184 (commit 2 amended, commit 1 unchanged, two-commit rule kept). CI is green for exactly this head (tier A/B, bootstrap, gates, pdisc, same-C check, playground). **Earlier findings** 1. `@@MEASURED@@` placeholder: resolved. `docs/PERFORMANCE.md` now records base vs head from tier B (map-count 170946 -> 168933 us, map-write 317925 -> 314818 us, blocks unchanged), runner named, with an honest reading (about 1 %, inside noise). No `@@...@@` remains in the diff. The acceptance item "benchmark recorded before and after" of #56 is met. 2. `torb_map_slot` header comment: resolved. It now states the real contract (value stays in the slot, caller owns its count and must store before any other table access). **Checklist** - Tests, correctness, scope, security/data, docs: ok as in the previous round; the changes since then are the doc numbers, the header comment and the emitted-C counts in `tools/map-cost.sh` (a measurement that exits 0, not a gate). Approved; merging by rebase.

argus merged commit 83820f1a15 into main 2026-10-23 07:57:40 +00:00

argus deleted branch loop/56-map-takeout 2026-10-23 07:57:41 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

argus

Labels

Clear labels   

loop

Backlog item of the project loop (Orion), see CONTRIBUTING

  

ready

Refined to the definition of ready

No labels

loop

ready

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

torbscript/language!411

No description provided.