Make property resolution direction-aware; amortize asymmetric visibility set checks into the runtime cache

[hollyschilling]

Summary

Writes to private(set) / protected(set) properties currently pay a per-write visibility re-check — zend_asymmetric_property_has_set_access() walks to the executing scope on every assignment — because set-visibility cannot be amortized into the runtime cache the way get-visibility is: zend_get_property_offset() doesn't know whether it is resolving for a read or a write.

This PR makes property resolution direction-aware and moves the asymmetric set check to cache-population time. After the first resolution at a call site, writes to asymmetric properties become identical in cost to public typed-property writes.

Mechanics

• zend_get_property_offset() gains a write_access flag (it is always_inline and every caller passes a constant, so the parameter folds away). The write handler, unset, and get_property_ptr_ptr resolve with write access; read/isset resolve without.
• Write-kind resolution performs the ZEND_ACC_PPP_SET_MASK check at population. When the running scope lacks set access, the property still resolves exactly as today — the caller's state-dependent slow path keeps deciding between the visibility error and the __set fallback — but the runtime cache slot is left unpopulated, and its key is invalidated (the hooked simple-write marking ORs a flag into the slot assuming resolution populated it; without invalidation a stale polymorphic entry could be corrupted).
• Invariant: a populated ZEND_ASSIGN_OBJ cache slot guarantees set access. The ZEND_ASSIGN_OBJ cache-hit path therefore uses a new zend_assign_to_typed_prop_granted() that skips the per-write scope walk, with a debug ZEND_ASSERT enforcing the invariant.
• Optimizer support (added after review, 495d68dfba): the literal-compaction pass shares one runtime cache slot among all $this property opcodes with the same property name, which would let a read-kind (unchecked) resolution warm the slot an ASSIGN_OBJ then trusts — the bypass found by @iluuu1994 below. ZEND_ASSIGN_OBJ now has its own slot-sharing pool: its misses populate exclusively through the set-checked write handler, so sharing among assign oplines preserves the invariant. All other property opcodes keep the common pool; their fast paths still check per operation.
• The __set-fallback semantics are preserved structurally: the fast path only runs when the slot value is defined (Z_TYPE != IS_UNDEF); every unset-state write already goes to the handler, which keeps the full state-dependent handling.

Soundness rests on: per-opline cache slots are scope-stable (an opline belongs to one op_array; rebound closures get fresh per-instance runtime caches), and after 495d68dfba, ASSIGN_OBJ slots are populated only by set-checked write resolutions — including under the optimizer's slot sharing.

Performance

Microbenchmark: release NTS --disable-all, Apple Silicon, best-of-9 over 20M tight method-scope writes, loop overhead subtracted (re-verified at head 495d68dfba):

net ns per write (interpreter)	master	this PR
public int	1.02	1.04
private(set) int	3.04	1.04
protected(set) int	3.88	1.02

The same collapse holds under opcache (raw: private(set) 5.46 → 3.40 ns, protected(set) 6.22 → 3.44 ns, public 3.46 ns). Read paths are unchanged within noise in every mode, and Zend/bench.php is identical on both builds — the change is invisible outside asymmetric writes.

Scope of this first cut

In scope: direct assignment (ZEND_ASSIGN_OBJ cached path) and write-kind population for write/unset/ptr_ptr. Deliberately unchanged, keeping their existing per-operation checks: static properties, compound assignment / incdec, writes through references, per-direction hook caching, and the JIT's helper paths. Under the tracing JIT asymmetric writes are therefore unchanged for now; teaching the JIT to inline granted asymmetric stores is the follow-up with the largest remaining win, and — together with extending the optimization to compound assignment/incdec — would motivate widening the optimizer change into a full read-pool/write-pool split (with ZEND_FETCH_OBJ_FUNC_ARG remaining read-pooled, as it is bimodal at runtime).

Beyond performance, population-time set checking gives per-direction visibility rules a single enforcement seam instead of scattered per-site checks, which future work can build on.

Verification

Behavior-neutral by test: the full Zend suite plus the asymmetric-visibility, readonly, and property-hooks suites pass with zero .phpt expectation changes in plain, opcache + protect_memory, and tracing-JIT modes. A regression test for the review-found optimizer interaction is included (Zend/tests/asymmetric_visibility/optimizer_shared_cache_slot.phpt) — that read-then-denied-write shape was previously uncovered by the test suite.

Benchmark script

<?php
const N = 20_000_000;
const REPEAT = 9;

class PubProp {
    public int $v = 0;
    public function fill(int $n): void { for ($i = 0; $i < $n; $i++) { $this->v = $i; } }
}
class PrivSetProp {
    public private(set) int $v = 0;
    public function fill(int $n): void { for ($i = 0; $i < $n; $i++) { $this->v = $i; } }
}
class ProtSetBase { public protected(set) int $v = 0; }
class ProtSetChild extends ProtSetBase {
    public function fill(int $n): void { for ($i = 0; $i < $n; $i++) { $this->v = $i; } }
}

function best(callable $f): float {
    $best = INF;
    for ($r = 0; $r < REPEAT; $r++) {
        $t0 = hrtime(true);
        $f();
        $best = min($best, hrtime(true) - $t0);
    }
    return $best / N;
}

$pub = new PubProp(); $priv = new PrivSetProp(); $prot = new ProtSetChild();
$pub->fill(1000); $priv->fill(1000); $prot->fill(1000);
printf("public         %.3f ns/op\n", best(fn() => $pub->fill(N)));
printf("private(set)   %.3f ns/op\n", best(fn() => $priv->fill(N)));
printf("protected(set) %.3f ns/op\n", best(fn() => $prot->fill(N)));

🤖 Generated with Claude Code

[iluuu1994]

Basically, the problem is that the cache slot is shared for read and write opcodes. See Zend/Optimizer/compact_literals.c (which runs only when opcache is enabled), where we find opcodes that fetch the same property and assign it the same cache slot. This way, the first opcode execution will warm the cache for all of them.

I have not actually tested your PR, but if we have a read followed by a write to the same property, the write check would always succeed because the cache is already warmed.

To fix this, Zend/Optimizer/compact_literals.c will need to stop folding cache slots for reads and writes.

[hollyschilling]

I have not actually tested your PR, but if we have a read followed by a write to the same property, the write check would always succeed because the cache is already warmed.

I can't speak to exactly that scenario, but from what I've seen it never can be "warmed" if it's asymmetric.

To fix this, Zend/Optimizer/compact_literals.c will need to stop folding cache slots for reads and writes.

That's pretty much what this PR does.

[iluuu1994]

class P {
    public private(set) string $prop = 'default';
}

class C extends P {
    public function test() {
        var_dump($this->prop);
        $this->prop = 'overwritten';
        var_dump($this->prop);
    }
}

$c = new C;
$c->test();

Run this example without opcache, and you'll get the expected result. Run it with opcache (and hence with Zend/Optimizer/compact_literals.c), and you'll run into the new assertion in zend_assign_to_typed_prop_granted(). If you remove this assertion, you get the behavior I described (private property is overwritten by the child class).

[hollyschilling]

A behavior change was reported against this branch: with opcache enabled, a subclass could silently write a parent's private(set) property when the same method also read the property:

class P { public private(set) string $prop = 'default'; }
class C extends P {
    public function test() {
        var_dump($this->prop);        // read
        $this->prop = 'overwritten';  // must throw — silently succeeded with opcache
    }
}

Confirmed real, and now fixed in 495d68d.

Root cause. The soundness argument for the cached fast path was "cache slots are per-opline, and oplines are direction-specific." That's true of the compiler's output but not of the optimizer's: the literal-compaction pass shares one runtime cache slot among all $this property opcodes referencing the same property name. The FETCH_OBJ_R populated the shared slot with a read-kind (unchecked) resolution, and ASSIGN_OBJ then hit it and skipped the set check. (This sharing is also precisely why the per-write check existed on master.)

Fix. ZEND_ASSIGN_OBJ now gets its own slot-sharing pool in the literal-compaction pass. Those oplines populate exclusively through the set-checked write handler, so sharing among them preserves the invariant; every other property opcode keeps the common pool unchanged, since their fast paths still perform per-operation checks. Cost: at most one extra cache slot triple per property name that is both read and directly assigned on $this within one op_array.

Also audited and ruled out as other routes to the same bypass: rebound closures (per-instance runtime caches — verified granted-from-P/denied-from-C on the same closure source), trait method copies (per-class op_arrays), and fake_scope internal writes (no cache slot).

Added a regression test (Zend/tests/asymmetric_visibility/optimizer_shared_cache_slot.phpt) with the reported shape — read-then-denied-write of the same $this property in one method — which was a coverage gap: no existing asymmetric-visibility test exercised it, which is why CI stayed green. Full suite plus the asymmetric/readonly/hooks suites re-verified in plain, opcache+protect_memory, and tracing-JIT modes. Benchmark numbers are unchanged: under opcache, private(set)/protected(set) writes remain at public-write cost (3.41/3.42 vs 3.44 ns raw).

🤖 Generated with Claude Code

[hollyschilling]

@iluuu1994 Your diagnosis was exactly right, and thank you for it — compact_literals.c sharing one slot across all $this property opcodes of the same name is precisely the hole, and it's also (I now realize) the reason the per-write check existed in the first place: the "slots are per-opline, oplines are direction-specific" argument in the original description was true of the compiler's output but not the optimizer's. The fix is in 495d68dfba, with your repro as a regression test (Zend/tests/asymmetric_visibility/optimizer_shared_cache_slot.phpt) — that read-then-denied-write shape turned out to be uncovered by the existing asymmetric-visibility tests, which is why CI stayed green.

One deliberate difference from "stop folding cache slots for reads and writes" generally: the fix only moves ZEND_ASSIGN_OBJ into its own sharing pool, rather than splitting the pool into read and write halves. The reasoning: ASSIGN_OBJ's cached path is currently the only consumer that treats a populated slot as proof of set access — every other write-flavored opcode (ASSIGN_OBJ_OP, pre/post inc/dec, FETCH_OBJ_W/RW, UNSET_OBJ) still performs its per-operation check in this PR, so they can keep sharing with reads soundly, and splitting them now would spend cache slots for no benefit. Since ASSIGN_OBJ misses populate exclusively through the (now set-checked) write handler, sharing among assign oplines alone preserves the invariant.

The general read-pool/write-pool split you describe is the right end state though: it's what would let the follow-up remove the per-operation checks from the compound-assign/incdec paths too, and it's a precondition for teaching the JIT to inline granted asymmetric stores. One boundary worth noting for that design: ZEND_FETCH_OBJ_FUNC_ARG is bimodal at runtime (read or write per call depending on by-ref argument sends), so it can populate a slot read-kind and can never join a trusted write pool — it either stays read-pooled or keeps its check permanently. If you'd rather have the blunt general split in this PR for the cleaner invariant statement ("a write-pool slot is always set-checked"), I'm happy to make that change — it's small; the current version just optimizes for the minimal reviewable diff.

Also audited for other routes to the same bypass: rebound closures (per-instance runtime caches — verified granted-from-parent/denied-from-child on the same closure source), trait method copies (per-class op_arrays), and fake_scope internal writes (no cache slot). Benchmarks re-run at the fixed head: under opcache, private(set)/protected(set) writes remain at public-write cost (3.40/3.44 vs 3.46 ns raw; net visibility surcharge still eliminated), reads unchanged, Zend/bench.php identical.

🤖 Generated with Claude Code