[RFC] Property Hook improvements

"A side effect of that optimization, however, is that we cannot
proactively detect the bug above. Instead, it would result in an
infinite loop, which would eventually trigger a a stack overflow."

This got a small typo ("a a" at the end).

Fixed, thanks.

This reads no different to me
than infinite recursion between 2 methods. It honestly does not bother
me, and any static analysis tool will be able to pick this anyway.
Sounds like a free performance gain to me!

Yep, it would be exactly 2 method recursion, and the engine would naturally treat it as such.

--Larry Garfield

Hi

I'd say we expect these to be uncontroversial, but this is PHP. :-) So I will instead just note that it's a short RFC and open the discussion accordingly.

Big fan of the performance improvement, given that I convinced Ilija to
look into that once more. Really happy with what he found there.

For the second part about supporting readonly: I don't want to rush this
in the last few weeks before feature freeze, especially since the word
'investigating' appears in the RFC text. Given that readonly is
currently entirely unsupported, this could very well be added with PHP
8.5, allowing folks to really think about the implications and how it
interacts with stuff. That's a "no" from my side.

"Investigating" here is a small term, honestly. Ilija just hadn't had a chance to poke at it to see where is more feasible. I expect it would be a day or so at most to sort that part out; we just wanted to get the discussion going given the timing, and he didn't have time over the weekend. :-)

Are there specific concerns you have about implications, or just a general "dude, it's July" sense?

Best regards
Tim Düsterhus

PS: Small typo: '$this-dbApi->loadCategory($this->categoryId);', '>'
missing there after 'this-'.

Fixed, thanks.

--Larry Garfield

Le 3 juil. 2024 à 11:54, Claude Pache claude.pache@gmail.com a écrit :

2. As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that readonly is only acceptable if there is no get hook.

Hi,

One more thing, why I think that we should be strict here. It is not just for preventing the user to write dumb code, it is also for preventing them to write creative code, e.g.

class doc {
    public readonly int page {
        get => $this->page + $this->offset;
    }
    private int $offset = 0;
    public function __construct(int $page) {
        $this->page = $page;
    }
    public function foo() {
        // $this->offset may be adjusted here
    }
}

—Claude

Le 1 juil. 2024 à 19:02, Larry Garfield larry@garfieldtech.com a
écrit :

Hi folks. As Ilija's been polishing off hooks to get the PR merged,
we've run into two small revisions that should make life better for
all involved. One is a performance improvement that requires a very
slight error handling behavior change, and the other is enabling
readonly in selected (but probably all of the relevant)
circumstances.

I'd say we expect these to be uncontroversial, but this is PHP. :-)
So I will instead just note that it's a short RFC and open the
discussion accordingly.

https://wiki.php.net/rfc/hook_improvements

1. Removing the guard against recursion is sensible to me, as the sort
   of bug it is supposed to prevent is not specific to property
   accessors. IMO, a better solution to the issue is to implement a
   global upper limit on the call stack size.

That is already done in PHP 8.4, if you end up using too much stack, PHP
will now bail out graciously:
https://github.com/php/php-src/blob/master/Zend/zend_call_stack.c

Currently, I am able to generate a call stack of more than 10 millions
items before an OOM error occurs; PHP should have thrown a stack
overflow error long before that.

Why? Most actual function calls are not done on the stack any more. So
it is now perfectly okay to have that many nested calls. If you want it
to bail out earlier (during testing), then Xdebug will help you.

cheers,
Derick

--
https://derickrethans.nl | https://xdebug.org | https://dram.io

Author of Xdebug. Like it? Consider supporting me: https://xdebug.org/support

mastodon: @derickr@phpc.social @xdebug@phpc.social

Hi Derick

https://wiki.php.net/rfc/hook_improvements

1. Remove the proactive guard against recursive backing value access

    "which would eventually trigger a stack overflow"
   

Would it though? PHP 8.4 has an actual protection against this now,
instead of getting the white-screen-of-death.

Not that it matters much, but it's probably good to be precise in the
language.

Yes, this excerpt is referring to the PHP stack overflow detection,
which will throw an exception rather than segfault. That is, on all
the architectures that already support stack overflow detection. We're
just reusing the same mechanism already used for normal function
calls.

Hi Claude

1. Removing the guard against recursion is sensible to me, as the sort of bug it is supposed to prevent is not specific to property accessors. IMO, a better solution to the issue is to implement a global upper limit on the call stack size. Currently, I am able to generate a call stack of more than 10 millions items before an OOM error occurs; PHP should have thrown a stack overflow error long before that. But this is entirely orthogonal to property hooks.

PHP allows using recursion for algorithms on arbitrarily large data
because function calls do not cause the C stack to grow. Limiting the
PHP stack size would thus require these algorithms to be rewritten to
iterative ones, which can make them more complicated. What we could do
is check for stack size on OOM and indicate that the OOM was most
likely caused by infinite recursion.

Anyway, This does not really apply to hooks, because it will (usually)
cause VM reentry for recursion, growing the stack size and thus
triggering the "fast" stack overflow detection error.

Ilija

Hi

The other half of this RFC, removing the recursion guard in return for performance, has had no objections. Its discussion period ends Friday, so I will open the vote on that Monday-ish.

Thank you, looks good to me, ship it.

One minor remark about the RFC page: You didn't remove the "(use today's
date here)" placeholder at the top.

Best regards
Tim Düsterhus