The following bugs relate to this discussion:
#64291 Indeterminate GC of evaled lambda function resources
#65915 Inconsistent results with require return value
I don't want to discuss these or the specific fixes here, since I can
work up a fix and discuss them in the bugrep. However,
my one-sentence Q is "should we replace the naming convention for
closures with a truly unique one?" What I would like is some feedback /
guidance / discussion on the general architectural issue which underlies
the reasons for these bugs occuring in the first place.
-
The associated closure functions are compiled during the
INCLUDE_OR_EVAL opcode that included / eval'ed the originating PHP
source, and are named according to the convention
"\0{closure}$name$addr" where the name is the resolved filename of the
source where the function was defined and the addr is the absolute
address of the function definition within the memory resident copy of
the source during the compilation process. The compilation creates an
entry in the GC(function_table) for this function -
Closure objects are instantiated during the execution
ZEND_DECLARE_LAMBDA_FUNCTION opcode at runtime. This handler invokes
the built-in CTOR for the closure bind the corresponding function, -
Closure objects are destroyed by the closure DTOR in line with the
usual PHP scoping rules. -
There is no specific deletion function for the GC(function_table)
entries other than normal request rundown, because the compiler and RTS
can't safely determine if a closure function will no longer be required
for further closure, e.g.for ($array as $i) {
$f = function($x) { ... };
...
} -
The "\0{closure}$name$addr" scheme does provide a sort of dirty reuse
in that if another function is compiled with the same filename and at
the same source address (which can happen for logically different
variables due to storage reuse), then the current implementation simply
overwrites the earlier function definition. Though the rationale for
this isn't documented, I assume that this is because it is usually
safe to assume that the earlier copy is no longer required. However, it
is possible to construct cases where this assumption is false --
especially as is the case of OPcached functions where the name can
persist for the life of the SMA rather than just one request.
What I am suggesting is that this convention should be changed for
something like "\0{closure}$name$ctime$index" or even just
"\0{closure}$uuid" using the standard UUID algo. Reactions? Comments?
The only side effect that I can see is that in circumstances where the
overwrite does replace stale functions then the function DTOR will no
longer be taking place leading a growth in the function table with the
associated memory overhead.
Thanks for any feedback
Terry
2013.11.20. 17:42, "Terry Ellison" ellison.terry@gmail.com ezt írta:
The following bugs relate to this discussion:
#64291 Indeterminate GC of evaled lambda function resources #65915 Inconsistent results with require return valueI don't want to discuss these or the specific fixes here, since I can
work up a fix and discuss them in the bugrep. However,
my one-sentence Q is "should we replace the naming convention for
closures with a truly unique one?" What I would like is some feedback /
guidance / discussion on the general architectural issue which underlies
the reasons for these bugs occuring in the first place.
The associated closure functions are compiled during the
INCLUDE_OR_EVAL opcode that included / eval'ed the originating PHP source,
and are named according to the convention "\0{closure}$name$addr" where the
name is the resolved filename of the source where the function was defined
and the addr is the absolute address of the function definition within the
memory resident copy of the source during the compilation process. The
compilation creates an entry in the GC(function_table) for this functionClosure objects are instantiated during the execution
ZEND_DECLARE_LAMBDA_FUNCTION opcode at runtime. This handler invokes the
built-in CTOR for the closure bind the corresponding function,Closure objects are destroyed by the closure DTOR in line with the
usual PHP scoping rules.There is no specific deletion function for the GC(function_table)
entries other than normal request rundown, because the compiler and RTS
can't safely determine if a closure function will no longer be required for
further closure, e.g.for ($array as $i) {
$f = function($x) { ... };
...
}The "\0{closure}$name$addr" scheme does provide a sort of dirty reuse
in that if another function is compiled with the same filename and at the
same source address (which can happen for logically different variables due
to storage reuse), then the current implementation simply overwrites the
earlier function definition. Though the rationale for this isn't
documented, I assume that this is because it is usually safe to assume
that the earlier copy is no longer required. However, it is possible to
construct cases where this assumption is false -- especially as is the case
of OPcached functions where the name can persist for the life of the SMA
rather than just one request.What I am suggesting is that this convention should be changed for
something like "\0{closure}$name$ctime$index" or even just
"\0{closure}$uuid" using the standard UUID algo. Reactions? Comments?The only side effect that I can see is that in circumstances where the
overwrite does replace stale functions then the function DTOR will no
longer be taking place leading a growth in the function table with the
associated memory overhead.Thanks for any feedback
Terry--
Bumping the thread and ccing Joe as we were having a similar discussion
about the internal naming of the inner classes proposed by him.
2013.11.20. 17:42, "Terry Ellison" <ellison.terry@gmail.com
mailto:ellison.terry@gmail.com> ezt írta:The following bugs relate to this discussion:
#64291 Indeterminate GC of evaled lambda function resources #65915 Inconsistent results with require return valueI don't want to discuss these or the specific fixes here, since I can
work up a fix and discuss them in the bugrep. However,
my one-sentence Q is "should we replace the naming convention for
closures with a truly unique one?" What I would like is some feedback /
guidance / discussion on the general architectural issue which underlies
the reasons for these bugs occuring in the first place....Bumping the thread and ccing Joe as we were having a similar discussion
about the internal naming of the inner classes proposed by him.
Thanks for bumping this.
Yup, the current build_runtime_defined_function_key() algo builds the
function name on the file name (closures) / the class/function name and
LANG_SCNG(yy_text) -- that is the memory address of the class / function
token being compiled. This is for closures, inner classes or any class
or function within a conditional block.
I guess this is on the assumption that this will be unique, but as I
have shown in #64291 and #65915 it is fairly straight forward to
construct test cases where this assumption is invalid, and arjan seems
to be hitting this in real-life OPcache cases.
My patch at least demonstrates that having a unique key will fix this,
but as submitted (adding a simple sequence) this will only reduces this
occurrence. However strictly, this could still fail in OPcache because
of sequence duplication across forked PHP children. We really need a
UUID-style element in the key say based on the creation microtime (or
alternatively start time of request + sequence within request) and PID.
Regards
Terry
2013.11.20. 17:42, "Terry Ellison" <ellison.terry@gmail.com
mailto:ellison.terry@gmail.com> ezt írta:The following bugs relate to this discussion:
#64291 Indeterminate GC of evaled lambda function resources #65915 Inconsistent results with require return valueI don't want to discuss these or the specific fixes here, since I can
work up a fix and discuss them in the bugrep. However,
my one-sentence Q is "should we replace the naming convention for
closures with a truly unique one?" What I would like is some feedback /
guidance / discussion on the general architectural issue which underlies
the reasons for these bugs occuring in the first place....Bumping the thread and ccing Joe as we were having a similar discussion
about the internal naming of the inner classes proposed by him.Thanks for bumping this.
Yup, the current build_runtime_defined_function_key() algo builds the
function name on the file name (closures) / the class/function name
and LANG_SCNG(yy_text) -- that is the memory address of the class /
function token being compiled. This is for closures, inner classes or
any class or function within a conditional block.I guess this is on the assumption that this will be unique, but as I
have shown in #64291 and #65915 it is fairly straight forward to
construct test cases where this assumption is invalid, and arjan seems
to be hitting this in real-life OPcache cases.My patch at least demonstrates that having a unique key will fix this,
but as submitted (adding a simple sequence) this will only reduces
this occurrence. However strictly, this could still fail in OPcache
because of sequence duplication across forked PHP children. We really
need a UUID-style element in the key say based on the creation
microtime (or alternatively start time of request + sequence within
request) and PID.
I have tabled a couple of PHPT tests on the bugreps which show this
error consistently.
Having discussed the options with Dmitry and another contributor off PHP
Internals list, we have decided to base the generated <internal>
function name on a hash of the source content between the text pointers
at zend_do_begin_function_declaration() and
zend_do_end_function_declaration(). I will prepare a patch on this basis
in the coming week.
Thanks and regards
Terry Ellison
Hi!
Having discussed the options with Dmitry and another contributor off PHP
Internals list, we have decided to base the generated <internal>
function name on a hash of the source content between the text pointers
Wouldn't that imply that two closures with the same code would be
identified as the same closure? If so, I'm not sure this is a correct
approach - one can definitely have two different closures (with
different states, different bindings, etc.) having same source text. Or
am I missing something here?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Having discussed the options with Dmitry and another contributor off PHP
Internals list, we have decided to base the generated <internal>
function name on a hash of the source content between the text pointers
Wouldn't that imply that two closures with the same code would be
identified as the same closure? If so, I'm not sure this is a correct
approach - one can definitely have two different closures (with
different states, different bindings, etc.) having same source text. Or
am I missing something here?
Stas, nope, not quite. The compiler generates a zend_function oparray
for each closure that it compiles at compile time, and stores this in
the execution function table under a mangled name. It also generates a
ZEND_DECLARE_LAMBDA_FUNCTION <mangled name> instruction at the
appropriate place in the oparray referencing the closure. When
executed, this acts as the CTOR for the closure object and this deep
copies the dummy mangled zend_function entry into the closure object. It
is this entry within the closure object that is in fact executed when
the closure is invoked. The use bindings take place as part of the
CTOR. This copy is then GCed at the closure DTOR.
So the CTOR will occur each time the ZEND_DECLARE_LAMBDA_FUNCTION is
executed, and the DTORs occur in line with normal PHP scoping / GC rules.
This bug is that the current mangling rules mean that under some
perverse (but possible in real life) conditions, two different source
codes could generate the same mangled name so the second entry would
incorrectly override the first, resulting in the closure CTORs binding
the wrong function, so we need a unique mangling algo.
The mangled zend_function entry is never executed; only used as a copy
template.
A simple sequence count of the number of closures compiled this request
would be fine to separate closure declarations and guarantee uniqueness
-- except when OPcaching is involved, so some context dependent hash i
needed.
This is really a separate thread, but in order to allow opcode caching,
the PHP compiler must generate the same oparray for a given source
sequence, so in fact even if the same function (...) {...} text sequence
occured multiple times in the same source file, then the oparrays would
be identical anyway, and using the same mangled zend_function entry is
fine. However, if the 1/2^64 (or thereabouts) chance of a false
collision is unacceptable, we could always embed the closure compile
count in the name as well, or the microsecond compile time.
Hope this explanation helps :-) Terry
Hi!
The mangled zend_function entry is never executed; only used as a copy
template.
I see. That makes it better, but not completely, please see below.
This is really a separate thread, but in order to allow opcode caching,
the PHP compiler must generate the same oparray for a given source
sequence, so in fact even if the same function (...) {...} text sequence
occured multiple times in the same source file, then the oparrays would
be identical anyway, and using the same mangled zend_function entry is
fine. However, if the 1/2^64 (or thereabouts) chance of a false
This is true. However, if multiple scripts have the same function name,
and are added to the cache separately in different time, by default the
engine does not allow adding the same function twice (even if it has the
same op-array). We could code around it but why create problems for
ourselves if we could easily avoid it? So I think it would be better to
add something to the mix - like filename/lineno or counter - to ensure
it is not the same. I'm not worried about the hash collisions, but
copy-paste happens much more frequent than 1/2^64 hash collision.
collision is unacceptable, we could always embed the closure compile
count in the name as well, or the microsecond compile time.
I would avoid using time, as getting time is usually a system call and
system calls are slow. Counter or filename/lineno or anything that makes
the hash different would be fine.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
The mangled zend_function entry is never executed; only used as a copy
template.
I see. That makes it better, but not completely, please see below.This is really a separate thread, but in order to allow opcode caching,
the PHP compiler must generate the same oparray for a given source
sequence,
I will expand in this further in a separate thread.
so in fact even if the same function (...) {...} text sequence
occured multiple times in the same source file, then the oparrays would
be identical anyway, and using the same mangled zend_function entry is
fine. However, if the 1/2^64 (or thereabouts) chance of a false
This is true. However, if multiple scripts have the same function name,
and are added to the cache separately in different time, by default the
engine does not allow adding the same function twice (even if it has the
same op-array).
Unfortunately this is not correct for closures. The current code for
closures overwrites the previous function silently -- that is without
error. OPcache has a different behaviour for a cached closure and
instead ignores the new function (again silently) leading to a different
behaviour for compiled scripts executed out of the opcode cache. See
https://bugs.php.net/bug.php?id=64291 for PHPTs which demonstrate this
failure.
We could code around it but why create problems for
ourselves if we could easily avoid it? So I think it would be better to
add something to the mix - like filename/lineno or counter - to ensure
it is not the same. I'm not worried about the hash collisions, but
copy-paste happens much more frequent than 1/2^64 hash collision.
Filename/lineno is not unique as the PHPT shows. Use of counter can be
flawed due to race conditions if OPcaching is enabled. This failure can
occur in real app frameworks e.g. those which use a common compiler
routine to generate custom templates.
collision is unacceptable, we could always embed the closure compile
count in the name as well, or the microsecond compile time.
I would avoid using time, as getting time is usually a system call and
system calls are slow. Counter or filename/lineno or anything that makes
the hash different would be fine.
+1 on avoiding time-related system calls -- even though these are pretty
optimized on current Linux kernels -- however this is why we suggested a
content based hash. Any alternatives that I can think of require a
materially larger larger patch involving more source changes.
Regards Terry
Hi!
+1 on avoiding time-related system calls -- even though these are pretty
optimized on current Linux kernels -- however this is why we suggested a
content based hash. Any alternatives that I can think of require a
materially larger larger patch involving more source changes.
I understand why you proposed content-based hash. What I was suggesting
is not replacing it but amending it to produce different hash even in
case same code would be encountered in different places - e.g. by
hashing not only the text but also filename & line number (or counter).
I'm not sure if it is strictly necessary for OPcache (could be that it
is not) but in general having multiple functions with the same name
floating around is not a very good idea, IMO, if we can avoid it...
After all, that's how we got this problem from the start :)
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Stas,
I understand why you proposed content-based hash. What I was
suggesting is not replacing it but amending it to produce different
hash even in case same code would be encountered in different places -
e.g. by hashing not only the text but also filename & line number (or
counter). I'm not sure if it is strictly necessary for OPcache (could
be that it is not) but in general having multiple functions with the
same name floating around is not a very good idea, IMO, if we can
avoid it... After all, that's how we got this problem from the start :)
Yup, the scheme (in PHP speak):
$mangled_name=sprintf("\0{closure}%s-%u-%x", $filename,
$offset_into_file, $hash);
addresses the concerns that you voice. I can't think of use cases where
this would lead to false collisions. I will code up my patch on this
basis.
Thanks Terry
What I am suggesting is that this convention should be changed for something
like "\0{closure}$name$ctime$index" or even just "\0{closure}$uuid" using the
standard UUID algo. Reactions? Comments?
For Xdebug to show them in stack traces, and for profiling, I rewrite
them to: {closure:/path/to/closure-stack-trace.php:4-7}
See https://github.com/xdebug/xdebug/blob/master/xdebug_stack.c#L909
cheers,
Derick
--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug
Posted with an email client that doesn't mangle email: alpine
Hi Derick,
What I am suggesting is that this convention should be changed for something
like "\0{closure}$name$ctime$index" or even just "\0{closure}$uuid" using the
standard UUID algo. Reactions? Comments?
For Xdebug to show them in stack traces, and for profiling, I rewrite
them to: {closure:/path/to/closure-stack-trace.php:4-7}
Seehttps://github.com/xdebug/xdebug/blob/master/xdebug_stack.c#L909
Thanks for this. What you are rewriting here is the function name of
the closure being executed and which is the backtrace. As I said in my
O/P, the closure contains a deep copy of the magic-named version stored
in the EG function table. The closure copy is always called {closure}
at the moment, but the original version has this supposedly unique magic
name that I now want to make unique. Hence we have two separate issues:
-
What do we name the zend_function structure in the closure object? As
you suggest, it would make a lot of sense here to make this meaningful
to the programmer so {closure:<filename>:<start-line>-<end-line>} is a
lot better than the current {closure}. Hoisting your approach into
zend_closures.c would make a lot of sense. Note that this name doesn't
need to be unique. -
What do we call the template copy in the EG function table? This is
never exposed to the programmer, so it's name is unimportant in these
terms, but it should be unique for a given closure source.
Regards Terry
Dmitry,
The underlying issue here is the assumption that
build_runtime_defined_function_key() in zend_compile.c generates unique
keys. I've realised that this impacts not only closures but also
late-bound functions and classes. I've now produced PHPTs which
demonstrates the failure in all three cases. Whilst hashing a closure
source text has an acceptable compile-time overhead, the case for
classes (with their larger source lengths) is more of a concern.
I am now proposing an alternative hash based on:
A. The class / function name (as per current)
B. The reference source file (as per current)
C. The address of the buffer (as per current)
D. The PID and microtime at the request start.
E. A running count of the function/classed compiled during the request.
(A-C) were assumed unique but, as #64291 and #65915 show, are not.
(D) should be sufficient to prevent race conditions across different
OPcached threads / processes, but this has the advantage that it only
needs to be computed once per request.
(E) guarantees uniqueness within thread.
The only weakness of D and E alone is a possible race on ZTS builds
where two clashing threads start at the same microsecond, which (C) will
split. If this is considered too unwieldy then we could always drop B
as C-E guarantee uniqueness, and B is never exposed to the programmer
anyway (Xdebug and the error logger use the appropriate fields in the
zend_function / zend_class record and not its key in the corresponding
EG table.
This also has the advantage that the main change can now be localised to
build_runtime_defined_function_key().
//Terry
The following bugs relate to this discussion:
#64291 Indeterminate GC of evaled lambda function resources #65915 Inconsistent results with require return valueI don't want to discuss these or the specific fixes here, since I can
work up a fix and discuss them in the bugrep. However,
my one-sentence Q is "should we replace the naming convention for
closures with a truly unique one?" ...
hi Terry,
Please share the new PHPT tests or better attach them to bug report.
(D) looks reasonable simple. I thinks it must be a good enough solution.
For ZTS we can use thread_id in addition to PID (or instead of it). Also we
may use a simple thread specific counter instead of microtime -
CG(key_counter).
Thanks. Dmitry.
On Thu, Dec 5, 2013 at 9:17 PM, Terry Ellison ellison.terry@gmail.comwrote:
Dmitry,
The underlying issue here is the assumption that
build_runtime_defined_function_key() in zend_compile.c generates unique
keys. I've realised that this impacts not only closures but also
late-bound functions and classes. I've now produced PHPTs which
demonstrates the failure in all three cases. Whilst hashing a closure
source text has an acceptable compile-time overhead, the case for classes
(with their larger source lengths) is more of a concern.I am now proposing an alternative hash based on:
A. The class / function name (as per current)
B. The reference source file (as per current)
C. The address of the buffer (as per current)
D. The PID and microtime at the request start.
E. A running count of the function/classed compiled during the request.(A-C) were assumed unique but, as #64291 and #65915 show, are not.
(D) should be sufficient to prevent race conditions across different
OPcached threads / processes, but this has the advantage that it only needs
to be computed once per request.(E) guarantees uniqueness within thread.
The only weakness of D and E alone is a possible race on ZTS builds where
two clashing threads start at the same microsecond, which (C) will split.
If this is considered too unwieldy then we could always drop B as C-E
guarantee uniqueness, and B is never exposed to the programmer anyway
(Xdebug and the error logger use the appropriate fields in the
zend_function / zend_class record and not its key in the corresponding EG
table.This also has the advantage that the main change can now be localised to
build_runtime_defined_function_key().//Terry
The following bugs relate to this discussion:
#64291 Indeterminate GC of evaled lambda function resources #65915 Inconsistent results with require return valueI don't want to discuss these or the specific fixes here, since I can work
up a fix and discuss them in the bugrep. However,
my one-sentence Q is "should we replace the naming convention for closures
with a truly unique one?" ...