Hi everyone
I've created a PR implementing two new optimizations for closures. [1]
There are some theoretical BC breaks, so please let me know if any of
them are of concern to you.
- Static inference
The engine will attempt to infer static for closures that are
guaranteed not to make any use of $this. This has the benefit of
avoiding a cycle when a closure is stored in a property of the
declarator object. Frequently, such objects and their closures will
not be collected for the entirety of the request, given the GC often
doesn't run at all. But the real reason for this optimization is to
enable optimization nr. 2.
It's worth noting the rules for inferring static are slightly
esoteric. You can read more about the rules in the PR description.
- Stateless closure caching
Stateless closures, i.e. those that are static, don't capture any
variables and don't declare any static variables are now cached.
function test() {
$x = function () {};
}
for ($i = 0; $i < 10_000_000; $i++) {
test();
}
Previously, this would have created 10 000 000 closure instances, even
though all closures are effectively identical. Now, the first closure
will be kept alive for reuse. With these two optimizations, this small
benchmark improves by ~80%. Of course, this is a very synthetic
benchmark. However, I could also measure improvements in real
applications that instantiate many stateless closures. For example, in
the Laravel template these two optimizations can avoid 2384 out of
3637 closure instantiations, improving performance by ~3% on my
machine.
The foreshadowed BC breaks come down to a three things:
-
For closures that are inferred as static,
ReflectionFunction::getClosureThis() will now return NULL. -
Objects that would previously have created cycles may be collected
earlier, also triggering destructors earlier. IMO, this is a feature
and more predictable than the current behavior. -
Two stateless closures originating from the same lexical location
will now be identical. I.e.:function test() {
return function () {};
}
test() === test(); // true
Of note is that Closure::bind() and Closure::bindTo() usually throw
when attempting to bind an object to a static closure. In my PR, this
is explicitly allowed only for closures that are inferred as static,
but not those that are explicitly static.
I'm looking forward to your feedback. If there are no concerns, I will
merge this PR into master for PHP 8.6 in roughly two weeks.
Ilija
Hi everyone
I've created a PR implementing two new optimizations for closures. [1]
There are some theoretical BC breaks, so please let me know if any of
them are of concern to you.
- Static inference
The engine will attempt to infer static for closures that are
guaranteed not to make any use of $this. This has the benefit of
avoiding a cycle when a closure is stored in a property of the
declarator object. Frequently, such objects and their closures will
not be collected for the entirety of the request, given the GC often
doesn't run at all. But the real reason for this optimization is to
enable optimization nr. 2.It's worth noting the rules for inferring static are slightly
esoteric. You can read more about the rules in the PR description.
- Stateless closure caching
Stateless closures, i.e. those that are static, don't capture any
variables and don't declare any static variables are now cached.function test() { $x = function () {}; } for ($i = 0; $i < 10_000_000; $i++) { test(); }Previously, this would have created 10 000 000 closure instances, even
though all closures are effectively identical. Now, the first closure
will be kept alive for reuse. With these two optimizations, this small
benchmark improves by ~80%. Of course, this is a very synthetic
benchmark. However, I could also measure improvements in real
applications that instantiate many stateless closures. For example, in
the Laravel template these two optimizations can avoid 2384 out of
3637 closure instantiations, improving performance by ~3% on my
machine.The foreshadowed BC breaks come down to a three things:
For closures that are inferred as static,
ReflectionFunction::getClosureThis() will now return NULL.Objects that would previously have created cycles may be collected
earlier, also triggering destructors earlier. IMO, this is a feature
and more predictable than the current behavior.Two stateless closures originating from the same lexical location
will now be identical. I.e.:function test() {
return function () {};
}
test() === test(); // trueOf note is that Closure::bind() and Closure::bindTo() usually throw
when attempting to bind an object to a static closure. In my PR, this
is explicitly allowed only for closures that are inferred as static,
but not those that are explicitly static.I'm looking forward to your feedback. If there are no concerns, I will
merge this PR into master for PHP 8.6 in roughly two weeks.Ilija
This all looks great, Ilija! I am not concerned about the BC mentions, as they seem to be accurate anyway.
My one question is about the bindTo() on an inferred static closure. What purpose does that serve? I'm assuming that anything that mentions $this in its body won't be marked static, and if it doesn't have $this I don't get what value bindTo() would have. What's the use case for making an exception here?
--Larry Garfield
Hi Larry
I've created a PR implementing two new optimizations for closures. [1]
There are some theoretical BC breaks, so please let me know if any of
them are of concern to you.Of note is that Closure::bind() and Closure::bindTo() usually throw
when attempting to bind an object to a static closure. In my PR, this
is explicitly allowed only for closures that are inferred as static,
but not those that are explicitly static.This all looks great, Ilija! I am not concerned about the BC mentions, as they seem to be accurate anyway.
My one question is about the bindTo() on an inferred static closure. What purpose does that serve? I'm assuming that anything that mentions $this in its body won't be marked static, and if it doesn't have $this I don't get what value bindTo() would have. What's the use case for making an exception here?
Oops, it seems I've left out some details I was meaning to add. The
aim for allowing the passing of objects to Closure::bindTo is to
retain BC when a closure can suddenly be inferred as static due to
seemingly unrelated changes.
As mentioned, the rules for when static can be inferred are slightly
esoteric. For example, your code may be calling Foo::bar(), which
looks like a static call, but could actually be an instance
grand-parent call (https://3v4l.org/uDi8W#v8.4.14). If you were to
remove this ambiguous call, and the closure could now be inferred as
static, it would be bad if existing Closure::bindTo calls would
break because they provide an object. For this reason,
Closure::bindTo will accept but discard objects only for
closures inferred as static.
Ilija
Hi Larry
I've created a PR implementing two new optimizations for closures. [1]
There are some theoretical BC breaks, so please let me know if any of
them are of concern to you.Of note is that Closure::bind() and Closure::bindTo() usually throw
when attempting to bind an object to a static closure. In my PR, this
is explicitly allowed only for closures that are inferred as static,
but not those that are explicitly static.This all looks great, Ilija! I am not concerned about the BC mentions, as they seem to be accurate anyway.
My one question is about the bindTo() on an inferred static closure. What purpose does that serve? I'm assuming that anything that mentions $this in its body won't be marked static, and if it doesn't have $this I don't get what value bindTo() would have. What's the use case for making an exception here?
Oops, it seems I've left out some details I was meaning to add. The
aim for allowing the passing of objects to Closure::bindTo is to
retain BC when a closure can suddenly be inferred as static due to
seemingly unrelated changes.As mentioned, the rules for when static can be inferred are slightly
esoteric. For example, your code may be calling Foo::bar(), which
looks like a static call, but could actually be an instance
grand-parent call (https://3v4l.org/uDi8W#v8.4.14). If you were to
remove this ambiguous call, and the closure could now be inferred as
static, it would be bad if existing Closure::bindTo calls would
break because they provide an object. For this reason,
Closure::bindTo will accept but discard objects only for
closures inferred as static.Ilija
Ah, so it's just a "don't break code that's already wrong but not harmfully so" feature. I'm OK with that, but please add that to the RFC.
--Larry Garfield
Hi Larry
As mentioned, the rules for when static can be inferred are slightly
esoteric. For example, your code may be calling Foo::bar(), which
looks like a static call, but could actually be an instance
grand-parent call (https://3v4l.org/uDi8W#v8.4.14). If you were to
remove this ambiguous call, and the closure could now be inferred as
static, it would be bad if existing Closure::bindTo calls would
break because they provide an object. For this reason,
Closure::bindTo will accept but discard objects only for
closures inferred as static.Ah, so it's just a "don't break code that's already wrong but not harmfully so" feature.
I wouldn't consider it wrong to not mark functions as static that
could be static, otherwise there wouldn't be a need for this
optimization. Consequently, it would also not be wrong to attempt to
Closure::bind() an object to such a closure, which is why I felt it
was important to keep this case working. The caveat here being that
the object is discarded, so ReflectionFunction::getClosureThis() will
still return NULL.
I'm OK with that, but please add that to the RFC.
I did not plan to make an RFC for this, given the BC breaks are mostly
theoretical. But if anybody objects, I'm happy to create one.
Ilija
Hi Larry
I've created a PR implementing two new optimizations for closures. [1]
There are some theoretical BC breaks, so please let me know if any of
them are of concern to you.Of note is that Closure::bind() and Closure::bindTo() usually throw
when attempting to bind an object to a static closure. In my PR, this
is explicitly allowed only for closures that are inferred as static,
but not those that are explicitly static.This all looks great, Ilija! I am not concerned about the BC mentions, as they seem to be accurate anyway.
My one question is about the bindTo() on an inferred static closure. What purpose does that serve? I'm assuming that anything that mentions $this in its body won't be marked static, and if it doesn't have $this I don't get what value bindTo() would have. What's the use case for making an exception here?
Oops, it seems I've left out some details I was meaning to add. The
aim for allowing the passing of objects to Closure::bindTo is to
retain BC when a closure can suddenly be inferred as static due to
seemingly unrelated changes.As mentioned, the rules for when static can be inferred are slightly
esoteric. For example, your code may be calling Foo::bar(), which
looks like a static call, but could actually be an instance
grand-parent call (https://3v4l.org/uDi8W#v8.4.14). If you were to
remove this ambiguous call, and the closure could now be inferred as
static, it would be bad if existing Closure::bindTo calls would
break because they provide an object. For this reason,
Closure::bindTo will accept but discard objects only for
closures inferred as static.Ilija
Ah, so it's just a "don't break code that's already wrong but not harmfully so" feature. I'm OK with that, but please add that to the RFC.
--Larry Garfield