Skip to content

perf(ir_passes): share repeated buffer property reads, descriptors and record addresses - #2044

Merged
nahime0 merged 8 commits into
mainfrom
perf/property-buffer-reuse
Oct 25, 2026
Merged

nahime0 merged 8 commits into
mainfrom
perf/property-buffer-reuse

Conversation

@nahime0

@nahime0 nahime0 commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

Was stacked on #2036, which has merged; main is merged into this branch, so the diff is this PR's own change.

Problem

Inside a method, every $this->slots[$index]->field access reloads the property, calls __rt_buffer_resolve and recomputes the record address. The #1959 hoisting only covers loops whose buffer handle is loop-invariant (locals, parameters); a property read inside the body is neither, and straight-line code is never looked at. Ivory's Entities::matches() (the shape bench/ecs calls once per entity) makes two such reads per call.

Base (main f708b38, debug build) EIR of the task's Table::matches:

entry:
  v0: Heap(Object) php=Table = load_local slot[0]
  v1: Heap(Buffer) php=buffer<packed Slot> = prop_get v0 data[0] ; effects: reads_heap, may_throw
  v2: Heap(Buffer) = acquire v1
  v3: I64 php=int = load_local slot[1]
  v4: Heap(Object) php=packed Slot = buffer_get v2 v3      # call __rt_buffer_resolve + bounds check
  release v2
  v5: I64 php=bool = prop_get v4 data[1]
  cond_br v5, bb1, bb2
logical.rhs:
  v6: Heap(Object) php=Table = load_local slot[0]
  v7: Heap(Buffer) = prop_get v6 data[0]                   # property read again
  v8: Heap(Buffer) = acquire v7
  v9: I64 php=int = load_local slot[1]
  v10: Heap(Object) php=packed Slot = buffer_get v8 v9     # resolved again, same record
  release v8
  v11: I64 php=int = prop_get v10 data[2]

On linux-x86_64 each prop_get is a null-receiver test, a typed-property initialization test and a load, and each buffer_get spills the handle and index around call __rt_buffer_resolve.

Change

A new straight-line phase src/ir_passes/buffer_property_reuse.rs runs inside buffer_access after loop hoisting (same generator, exception-handler, eval and pcntl_async_signals() guards). Three rewrites, each only across a clean region (every instruction on any path from the earliest instruction either access depends on to the later access):

  1. A dominated buffer-typed prop_get of the same property on a matching receiver is replaced by the dominating read.
  2. A dominated packed buffer_get of the same property-read handle and a matching index reuses the dominating record address.
  3. The remaining accesses of one property-read handle that a leading access dominates share one non-faulting buffer_resolve inserted right before the leader and become buffer_get_resolved / buffer_set_resolved (so a stale handle still dies with the use-after-free fatal at the leader).

Only property reads whose receiver is the method's own $this are trusted, as candidates and inside a region: effect refinement drops the null-receiver warning of any other receiver (a nullable one, or a non-nullable parameter a dynamic call passed null; filed as task b153a540), and that warning can run a user error handler that replaces or frees the buffer. Modules where a class declares __get, or where the eval bridge can declare one, skip the phase, so no trusted read dispatches to user code.

The region classifier is default-deny: the loop pass's buffer-preserving allowlist minus every opcode that can run user code (even a call with a read-only effect summary, since summaries drop the callee's local effects and matched slots are not proven free of address escapes), plus the no-op acquire/release of a buffer handle and named property reads refined to reads_heap, may_throw. A property store or unset on any object (another reference may alias the receiver), buffer_free(), a call, a release, an allocation, a warning or output ends the region. Receivers and indexes match when they are one SSA value (seen through trivial join parameters), equal integer constants, or loads of one stable slot (a by-value PHP local that only load_local/store_local name) that the region does not write. A buffer acquire left with nothing but its release (both emit no code for buffers) is dropped with it. Local buffer handles in straight-line code are left alone.

Expected EIR of Table::matches after the change (asserted by the tests): one prop_get of slots, one buffer_get, no buffer_resolve; logical.rhs keeps only the field read prop_get v4 data[2] on the first record address. A method touching two indexes of one property (swapMasks) gets one buffer_resolve and two buffer_get_resolved.

Small shared-code changes in src/ir_passes/buffer_access.rs (from #2036): optimize_function now calls the loop hoisting (hoist_loops) and then the new phase, and instruction_preserves_buffers, slots_named_as_storage, insert_instruction, incoming_arguments and canonical_value become pub(super).

Tests

  • src/ir_passes/tests/buffer_property_reuse_test.rs (hand-built EIR): shared property read and record address, one descriptor for two indexes, and the reads stay apart across a property store through another reference, unset, buffer_free(), a read-only user call, a property read on another receiver, an index write (descriptor still shared, records not), another receiver, functions without $this, and local handles.
  • tests/codegen/optimizer/buffer_property_reuse.rs (EIR shape plus optimizer on/off runtime parity): Table::matches, Table::swapMasks, the same reads inside a loop body (countMatching), an aliasing property store ($alias->cells = ... with $alias === $this), an index write, buffer_free() or a freeing call between the reads, which must keep the use-after-free fatal in both modes, a null-receiver warning whose error handler replaces or frees the buffer between the reads, and a program declaring __get, which turns the phase off. The fixture outputs were checked against the base compiler in both modes.
  • New example examples/buffer-property-table/main.php (an ECS-style slot table), compiled and run by test_buffer_property_table_example_shares_property_reads.

Docs: docs/internals/the-optimizer.md, docs/compiling/optimization.md, docs/compiling/source-maps.md.

Limitations

  • Reads through any receiver other than $this are left alone. Once a method such as Entities::matches() is inlined into a caller's loop, its receiver becomes the caller's $entities parameter, which a dynamic call can set to null, so those inlined reads are not shared until effect refinement proves receivers non-null (task b153a540).
  • A loop over $this->slots[$i] still resolves per access across iterations (this PR shares the work within one iteration); hoisting the property read out of such loops is task ad0374ba.

No benchmark was run locally (shared machine); the EIR shape above is the evidence, and a benchmark run (Ivory bench/ecs) is left to CI or the maintainer.

@github-actions github-actions Bot added area:eir Touches EIR definitions, lowering, validation, or passes. area:optimizer Touches AST or EIR optimization passes. size:m Medium-sized pull request. type:triage Needs a conventional title or branch prefix before review. labels Oct 24, 2026
@github-actions github-actions Bot added size:l Large pull request. and removed size:m Medium-sized pull request. labels Oct 24, 2026
@nahime0
nahime0 marked this pull request as ready for review October 24, 2026 08:08
@greptile-apps

greptile-apps Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable new issues remain.

Summary

Shares repeated buffer-property reads, packed record addresses, and buffer descriptors within clean regions of a method.

  • Methods reuse buffer-property reads within a safe stretch of code.

Review checks were based on source inspection; tests were not run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Loop hoisting] --> B[Share matching property reads]
    B --> C[Share matching packed record addresses]
    C --> D[Share descriptors for remaining accesses]
    D --> E[Drop unused buffer handle copies]
Loading

Reviews (3) · Last reviewed commit: "chore: merge main into perf/property-buf..." · Reviewed by Greptile

Comment thread src/ir_passes/buffer_property_reuse.rs
Comment thread src/ir_passes/buffer_property_reuse.rs
@nahime0
nahime0 added this pull request to the merge queue Oct 24, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to invalid changes in the merge commit Oct 24, 2026
@nahime0
nahime0 added this pull request to the merge queue Oct 24, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to invalid changes in the merge commit Oct 24, 2026
@github-actions github-actions Bot added size:m Medium-sized pull request. and removed size:l Large pull request. labels Oct 24, 2026
@nahime0
nahime0 added this pull request to the merge queue Oct 24, 2026
@nahime0
nahime0 removed this pull request from the merge queue due to a manual request Oct 25, 2026
@nahime0
nahime0 added this pull request to the merge queue Oct 25, 2026
Merged via the queue into main with commit 3a65464 Oct 25, 2026
148 checks passed
@nahime0
nahime0 deleted the perf/property-buffer-reuse branch October 25, 2026 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:eir Touches EIR definitions, lowering, validation, or passes. area:optimizer Touches AST or EIR optimization passes. size:m Medium-sized pull request. type:triage Needs a conventional title or branch prefix before review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant