Make static binding consistent and a little faster #1899
Conversation
| static_variables = func->static_variables; | ||
| func->static_variables = NULL; | ||
| zend_hash_str_del(EG(function_table), LAMBDA_TEMP_FUNCNAME, sizeof(LAMBDA_TEMP_FUNCNAME)-1); | ||
| func->static_variables = static_variables; |
There was a problem hiding this comment.
hmm, why change this one? the double free won't be occurred ?
There was a problem hiding this comment.
Was fine in an earlier iteration of the patch … but now … Not quite (free is after refcount), but it will destroy the contents of the static vars ... Fixing and thanks for catching.
Previously cases like
class A { function foo() { static $a = 0; print $a++; } } (new A)->foo(); class B extends A {} (new B)->foo(); (new A)->foo(); (new B)->foo();
exposed different behavior depending on whether class B was early bound (add a surrounding if (1) { } statement) or the first call to the foo method happens after binding.
The reference was only installed at the first BIND_STATIC opcode, causing different behavior depending on whether statics were duplicated before or after BIND_STATIC.
This patch fixes that by always creating a reference for statics in functions, right at compile time.
The semantics are now that each function (with the exception of trait functions, which are expected to be literally copy-pasted) shares its statics with all instances of it.
The case above will now print 0123 in every case instead of 0011 or 0123 depending on binding times.
Also with Closures:
function getClosure() { return function () { static $a = 0; print $a++; }; } $f = getClosure(); $f(); $g = getClosure(); $f(); $g(); $f();
will now print 0123 instead of 0102.
This makes behavior much more consistent.
To achieve this, it was needed to move to a zval+name array (instead of HashTable) for statics (as we cannot have mutable references in an immutable array in opcache).
Hence statics need to be copied directly into process memory upon each request instead of residing in shared memory. (That also removes the need for some run-time duplications.)
To support that, the patch adds a way to relocate pointers inside copied memory and arena allocates the copied parts.
Additionally, the better locality (statics now are typically closer to the op_array - side effect of arena allocation), one less fetch inside ZEND_BIND_STATIC (hello stalling cycles waiting for memory...) and much faster Closure binding account for (up to) 0.3% run-time improvement on real world applications (typical modern applications are relatively heavy on Closures nowadays).
|
Even, if this was a bug fix, it changes well-known PHP behavior. I believe, this tricky "static property cloning" is used by some frameworks. Thanks. Dmitry. From: Joe Watkins notifications@github.com What is the status of this PR ? Looks to me like a bug fix ... @dstogovhttps://github.com/dstogov @bwoebihttps://github.com/bwoebi @nikichttps://github.com/nikic You are receiving this because you were mentioned. |
Yes, I'm just trying to determine if it's going to be targeting 7.1. |
|
Thanks for asking… I indeed forgot about this patch… @dstogov Can you please figure out the actual BC impact? |
|
It's clear. Now static properties in children classes are not shared with parent. With the patch they are going to be common and modification of a static property in a single child will also affect all other children. From: Bob Weinand notifications@github.com Thanks for asking... I indeed forgot about this patch... @dstogovhttps://github.com/dstogov Can you please figure out the actual BC impact? You are receiving this because you were mentioned. |
|
That's the change, yes. The question was whether that has actual larger BC impact on actual applications? |
|
@bwoebi Whether or not the BC break is large, this is a non-trivial change, and as such would probably benefit from an RFC. |
|
In OTR discussion (thread "Little faster and more consistent static binding" starting May 5, 2016) the reception to this change was negative. Do you still plan to pursue this, or can we close this PR? |
|
As there doesn't seem to be further interest here, I'm closing this. |
|
I think, this change may be interesting, but it should wait for PHP-8,
|
Previously cases like
exposed different behavior depending on whether class B was early bound (add a surrounding
if (1) { }statement) or the first call to the foo method happens after binding.The reference was only installed at the first BIND_STATIC opcode, causing different behavior depending on whether statics were duplicated before or after BIND_STATIC.
This patch fixes that by always creating a reference for statics in functions, right at compile time.
The semantics are now that each function (with the exception of trait functions, which are expected to be literally copy-pasted) shares its statics with all instances of it.
The case above will now print 0123 in every case instead of 0011 or 0112 depending on binding times.
Also with Closures:
will now print 0123 instead of 0101.
This makes behavior much more consistent.
To achieve this, it was needed to move to a zval+name array (instead of HashTable) for statics (as we cannot have mutable references in an immutable array in opcache).
Hence statics need to be copied directly into process memory upon each request instead of residing in shared memory. (That also removes the need for some run-time duplications.)
To support that, the patch adds a way to relocate pointers inside copied memory and arena allocates the copied parts.
Additionally, the better locality (statics now are typically closer to the op_array - side effect of arena allocation), one less fetch inside ZEND_BIND_STATIC (hello stalling cycles waiting for memory...) and much faster Closure binding account for (up to) 0.3% run-time improvement on real world applications (typical modern applications are relatively heavy on Closures nowadays).