Infer static for closures

[iluuu1994]

This PR implements static inference for closures, with some restrictions. The closure must not:

1. Use $this. That's the obvious case.
2. Use $$var, given $var could be 'this'.
3. Use Foo::bar(), given this could be a hidden instance call to a (grand-)parent method.
4. Use $f(), for the same reason as 3.
5. Use call_user_func(), for the same reason as 3.
6. Declare another non-static (explicit or inferred) closure, where $this flows from parent to child.
7. Use require, include or eval, given the called code might do any of the above.

In a Symfony Demo run specifically, static inference works for 68/87 (~78%) closures that were explicitly marked as static by Symfony. That seems quite decent.

The PR also adds caching for static closures that don't have any bindings and don't declare static variables. Instances of such closures can be re-used almost without side-effects (except for object identity).

For Symfony Demo (with static removed from all closures), I measured an improvemend of ~0.1%, so definitely not very significant. Synthetic benchmarks can improve quite a bit, though this will also apply to real-world code that creates closures in loops.

function test() {
    $x = function () {};
}
for ($i = 0; $i < 10_000_000; $i++) {
    test();
}

improves by ~78% in my test runs.

If persistent objects are ever implemented, the instantiation could be done fully at compile-time.

[iluuu1994]

@dktapps Ping. I haven't done any benchmarking yet.

[dktapps]

This isn't caching static closures yet, right? So there should be no observable effect on performance

[iluuu1994]

This isn't caching static closures yet, right?

Right. IIRC static closures themselves have a small benefit, though I didn't see it in the CI benchmark. I'm not sure if maybe Symfony already uses linting to add static.

[iluuu1994]

Okay, testing Symfony Demo with all static closures turned into non-static ones shows virtually no improvement. Regardless, they should benefit if we can cache them. I'll try this in the same PR then, given this one isn't useful on its own.

[arnaud-lb]

One drawback of non-static closures is that they retain $this, which can increase memory usage when the lifetime of $this is supposed to be shorter and references a large graph. I believe this is why coding guidelines and IDEs recommend to manually declare closure as static.

Big +1 on this.

[iluuu1994]

Quick test: In Symfony Demo with all static closures turned into non-static ones, this patch can turn 122/283 into static ones. That's ~43%, so not bad at all. How many of those can also be cached remains to be seen.

[dktapps]

Some other weird cases that might need to be considered:

• Variable variables
• include,require et al
• eval()

Source: https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blame/b0a17bf1288a696cba2c8492126f0a485d5637f0/src/Fixer/FunctionNotation/StaticLambdaFixer.php#L110

[dktapps]

To be honest I start to wonder if this actually makes sense considering the number of obscure conditions that might cause an unintended $this binding just for a potential usage. It'd avoid some unnecessary refs but there'd still be plenty of cases where static would be needed to avoid cycles and accidental refs. (I suppose it'd still benefit for common array_map() style cases where a throwaway closure is used, but 🤷 )

Has there been any discussion about a shorter syntax for static closures? e.g. something like sfn() (idk)

[iluuu1994]

Var var is already handled in this patch. I forgot about eval, but that's quite rare as well, especially in closures.

[Girgias]

Ack for ext/spl test changes

[dktapps]

Is there any movement on caching static stateless closures generally? I know the 1cc cache was merged, but there's plenty of cases where explicitly static closures could be cached too without the need for this inference. This inference would just be an added benefit to auto-static stuff in certain cases.

[iluuu1994]

Ofc, explicitly static closures will be cached too, assuming they are stateless.

[dktapps]

Ok, I hadn't realised caching was also in this PR. Good stuff

[arnaud-lb]

This looks right to me!

[arnaud-lb]

Can this one be moved to zend_compile_dynamic_call()?

[iluuu1994]

The only other call to zend_compile_dynamic_call() passes compile-time known string names. Do you still think that would be better?