perf(trap): look up trap table addresses without copying or scanning the protected-code ranges - #1419
Conversation
…d-code ranges trap_table_address snapshotted the bus's protected-code ownership on every trap dispatch, cloning the readonly range list into a fresh Vec so the check closure could outlive an immutable borrow it did not need: the whole lookup holds the bus immutably. On a 3 in Three headless replay the lookup was 22% of host self time with another 6% in the memmove under it. Answer the check from the live ranges through a borrowed accessor; the snapshot stays for install_trap_address, whose memory closure needs the mutable bus.
…rship by binary search Every materialized trap slot registers a protected range (trampoline, gateway, come-from head), so the list grows to about a thousand entries, and since the unified runtime every trap dispatch asks whether each chain hop is a Systemless-owned head: a linear scan over the whole list per hop. Keep the list sorted by start with overlapping and touching entries merged, so coverage and overlap questions are one binary search each. Answers are unchanged: coverage of a contiguous interval by the union of ranges is coverage by the single merged range that contains its start.
|
Per-step CPU time, re-measured with the machine idle (3 in Three headless replay, 200 M guest instructions, 3 pairs each, identical ticks and framebuffer hash on every pair; each step against the previous one in the order the fixes were built):
All four together vs rebased #1220 (5 pairs, both workloads): 3 in Three −60.7% instructions / −53.0% CPU time; SimCity 2000 −39.1% / −30.8%. Note on the first row: removing the per-dispatch clone alone lowers the instruction count but costs CPU time (the borrowed check still walks the whole range list, now without the copy that had just pulled it into cache). The sorted-ranges commit removes that walk, so #1419 is measured as the pair; a run of #1419's two commits on their own is queued and will be posted here. |
benletchford
left a comment
benletchford
left a comment
There was a problem hiding this comment.
Reviewed the range insertion and binary-search invariants, all range writers, 24-bit translation, and protected longword boundaries. The immutable borrow removes the dispatch-time copy without changing ownership checks. Tested this together with #1420 and #1220 on current master: 4,987 library tests passed, three ignored; CI is green.
|
This PR's two commits on their own (on rebased #1220, against rebased #1220), 5 pairs, identical ticks and framebuffer hash on every pair:
The 3 in Three pairs ran on an idle machine; during the SimCity 2000 pairs macOS PDF indexing was using about 1.6 cores, so treat that CPU-time figure as slightly pessimistic. |
2 participants
Summary
Two fixes to the per-dispatch trap-table lookup that #1364 introduced, keeping its provenance check intact:
trap_table_addresssnapshotted the bus's protected-code ownership on every trap dispatch, cloning thereadonly_code_rangesVecso the check closure could outlive an immutable borrow it did not need (the whole lookup holds the bus immutably). It now answers the check from the live ranges through a borrowed accessor; the snapshot stays forinstall_trap_address, whose memory closure needs the mutable bus.is_protected_headon every hop of every trap-table chain walk, byreadonly_code_containson protected writes, and byreadonly_code_overlapson every guest write. Every materialized trap slot registers a range (trampoline, gateway, come-from head), so the list holds on the order of a thousand entries and each query was a linear scan. The list is now kept sorted by start with overlapping and touching entries merged on insert, and all three queries are one binary search. Answers are unchanged: coverage of a contiguous interval by the union of ranges is coverage by the single merged range containing its start, and the last range starting before an interval's end has the largest stop. A unit test checks merged insertion and compares both queries against a brute-force union over every interval in a synthetic layout.Why
Re-baselining perf work against current
mastershowed it executing several times the host work of a tree based on 3c63ca4. Bisect (3 in Three headless replay, 200 M guest instructions, single runs): 5d87791 = 105.9 B host instructions, eef89a3 (#1364) = 268.0 B, 305027d (#1362) = 308.6 B, 83af901 = 308.5 B. Asampleprofile ofmaster(with #1220 rebased on it, so the single-step effect is excluded) puttrap_table_addressat 22% of self time with another 6% in thememmoveunder it.Measurements
Paired hardware counters on the headless replay (
--max-instructions, scripted input where the game needs it), both arms interleaved, identical guest instructions, ticks and framebuffer hash on every pair; the medians of the pair ratios:Absolute, 3 in Three: 246.7 B → 128.9 B host instructions for the second step. Cycles and CPU time moved the same direction but the machine was loaded during these runs, so the instruction counts are the numbers to trust.
Together with the routing fast path and the themed-chrome cache (separate PRs), 3 in Three goes from 308.5 B on
masterto 128.9 B; the pre-#1364 tree is 105.9 B.Measured on builds based on c40588f, the
masterof the day; the branch is rebased onto d30234c, whose seven commits do not touch these paths.The per-step instruction counts above were measured while the machine was loaded (cycles and CPU time were noise); the combined row was re-measured with the machine idle, and per-step CPU times from the same idle rerun follow in a comment.
Related fixes for the same regression: #1419 (trap-table lookup), #1420 (routing fast path), #1421 (themed chrome cache); #1220 removes the single-step mode they were measured on top of.
Tests
cargo test: the new unit test plus the existing suite; one failure,step_frame_forces_render_after_same_tick_foreground_progress, fails identically on untouchedmaster(c40588f) here.🤖 Generated with Claude Code
https://claude.ai/code/session_01UqCuD9vsGeeij5DdYt3vcK