# Comments on non-unique naming convention for closures - Thread: https://externals.io/message/70237 - Original archive: https://news-web.php.net/php.internals/70237 - Started by Terry Ellison on 2013-11-20T16:41:46+00:00 - Messages: 14 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #70237 — Terry Ellison — 2013-11-20T16:41:46+00:00 URL: https://externals.io/message/70237#70237 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 --- ## #70268 — Ferenc Kovacs — 2013-11-21T20:05:52+00:00 URL: https://externals.io/message/70237#70268 · In reply to: #70237 2013.11.20. 17:42, "Terry Ellison" ezt írta: > > 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 > > -- > > > Bumping the thread and ccing Joe as we were having a similar discussion about the internal naming of the inner classes proposed by him. --- ## #70269 — Terry Ellison — 2013-11-22T00:06:30+00:00 URL: https://externals.io/message/70237#70269 · In reply to: #70268 > > 2013.11.20. 17:42, "Terry Ellison" > ezt írta: > > […] > work up a fix and discuss them in the bugrep. However, > > […] > 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 --- ## #70451 — Terry Ellison — 2013-11-30T00:30:08+00:00 URL: https://externals.io/message/70237#70451 · In reply to: #70269 > > […] > 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 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 --- ## #70452 — Stas Malyshev — 2013-11-30T03:46:31+00:00 URL: https://externals.io/message/70237#70452 · In reply to: #70451 Hi! > Having discussed the options with Dmitry and another contributor off PHP > Internals list, we have decided to base the generated > 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 --- ## #70453 — Terry Ellison — 2013-11-30T09:34:28+00:00 URL: https://externals.io/message/70237#70453 · In reply to: #70452 > > […] > 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 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 --- ## #70458 — Stas Malyshev — 2013-11-30T21:31:51+00:00 URL: https://externals.io/message/70237#70458 · In reply to: #70453 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 --- ## #70459 — Terry Ellison — 2013-12-01T00:30:20+00:00 URL: https://externals.io/message/70237#70459 · In reply to: #70458 > > […] > I see. That makes it better, but not completely, please see below. > > > […] I will expand in this further in a separate thread. > > […] > 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. > > […] > 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 --- ## #70479 — Stas Malyshev — 2013-12-03T08:25:08+00:00 URL: https://externals.io/message/70237#70479 · In reply to: #70459 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 --- ## #70483 — Terry Ellison — 2013-12-03T09:14:12+00:00 URL: https://externals.io/message/70237#70483 · In reply to: #70479 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 --- ## #70488 — Derick Rethans — 2013-12-03T10:19:19+00:00 URL: https://externals.io/message/70237#70488 · In reply to: #70237 > 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 --- ## #70491 — Terry Ellison — 2013-12-03T13:01:00+00:00 URL: https://externals.io/message/70237#70491 · In reply to: #70488 Hi Derick, > > > […] > 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::-} 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 --- ## #70501 — Terry Ellison — 2013-12-05T17:17:07+00:00 URL: https://externals.io/message/70237#70501 · In reply to: #70237 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 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?" ... --- ## #70544 — Dmitry Stogov — 2013-12-09T07:39:05+00:00 URL: https://externals.io/message/70237#70544 · In reply to: #70501 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 wrote: > 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 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?" ...