Reference runtime-defined functions from literals

[nikic]

Currently, we handle closures and conditionally defined functions by inserting them into the function table with an RTD key. There are two issues with this:

1. RTD key collisions. It is hard to ensure that RTD keys unique if opcache is involved. Our changes in 7.4 were insufficient, see https://bugs.php.net/bug.php?id=79603.
2. Leaks. Function templates that end up being unused or not used anymore are never released and leak, see https://bugs.php.net/bug.php?id=76982. This is not much of a problem in web scenarios, but may result in huge leaks in unit tests for example.

This patch addresses this by storing a direct reference to the function as CONST opcode literal. We add a small zend_func_ref wrapper to allow zval storage in a way that is integrated with the existing refcounting system.

This naturally avoids RTD key collisions, because we no longer use an RTD key for this purpose. It partially avoids the memory leak as well. Most of the function structures will now get freed and we only leak the arena allocated parts. Avoiding arena-allocated parts for runtime-defined functions would be a followup.

There are still two problems here:

• As the functions are no longer part of the function_table, they'll get skipped by the optimizer.
• This only works if the function is immutable in opcache. I think this is currently not fully guaranteed, but I'm not sure under what circumstances free functions can be non-immutable in opcache.

Unfortunately this approach will not extend to classes, because those are often not immutable...

@dstogov What do you think about this?

[nikic]

A possible alternative in terms of general approach would be to add zend_function **dynamic_funcs to op_array, store all dynamically declared functions in there and reference them by offset from the literals. The advantage of this approach is that it does not require immutability and as such naturally extends to classes.

[nikic]

I created a prototype for the variant in my last comment at #5595. Unfortunately, I don't think it really helps with the class case either, mutability still remains an issue.

Overall I'm not sure how to solve these problems. Just solving the function case would already be good, but it would be good to have a solution that also works with classes.

[dstogov]

1. RTD key collisions. It is hard to ensure that RTD keys unique if opcache is involved. Our changes in 7.4 were insufficient, see https://bugs.php.net/bug.php?id=79603.

To fix the problem, we may increment CG(rtd_key_counter) and try another key.

2. Leaks. Function templates that end up being unused or not used anymore are never released and leak, see https://bugs.php.net/bug.php?id=76982. This is not much of a problem in web scenarios, but may result in huge leaks in unit tests for example.

The problem occurs only without opcache, if we include the same file again and again.

I understood the problem and the ways you are trying to solve it.
Unfortunately, the solutions look too complex and incomplete + add troubles to optimizer.

Quick ideas:

1. zend_op_array and zend_class_entry already have reference counters and we may reuse them.
2. we may delete RTD keys by special opcodes in implicit "finally" block added to main op_array

[nikic]

To fix the problem, we may increment CG(rtd_key_counter) and try another key.

It's not a full solution, but yes, we should do this for 7.4 at least.

2. Leaks. Function templates that end up being unused or not used anymore are never released and leak, see https://bugs.php.net/bug.php?id=76982. This is not much of a problem in web scenarios, but may result in huge leaks in unit tests for example.

The problem occurs only without opcache, if we include the same file again and again.

That's right.

I understood the problem and the ways you are trying to solve it.
Unfortunately, the solutions look too complex and incomplete + add troubles to optimizer.

Quick ideas:

1. zend_op_array and zend_class_entry already have reference counters and we may reuse them.

This patch is actually based on the normal zend_op_array refcount (which is already incremented/decremented by closures). The additional refcount in the zend_func_ref structure is not really needed, it's just there so functions can be stored as a normal zval, without special-casing. The variant in #5595 has a simpler implementation that does not store functions in zvals.

For zend_class_entry, apart from the issues with mutability, there is also the issue that objects referencing a class don't increase its refcount, which means it wouldn't actually be safe to drop "unused" anon class templates, as we don't track if they are still used by an object. (We could of course add this tracking).

2. we may delete RTD keys by special opcodes in implicit "finally" block added to main op_array

Handling this just in the main op_array is not enough, due to e.g. nested closures. But yes, in principle it can work. However, finally blocks interfere with optimizer and runtime behavior (e.g. in generators).

[nikic]

Closing in favor of #5595.