Made closure php-src cachable

[staabm]

see #4701 (comment)

[staabm]

@iluuu1994 could you have another look at this PR. intention is, that php-src will be able to cache the closure, while it could not before.

does this make sense like that and will it have the expected behaviour?

[staabm]

regarding other examples:

$classType = TypeTraverser::map($classType, static function (Type $type, callable $traverse) use (&$uncertainty): Type {
					if ($type instanceof UnionType || $type instanceof IntersectionType) {
						return $traverse($type);
					}
					if ($type->getObjectClassNames() !== []) {
						$uncertainty = true;
						return $type;
					}
					if ($type instanceof GenericClassStringType) {
						$uncertainty = true;
						return $type->getGenericType();
					}
					if ($type instanceof ConstantStringType) {
						return new ObjectType($type->getValue());
					}
					return new MixedType();
				});

do you think it would make things faster if we transform such closures into e.g. __invoke classes or similar?

Some callsites of TypeTraverser::map are easily invoked multi-million times in hot paths

[dktapps]

Basically the requirement for being cached is that it has to be stateless. So:

• no $this
• no used variables
• no local static variables

In some cases it might be worth considering if a plain old function makes more sense

[dktapps]

PR changes look good to me wrt. cacheability, although it might be worth considering if a private static function would make more sense than a closure for larger functions

[staabm]

@dktapps thank you

Basically the requirement for being cached is that it has to be stateless

can this be measured somehow?

[phpstan-bot]

This pull request has been marked as ready for review.

[iluuu1994]

do you think it would make things faster if we transform such closures into e.g. __invoke classes or similar?

The static closure patch avoids an allocation each time the closure is used (not called). Allocating an object rather than a closure shouldn't make a significant difference, unless you can avoid allocating the object each time. For closures that have usees, that might be easier to achieve with __invokeable objects with properties I suppose.

[iluuu1994]

What's the purpose of this static closure in the first place? The other case has an include (so possibly there to hide local vars?) and does some control flow. Why not just drop the closure entirely?

[staabm]

good point. removed this closure.

thank you.

[dktapps]

@dktapps thank you

Basically the requirement for being cached is that it has to be stateless

can this be measured somehow?

Not sure what you mean by this, but for an explicitly static closure it just can't have used vars or local static vars.

The PR you mentioned on php-src is mostly focused on inferring static when it's not explicitly specified.
Inferring static won't guarantee cacheability, because a static closure could still use local scope vars or have local static variables.
But inferring static does potentially make it possible to cache closures which aren't explicitly marked with static, if they by chance have no uses and no local static vars (a fairly common case seen a lot with array_map callbacks and such).

[staabm]

The static closure patch avoids an allocation each time the closure is used (not called).

does this means, after making a closure cachable we might see a runtime improvement or memory_get_peak improvement?
or is this such a minor thing that changing code to be a cachable closure will kindly be noticable in perf metrics?
(making this more efficient is my primary motivation for such changes atm)

[staabm]

@ondrejmirtes I wonder why this errors show up now, but it seems they were not reported when 1:1 the same code was part of MutatingScope class. I could not find ignore-error rules or something which would swallow them either.

---

besides that, I think this PR is ready to merge.
I would do more *Traverser-extractions in separate PRs, as we have too many of them.

[dktapps]

The static closure patch avoids an allocation each time the closure is used (not called).

does this means, after making a closure cachable we might see a runtime improvement or memory_get_peak improvement? or is this such a minor thing that changing code to be a cachable closure will kindly be noticable in perf metrics? (making this more efficient is my primary motivation for such changes atm)

It depends on how hot the code is, you might see a performance improvement. I wouldn't expect to see much difference in memory usage

[iluuu1994]

does this means, after making a closure cachable we might see a runtime improvement or memory_get_peak improvement?

That's the idea. It should be beneficial for code that creates closures in a loop. Of course, you could already avoid this performance hit by moving the closure out of the loop, but the idea here is of course is that it happens automatically.

Regarding __invoke: Make sure to benchmark. Performance can be unpredictable.